Re: [PATCH 5/13] KVM: virtualization infrastructure

[Arnd Bergmann]

On Monday 23 October 2006 22:28, Avi Kivity wrote:

> >> +struct segment_descriptor {
> >> +ÂÂÂÂu16 limit_low;
> >> +ÂÂÂÂu16 base_low;
> >> +ÂÂÂÂu8 Âbase_mid;
> >> +ÂÂÂÂu8 Âtype : 4;
> >> +ÂÂÂÂu8 Âsystem : 1;
> >> +ÂÂÂÂu8 Âdpl : 2;
> >> +ÂÂÂÂu8 Âpresent : 1;
> >> +ÂÂÂÂu8 Âlimit_high : 4;
> >> +ÂÂÂÂu8 Âavl : 1;
> >> +ÂÂÂÂu8 Âlong_mode : 1;
> >> +ÂÂÂÂu8 Âdefault_op : 1;
> >> +ÂÂÂÂu8 Âgranularity : 1;
> >> +ÂÂÂÂu8 Âbase_high;
> >> +} __attribute__((packed));
> >> Â Â
> >
> > Bitfields are generally frowned upon. It's better to define
> > constants for each of these and use a u64.
>
> Any specific reasons? ÂI find the code much more readable (and
> lowercase) with bitfields.

The strongest reason against bitfields is that they are not
endian-clean. This doesn't apply on a architecture-specific
patch such as KVM, but it just feels wrong to read code
with bit fields in the kernel.

> The structure's size is defined by the hardware (struvt vmcs is just a
> header). ÂIn addition, current_vmcs changes when another guest is
> switched in (it is somewhat like the scheduler's current for the VT
> hardware).

Ok, I see.

> >> +static long kvm_dev_ioctl(struct file *filp,
> >> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ Âunsigned int ioctl, unsigned long arg)
> >> +{
> >> +ÂÂÂÂstruct kvm *kvm = filp->private_data;
> >> +ÂÂÂÂint r = -EINVAL;
> >> +
> >> +ÂÂÂÂswitch (ioctl) {
> >> +ÂÂÂÂdefault:
> >> +ÂÂÂÂÂÂÂÂÂÂÂÂ;
> >> +ÂÂÂÂ}
> >> +out:
> >> +ÂÂÂÂreturn r;
> >> +}
> >> Â Â
> >
> > Huh? Just leave out stuff like this. If the ioctl function is introduced
> > in a later patch, you can still add the whole function there.Â
>
> Several different patches add content here, so I thought I wouldn't play
> favorite.
>
> It also makes reordering the patches a little less painful. ÂAny tips on
> that or is that a normal ramp up? ÂI'm using quilt for now and syncing
> to a conventional source control repository.

I saw later how you add specific calls to this function. I guess it's
already as good as it gets, so just leave it this way.

	Arnd <><
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/