Re: [PATCH V9] xen/vm_event: Clean up control-register-write vm_events and add XCR0 event

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 06/03/2015 04:29 PM, Lengyel, Tamas wrote:
> 
> 
> 
>     diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
>     index 45b5283..a3c117f 100644
>     --- a/xen/include/asm-x86/domain.h
>     +++ b/xen/include/asm-x86/domain.h
>     @@ -341,15 +341,9 @@ struct arch_domain
> 
>          /* Monitor options */
>          struct {
>     -        uint16_t mov_to_cr0_enabled          : 1;
>     -        uint16_t mov_to_cr0_sync             : 1;
>     -        uint16_t mov_to_cr0_onchangeonly     : 1;
>     -        uint16_t mov_to_cr3_enabled          : 1;
>     -        uint16_t mov_to_cr3_sync             : 1;
>     -        uint16_t mov_to_cr3_onchangeonly     : 1;
>     -        uint16_t mov_to_cr4_enabled          : 1;
>     -        uint16_t mov_to_cr4_sync             : 1;
>     -        uint16_t mov_to_cr4_onchangeonly     : 1;
>     +        uint16_t write_ctrlreg_enabled       : 4;
>     +        uint16_t write_ctrlreg_sync          : 4;
>     +        uint16_t write_ctrlreg_onchangeonly  : 4;
> 
> 
> Just looking at this here again, we will now have a bitmap within a
> bitmap, which doesn't seem to be very efficient. IMHO it would be better
> to just take the ctrlreg bitmap out as a separate uint8_t within struct
> {} monitor.

How is it inefficient? I don't see that at all. And I'm not sure what
you mean about the uint8_t: there are 3 fields there, each 4-bits wide
(write_ctrlreg_enabled, write_ctrlreg_sync, write_ctrlreg_onchangeonly)
and only (at most) two of them would fit into a uint8_t. To put them all
into a new struct would mean wasting an uint16_t for 12-bits.

You might argue that it's not as clean-cut as having one explicitly
specified bit for each control register (like mov_to_cr0_enabled,
mov_to_cr3_enabled, etc. were before), but that's intentional, as it
makes it trivial to add a new control register write event: just go
ahead and "#define VM_EVENT_X86_XCR1  4", for example, and then all you
need to do is make sure that write_ctrlreg_enabled is wide enough to
hold 5 events, and that's it, you can use the new event in the HV and libxc.

Having explicit 1-bit fields for each new event (which I think is what
you're proposing) makes it much more work to add a new ctrlreg event,
and it defeats the purpose of much of the conversations I've had with
Jan about all those shifting and convenience macros.


Thanks,
Razvan