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 05:19 PM, Lengyel, Tamas wrote:
> 
> 
> On Wed, Jun 3, 2015 at 3:48 PM, Razvan Cojocaru
> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote:
> 
>     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.
> 
> 
> Right now if you want to access a bit using the index on the ctrlreg
> fields, you would for example do (monitor.write_ctrlreg_enabled &
> index). This is actually going to perform two bitmask operations. First,
> monitor.write_ctrlreg_enabled does a masking operating to get you only
> the 4 bits corresponding to this field, then you do another mask with
> the index. In general bitmasks are pretty slow operations, thus IMHO
> minimizing them would be ideal. What I meant by doing a uint8_t
> separately would be like "uint8_t write_ctrlreg_enabled;" which would
> eliminate the first bitmask operation. You would have to do that for the
> other two bitfields as well, write_ctrlreg_sync and
> write_ctrlreg_onchangeonly. The trade-off with this is that now we will
> have 12 bits that are not used for anything (yet). On the other hand
> this way we could also reduce the size of the remaining bitfield from
> uint16_t to uint8_t, so IMHO it's not that big of an issue.

Right, I've assumed that you weren't talking about that, since the
initial patch used more bits than strictly necessary for storing the
current number of events, and you preferred the patch to use the least
possible ammount of bits to store the info, but using dedicated uint8_ts
is indeed a slightly different approach.

> Sorry for the last minute addition here, I know you want to close this
> patch now that you have the acks on it. If the maintainers don't share
> my concern here then feel free to ignore it ;)

No problem, I'd rather have it done right than quick, so if extra
shifting to get to those fields trumps the space economy bitfields
provide, I'll send a new version of the patch. It it after all not that
great of a modification.

Waiting for maintainers' / other input.


Thanks,
Razvan