| Message ID | 1474425470-3629-5-git-send-email-feng.wu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote: > We may hit the ASSERT() in vmx_vcpu_block in the current code, There are three of them - please be explicit which one is what gets dealt with here. > --- 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 = 0xff; Is this a proper "invalid" indicator in the x2APIC case? I would have assumed you want 0xffffffff in that case. > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -211,14 +211,39 @@ static void vmx_pi_list_cleanup(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 > + */ Stray tabs. > + (void)cmpxchg(&pi_desc->ndst, 0xff, > + x2apic_enabled ? dest : MASK_INSR(dest, PI_xAPIC_NDST_MASK)); Here even more than above I think you want to derive the xAPIC value from the wider x2APIC one not just for the value to write, but also for the value to compare against. If you did so in pi_desc_init() as well as here, I don't see a strict need for introducing a suitable #define. If you don't, however, you definitely want a pair of #define-s to one can associate both places with one another. But overall I think this is much better than the previous version, so thanks for doing (and testing) this. Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Monday, September 26, 2016 8:46 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 v4 4/6] VMX: Make sure PI is in proper state before install > the hooks > > >>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote: > > We may hit the ASSERT() in vmx_vcpu_block in the current code, > > There are three of them - please be explicit which one is what gets > dealt with here. > > > --- 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 = 0xff; > > Is this a proper "invalid" indicator in the x2APIC case? I would have > assumed you want 0xffffffff in that case. Right. We should use 0xffffffff here. Thanks a lot! Thanks, Feng > > > --- a/xen/arch/x86/hvm/vmx/vmx.c > > +++ b/xen/arch/x86/hvm/vmx/vmx.c > > @@ -211,14 +211,39 @@ static void vmx_pi_list_cleanup(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 > > + */ > > Stray tabs. > > > + (void)cmpxchg(&pi_desc->ndst, 0xff, > > + x2apic_enabled ? dest : MASK_INSR(dest, PI_xAPIC_NDST_MASK)); > > Here even more than above I think you want to derive the xAPIC > value from the wider x2APIC one not just for the value to write, but > also for the value to compare against. If you did so in pi_desc_init() > as well as here, I don't see a strict need for introducing a suitable > #define. If you don't, however, you definitely want a pair of > #define-s to one can associate both places with one another. > > But overall I think this is much better than the previous version, so > thanks for doing (and testing) this. > > Jan
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c index 1bd875a..e733753 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 = 0xff; } 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 821cba2..09262d5 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_list_cleanup(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, 0xff, + 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 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> --- v4: - This patch is previously called "Pause/Unpause the domain before/after assigning PI hooks" - Remove the pause/unpause method - Use cmpxchg to update NDST xen/arch/x86/hvm/vmx/vmcs.c | 13 +++++-------- xen/arch/x86/hvm/vmx/vmx.c | 27 ++++++++++++++++++++++++++- 2 files changed, 31 insertions(+), 9 deletions(-)