From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Wu, Feng" Subject: Re: [v3 06/15] vmx: Extend struct pi_desc to support VT-d Posted-Interrupts Date: Wed, 15 Jul 2015 09:30:34 +0000 Message-ID: References: <1435123109-10481-1-git-send-email-feng.wu@intel.com> <1435123109-10481-7-git-send-email-feng.wu@intel.com> <559FDFD4020000780008F85F@mail.emea.novell.com> <55A633DA02000078000911AB@mail.emea.novell.com> <55A637A202000078000911EF@mail.emea.novell.com> <55A643D7020000780009133E@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55A643D7020000780009133E@mail.emea.novell.com> Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: "Tian, Kevin" , "keir@xen.org" , "george.dunlap@eu.citrix.com" , "andrew.cooper3@citrix.com" , "xen-devel@lists.xen.org" , "Zhang, Yang Z" , "Wu, Feng" List-Id: xen-devel@lists.xenproject.org > -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Wednesday, July 15, 2015 5:28 PM > To: Wu, Feng > Cc: andrew.cooper3@citrix.com; george.dunlap@eu.citrix.com; Tian, Kevin; > Zhang, Yang Z; xen-devel@lists.xen.org; keir@xen.org > Subject: RE: [v3 06/15] vmx: Extend struct pi_desc to support VT-d > Posted-Interrupts > > >>> On 15.07.15 at 10:43, wrote: > > > > >> -----Original Message----- > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: Wednesday, July 15, 2015 4:36 PM > >> To: Wu, Feng > >> Cc: andrew.cooper3@citrix.com; george.dunlap@eu.citrix.com; Tian, Kevin; > >> Zhang, Yang Z; xen-devel@lists.xen.org; keir@xen.org > >> Subject: RE: [v3 06/15] vmx: Extend struct pi_desc to support VT-d > >> Posted-Interrupts > >> > >> >>> On 15.07.15 at 10:26, wrote: > >> > >> > > >> >> -----Original Message----- > >> >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> >> Sent: Wednesday, July 15, 2015 4:20 PM > >> >> To: Wu, Feng > >> >> Cc: andrew.cooper3@citrix.com; george.dunlap@eu.citrix.com; Tian, > Kevin; > >> >> Zhang, Yang Z; xen-devel@lists.xen.org; keir@xen.org > >> >> Subject: RE: [v3 06/15] vmx: Extend struct pi_desc to support VT-d > >> >> Posted-Interrupts > >> >> > >> >> >>> On 15.07.15 at 04:40, wrote: > >> >> > >> >> > > >> >> >> -----Original Message----- > >> >> >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> >> >> Sent: Friday, July 10, 2015 9:08 PM > >> >> >> To: Wu, Feng > >> >> >> Cc: andrew.cooper3@citrix.com; george.dunlap@eu.citrix.com; Tian, > >> Kevin; > >> >> >> Zhang, Yang Z; xen-devel@lists.xen.org; keir@xen.org > >> >> >> Subject: Re: [v3 06/15] vmx: Extend struct pi_desc to support VT-d > >> >> >> Posted-Interrupts > >> >> >> > >> >> >> >>> On 24.06.15 at 07:18, wrote: > >> >> >> > @@ -81,8 +81,19 @@ struct vmx_domain { > >> >> >> > > >> >> >> > struct pi_desc { > >> >> >> > DECLARE_BITMAP(pir, NR_VECTORS); > >> >> >> > - u32 control; > >> >> >> > - u32 rsvd[7]; > >> >> >> > + union { > >> >> >> > + struct > >> >> >> > + { > >> >> >> > + u16 on : 1, /* bit 256 - Outstanding Notification */ > >> >> >> > + sn : 1, /* bit 257 - Suppress Notification */ > >> >> >> > + rsvd_1 : 14; /* bit 271:258 - Reserved */ > >> >> >> > + u8 nv; /* bit 279:272 - Notification Vector */ > >> >> >> > + u8 rsvd_2; /* bit 287:280 - Reserved */ > >> >> >> > + u32 ndst; /* bit 319:288 - Notification > Destination > >> */ > >> >> >> > + }; > >> >> >> > + u64 control; > >> >> >> > + }; > >> >> >> > >> >> >> So current code, afaics, uses e.g. test_and_set_bit() to set ON. > >> >> >> By also declaring this as a bitfield you're opening the structure for > >> >> >> non-atomic accesses. If that's correct, why is other code not > >> >> >> being changed to _only_ use the bitfield mechanism (likely also > >> >> >> eliminating the need for it being a union with the now 64-bit > >> >> >> "control"? If atomic accesses are required, then I'd strongly > >> >> >> suggest against making this a bit field. > >> >> >> > >> >> >> And in no event can I see why "ndst" needs to be union-ized > >> >> >> with "control" if it doesn't need to be updated atomically with > >> >> >> e.g. "nv". > >> >> >> > >> >> > > >> >> > When the vCPU is to be blocked, we need to atomically update > >> >> > the "nv" and "ndst", then the wakeup notification event can be > >> >> > delivered to the right destination. > >> >> > >> >> Okay. Your reply made me go through the patches again to check > >> >> where updates to nv/ndst happen - what's the reason they aren't > >> >> being updated as a pair in patch 14's RUNSTATE_running handling > >> >> (or in the replacement draft's vmx_ctxt_switch_to() adjustment)? > >> > > >> > It is because, we can only enter running state from runnable, in which, > >> > the NV field has been already changed back to ' posted_intr_vector ', > >> > we don't need to do it here again. > >> > >> Without sitting in the runstate update path anymore, I can't see how > >> you would get to see all transitions to runnable. > > > > Sorry, I cannot understanding the above comments well. Do you mean > > after using the new method (arch hooks ) to update posted-interrupt > > descriptor, I cannot track all the state transitions to runnable? > > Not sure if "track" is the right word here, but yes. > The new method is still in development, let's see how it will be then. :) Thanks, Feng > Jan