Re: [Xen-devel] [PATCH v4 4/6] VMX: Make sure PI is in proper state before install the hooks

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Monday, September 26, 2016 8:46 PM
> To: Wu, Feng <feng.wu@xxxxxxxxx>
> Cc: andrew.cooper3@xxxxxxxxxx; dario.faggioli@xxxxxxxxxx;
> george.dunlap@xxxxxxxxxxxxx; Tian, Kevin <kevin.tian@xxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxx
> 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@xxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel