Re: [Xen-devel] [PATCH v11 1/2] vmx: VT-d posted-interrupt core logic handling

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Friday, January 29, 2016 5:32 PM
> To: Wu, Feng <feng.wu@xxxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Dario Faggioli
> <dario.faggioli@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxxxxx>;
> Tian, Kevin <kevin.tian@xxxxxxxxx>; xen-devel@xxxxxxxxxxxxx; Keir Fraser
> <keir@xxxxxxx>
> Subject: RE: [PATCH v11 1/2] vmx: VT-d posted-interrupt core logic handling
> 
> >>> On 29.01.16 at 02:53, <feng.wu@xxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: Friday, January 29, 2016 12:38 AM
> >> >>> On 28.01.16 at 06:12, <feng.wu@xxxxxxxxx> wrote:
> >> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> >> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> >> > @@ -83,7 +83,140 @@ static int vmx_msr_write_intercept(unsigned int
> msr,
> >> uint64_t msr_content);
> >> >  static void vmx_invlpg_intercept(unsigned long vaddr);
> >> >  static int vmx_vmfunc_intercept(struct cpu_user_regs *regs);
> >> >
> >> > +static void vmx_vcpu_block(struct vcpu *v)
> >> > +{
> >> > +    unsigned long flags;
> >> > +    unsigned int dest;
> >> > +
> >> > +    struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
> >>
> >> Stray blank line between declarations.
> >>
> >> > +    ASSERT(v->arch.hvm_vmx.pi_block_list_lock == NULL);
> >>
> >> I think this needs to be done after acquiring the lock.
> >
> > I am not sure what you mean here. The ASSERT here means that
> > the vCPU is not on any pCPU list when we get here.
> 
> My point is that until you've taken the lock, whether the vCPU is on
> any such list may change. All state updates are done under lock, so
> checking for valid incoming state should be done under lock too, to
> be most useful.

I don't think so. We get this function via vcpu_block()/do_poll() ->
arch_vcpu_block() -> ... -> vmx_vcpu_block(), and the parameter
of this function should point to the current vCPU, so when we
get here, the v->arch.hvm_vmx.pi_block_list_lock should be NULL,
which means the vCPU is not in any blocking. and no one can change
it behind us this this moment.

> >> > +static void vmx_pi_do_resume(struct vcpu *v)
> >> > +{
> >> > +    unsigned long flags;
> >> > +    spinlock_t *pi_block_list_lock;
> >> > +    struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
> >> > +
> >> > +    ASSERT(!test_bit(_VPF_blocked, &v->pause_flags));
> >> > +
> >> > +    /*
> >> > +     * Set 'NV' field back to posted_intr_vector, so the
> >> > +     * Posted-Interrupts can be delivered to the vCPU when
> >> > +     * it is running in non-root mode.
> >> > +     */
> >> > +    if ( pi_desc->nv != posted_intr_vector )
> >> > +        write_atomic(&pi_desc->nv, posted_intr_vector);
> >>
> >> Perhaps this was discussed before, but I don't recall and now
> >> wonder - why inside an if()? This is a simple memory write on
> >> x86.
> >
> > The initial purpose is that if NV is already equal to 'posted_intr_vector',
> > we can save the following atomically write operation. There are some
> > volatile stuff and barriers in write_atomic().
> 
> But what does the final generated code look like? As I said, I'm
> sure it's just a single MOV. And putting a conditional around it will
> likely make things slower rather than faster.

Looking at the implementation of wirte_atomic(), it has volatile key
word barrier inside, if you think this is not a big deal, I am fine to
remove the check.

> 
> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> > @@ -2293,6 +2293,8 @@ static int reassign_device_ownership(
> >> >          pdev->domain = target;
> >> >      }
> >> >
> >> > +    vmx_pi_hooks_reassign(source, target);
> >> > +
> >> >      return ret;
> >> >  }
> >>
> >> I think you need to clear source's hooks here, but target's need to
> >> get set ahead of the actual assignment.
> >
> > I think this is the place where the device is moved from
> > &source->arch.pdev_list to &target->arch.pdev_list, if that is the
> > case, we should clear source and set target here, right?
> 
> No - target needs to be ready to deal with events from the device
> _before_ the device actually gets assigned to it.

I still don't get your point. I still think this is the place where the device
is being got assigned. Or maybe you can share more info about the
place "_before_ the device actually gets assigned to it " ?

Thanks,
Feng

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