Messages in this thread | | | From | Arnd Bergmann <> | Subject | Re: [PATCH 5/13] KVM: virtualization infrastructure | Date | Mon, 23 Oct 2006 22:35:29 +0200 |
| |
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |