From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH V9] xen/vm_event: Clean up control-register-write vm_events and add XCR0 event Date: Wed, 03 Jun 2015 16:48:15 +0300 Message-ID: <556F059F.8090901@bitdefender.com> References: <1433331352-18287-1-git-send-email-rcojocaru@bitdefender.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "Lengyel, Tamas" Cc: kevin.tian@intel.com, wei.liu2@citrix.com, Ian Campbell , Stefano Stabellini , tim@xen.org, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, Jan Beulich , eddie.dong@intel.com, Jun Nakajima , andrew.cooper3@citrix.com, keir@xen.org List-Id: xen-devel@lists.xenproject.org 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