Re: [PATCH V2 2/3] xen/vm_event: Support for guest-requested events

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 06/30/2015 05:48 PM, Lengyel, Tamas wrote:
>>> --- a/xen/include/asm-x86/domain.h
> 
>     >> +++ b/xen/include/asm-x86/domain.h
>     >> @@ -342,13 +342,15 @@ struct arch_domain
>     >>
>     >>      /* Monitor options */
>     >>      struct {
>     >> -        uint16_t write_ctrlreg_enabled       : 4;
>     >> -        uint16_t write_ctrlreg_sync          : 4;
>     >> -        uint16_t write_ctrlreg_onchangeonly  : 4;
>     >> -        uint16_t mov_to_msr_enabled          : 1;
>     >> -        uint16_t mov_to_msr_extended         : 1;
>     >> -        uint16_t singlestep_enabled          : 1;
>     >> -        uint16_t software_breakpoint_enabled : 1;
>     >> +        uint32_t write_ctrlreg_enabled       : 4;
>     >> +        uint32_t write_ctrlreg_sync          : 4;
>     >> +        uint32_t write_ctrlreg_onchangeonly  : 4;
>     >> +        uint32_t mov_to_msr_enabled          : 1;
>     >> +        uint32_t mov_to_msr_extended         : 1;
>     >> +        uint32_t singlestep_enabled          : 1;
>     >> +        uint32_t software_breakpoint_enabled : 1;
>     >> +        uint32_t request_enabled             : 1;
>     >> +        uint32_t request_sync                : 1;
>     >
>     > Can you please switch to plain unsigned int if you already have to
>     > touch this? There's no reason I can see to use a fixed width integer
>     > type here.
> 
>     Ack, will make it plain int.
> 
> 
> IMHO having it fix-width is easier to read when adding new elements to
> see how many bits we have left free. I would not want this changed
> unless there is a clear benefit to doing so.

I don't really have a preference one way or the other about this based
on readability, so I'm fine with going either way.

However, while in practice this modification is safe, AFAIK the C
standard does not guarantee that an (unsigned) int is at least 32-bits
wide, so it might raise the question of what might happen if somebody
builds the hypervisor with a 16-bit int compiler (though I would imagine
there would be some compile-time warning / error in that case). Do we
care about that?


Thanks,
Razvan