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

["Lengyel, Tamas" <tlengyel@xxxxxxxxxxx>]

On Wed, Jun 3, 2015 at 3:48 PM, Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx> 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.

Â

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.

I'm fine with having the index-based bitfield approach, don't get me wrong. This is about how those bitfields are stored.
Â

Thanks,
Razvan

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

Cheers!

--

Tamas K Lengyel

Senior Security Researcher

7921 Jones Branch Drive

McLean VA 22102

Email Âtlengyel@novetta.com

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel