| Message ID | 1476147473-30970-5-git-send-email-feng.wu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 11.10.16 at 02:57, <feng.wu@intel.com> wrote: > static void pi_desc_init(struct vcpu *v) > { > - uint32_t dest; > - > v->arch.hvm_vmx.pi_desc.nv = posted_intr_vector; > > - dest = cpu_physical_id(v->processor); > - > - if ( x2apic_enabled ) > - v->arch.hvm_vmx.pi_desc.ndst = dest; > - else > - v->arch.hvm_vmx.pi_desc.ndst = MASK_INSR(dest, PI_xAPIC_NDST_MASK); > + /* > + * Mark NDST as invalid, then we can use this invalid value as a > + * marker to whether update NDST or not in vmx_pi_hooks_assign(). > + */ > + v->arch.hvm_vmx.pi_desc.ndst = 0xffffffff; I think I had at the same time asked to make this a #define, so the two (currently) instance can be connected together. > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -211,14 +211,39 @@ static void vmx_pi_do_resume(struct vcpu *v) > /* This function is called when pcidevs_lock is held */ > void vmx_pi_hooks_assign(struct domain *d) > { > + struct vcpu *v; > + > if ( !iommu_intpost || !has_hvm_container_domain(d) ) > return; > > ASSERT(!d->arch.hvm_domain.vmx.vcpu_block); > > - d->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block; > + /* > + * We carefully handle the timing here: > + * - Install the context switch first > + * - Then set the NDST field > + * - Install the block and resume hooks in the end > + * > + * This can make sure the PI (especially the NDST feild) is > + * in proper state when we call vmx_vcpu_block(). > + */ > d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from; > d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to; > + > + for_each_vcpu ( d, v ) > + { > + unsigned int dest = cpu_physical_id(v->processor); > + struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc; > + > + /* > + * We don't need to update NDST if 'arch.hvm_domain.vmx.pi_switch_to' > + * already gets called I think you mean "got" or "has got". Also I think you'd better refer to the actual VMX function instead of the hook pointer field. Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Wednesday, October 12, 2016 9:45 PM > To: Wu, Feng <feng.wu@intel.com> > Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; > george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- > devel@lists.xen.org > Subject: Re: [PATCH v5 4/7] VMX: Make sure PI is in proper state before install > the hooks > > >>> On 11.10.16 at 02:57, <feng.wu@intel.com> wrote: > > static void pi_desc_init(struct vcpu *v) > > { > > - uint32_t dest; > > - > > v->arch.hvm_vmx.pi_desc.nv = posted_intr_vector; > > > > - dest = cpu_physical_id(v->processor); > > - > > - if ( x2apic_enabled ) > > - v->arch.hvm_vmx.pi_desc.ndst = dest; > > - else > > - v->arch.hvm_vmx.pi_desc.ndst = MASK_INSR(dest, > PI_xAPIC_NDST_MASK); > > + /* > > + * Mark NDST as invalid, then we can use this invalid value as a > > + * marker to whether update NDST or not in vmx_pi_hooks_assign(). > > + */ > > + v->arch.hvm_vmx.pi_desc.ndst = 0xffffffff; > > I think I had at the same time asked to make this a #define, so the > two (currently) instance can be connected together. Sorry, Maybe I didn't get that. Do you mean I need to define a Macro for 0xffffffff, so we can use it here and in vmx.c? Thanks, Feng
>>> On 17.10.16 at 08:26, <feng.wu@intel.com> wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Wednesday, October 12, 2016 9:45 PM >> >>> On 11.10.16 at 02:57, <feng.wu@intel.com> wrote: >> > static void pi_desc_init(struct vcpu *v) >> > { >> > - uint32_t dest; >> > - >> > v->arch.hvm_vmx.pi_desc.nv = posted_intr_vector; >> > >> > - dest = cpu_physical_id(v->processor); >> > - >> > - if ( x2apic_enabled ) >> > - v->arch.hvm_vmx.pi_desc.ndst = dest; >> > - else >> > - v->arch.hvm_vmx.pi_desc.ndst = MASK_INSR(dest, >> PI_xAPIC_NDST_MASK); >> > + /* >> > + * Mark NDST as invalid, then we can use this invalid value as a >> > + * marker to whether update NDST or not in vmx_pi_hooks_assign(). >> > + */ >> > + v->arch.hvm_vmx.pi_desc.ndst = 0xffffffff; >> >> I think I had at the same time asked to make this a #define, so the >> two (currently) instance can be connected together. > > Sorry, Maybe I didn't get that. Do you mean I need to define a Macro > for 0xffffffff, so we can use it here and in vmx.c? Yes. Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Monday, October 24, 2016 3:23 PM > To: Wu, Feng <feng.wu@intel.com> > Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; > george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- > devel@lists.xen.org > Subject: RE: [PATCH v5 4/7] VMX: Make sure PI is in proper state before install > the hooks > > >>> On 17.10.16 at 08:26, <feng.wu@intel.com> wrote: > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: Wednesday, October 12, 2016 9:45 PM > >> >>> On 11.10.16 at 02:57, <feng.wu@intel.com> wrote: > >> > static void pi_desc_init(struct vcpu *v) > >> > { > >> > - uint32_t dest; > >> > - > >> > v->arch.hvm_vmx.pi_desc.nv = posted_intr_vector; > >> > > >> > - dest = cpu_physical_id(v->processor); > >> > - > >> > - if ( x2apic_enabled ) > >> > - v->arch.hvm_vmx.pi_desc.ndst = dest; > >> > - else > >> > - v->arch.hvm_vmx.pi_desc.ndst = MASK_INSR(dest, > >> PI_xAPIC_NDST_MASK); > >> > + /* > >> > + * Mark NDST as invalid, then we can use this invalid value as a > >> > + * marker to whether update NDST or not in vmx_pi_hooks_assign(). > >> > + */ > >> > + v->arch.hvm_vmx.pi_desc.ndst = 0xffffffff; > >> > >> I think I had at the same time asked to make this a #define, so the > >> two (currently) instance can be connected together. > > > > Sorry, Maybe I didn't get that. Do you mean I need to define a Macro > > for 0xffffffff, so we can use it here and in vmx.c? > > Yes. Thanks for confirm. Which one do you like? Put the macro in vmx.h or asm/apic.h? Thanks, Feng > > Jan
>>> On 24.10.16 at 09:45, <feng.wu@intel.com> wrote: > >> -----Original Message----- >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Monday, October 24, 2016 3:23 PM >> To: Wu, Feng <feng.wu@intel.com> >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- >> devel@lists.xen.org >> Subject: RE: [PATCH v5 4/7] VMX: Make sure PI is in proper state before > install >> the hooks >> >> >>> On 17.10.16 at 08:26, <feng.wu@intel.com> wrote: >> >> From: Jan Beulich [mailto:JBeulich@suse.com] >> >> Sent: Wednesday, October 12, 2016 9:45 PM >> >> >>> On 11.10.16 at 02:57, <feng.wu@intel.com> wrote: >> >> > static void pi_desc_init(struct vcpu *v) >> >> > { >> >> > - uint32_t dest; >> >> > - >> >> > v->arch.hvm_vmx.pi_desc.nv = posted_intr_vector; >> >> > >> >> > - dest = cpu_physical_id(v->processor); >> >> > - >> >> > - if ( x2apic_enabled ) >> >> > - v->arch.hvm_vmx.pi_desc.ndst = dest; >> >> > - else >> >> > - v->arch.hvm_vmx.pi_desc.ndst = MASK_INSR(dest, >> >> PI_xAPIC_NDST_MASK); >> >> > + /* >> >> > + * Mark NDST as invalid, then we can use this invalid value as a >> >> > + * marker to whether update NDST or not in vmx_pi_hooks_assign(). >> >> > + */ >> >> > + v->arch.hvm_vmx.pi_desc.ndst = 0xffffffff; >> >> >> >> I think I had at the same time asked to make this a #define, so the >> >> two (currently) instance can be connected together. >> > >> > Sorry, Maybe I didn't get that. Do you mean I need to define a Macro >> > for 0xffffffff, so we can use it here and in vmx.c? >> >> Yes. > > Thanks for confirm. Which one do you like? Put the macro in vmx.h > or asm/apic.h? This being (for now at least) VMX-specific, I think the former is to be preferred. Jan
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c index 1bd875a..10976bd 100644 --- a/xen/arch/x86/hvm/vmx/vmcs.c +++ b/xen/arch/x86/hvm/vmx/vmcs.c @@ -956,16 +956,13 @@ void virtual_vmcs_vmwrite(const struct vcpu *v, u32 vmcs_encoding, u64 val) */ static void pi_desc_init(struct vcpu *v) { - uint32_t dest; - v->arch.hvm_vmx.pi_desc.nv = posted_intr_vector; - dest = cpu_physical_id(v->processor); - - if ( x2apic_enabled ) - v->arch.hvm_vmx.pi_desc.ndst = dest; - else - v->arch.hvm_vmx.pi_desc.ndst = MASK_INSR(dest, PI_xAPIC_NDST_MASK); + /* + * Mark NDST as invalid, then we can use this invalid value as a + * marker to whether update NDST or not in vmx_pi_hooks_assign(). + */ + v->arch.hvm_vmx.pi_desc.ndst = 0xffffffff; } static int construct_vmcs(struct vcpu *v) diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index b8b152a..b14c84e 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -211,14 +211,39 @@ static void vmx_pi_do_resume(struct vcpu *v) /* This function is called when pcidevs_lock is held */ void vmx_pi_hooks_assign(struct domain *d) { + struct vcpu *v; + if ( !iommu_intpost || !has_hvm_container_domain(d) ) return; ASSERT(!d->arch.hvm_domain.vmx.vcpu_block); - d->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block; + /* + * We carefully handle the timing here: + * - Install the context switch first + * - Then set the NDST field + * - Install the block and resume hooks in the end + * + * This can make sure the PI (especially the NDST feild) is + * in proper state when we call vmx_vcpu_block(). + */ d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from; d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to; + + for_each_vcpu ( d, v ) + { + unsigned int dest = cpu_physical_id(v->processor); + struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc; + + /* + * We don't need to update NDST if 'arch.hvm_domain.vmx.pi_switch_to' + * already gets called + */ + (void)cmpxchg(&pi_desc->ndst, 0xffffffff, + x2apic_enabled ? dest : MASK_INSR(dest, PI_xAPIC_NDST_MASK)); + } + + d->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block; d->arch.hvm_domain.vmx.pi_do_resume = vmx_pi_do_resume; }
We may hit the last ASSERT() in vmx_vcpu_block in the current code, since vmx_vcpu_block() may get called before vmx_pi_switch_to() has been installed or executed. Here We use cmpxchg to update the NDST field, this can make sure we only update the NDST when vmx_pi_switch_to() has not been called. So the NDST is in a proper state in vmx_vcpu_block(). Suggested-by: Jan Beulich <JBeulich@suse.com> Signed-off-by: Feng Wu <feng.wu@intel.com> --- v5: - Use 0xffffffff as the invalid value for NDST field. xen/arch/x86/hvm/vmx/vmcs.c | 13 +++++-------- xen/arch/x86/hvm/vmx/vmx.c | 27 ++++++++++++++++++++++++++- 2 files changed, 31 insertions(+), 9 deletions(-)