On Wed, Jun 3, 2015 at 3:48 PM, Razvan Cojocaru <rcojoc...@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.
>
> 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!
--
[image: www.novetta.com]
Tamas K Lengyel
Senior Security Researcher
7921 Jones Branch Drive
McLean VA 22102
Email tleng...@novetta.com
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
