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

[Arnd Bergmann <arnd@arndb.de>]

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 <><