Re: [Xen-devel] [v3 10/15] vt-d: Add API to update IRTE when VT-d PI is used

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Wednesday, July 15, 2015 4:25 PM
> To: Wu, Feng
> Cc: andrew.cooper3@xxxxxxxxxx; george.dunlap@xxxxxxxxxxxxx; Tian, Kevin;
> Zhang, Yang Z; xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx
> Subject: RE: [v3 10/15] vt-d: Add API to update IRTE when VT-d PI is used
> 
> >>> On 15.07.15 at 08:04, <feng.wu@xxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: Friday, July 10, 2015 10:02 PM
> >> I'm particularly worried by the call to acpi_find_matched_drhd_unit()
> >> - is it maybe worth storing the iommu pointer in struct msi_desc?
> >
> > I think it worth, Like Andrew also mentioned this point before. I tend
> > to make this a independent work and do it later, since the 4.6 release
> > is coming, I am still try my best to target it. Could you please share
> > your concern here, performance? Or other things? Thanks!
> 
> Interrupt latency in particular.

This update IRTE operation is not so frequently. It only happens in few times,
especially in the initialization phase of the guest. And even the guest set
the affinity, in the MSI/MSIx configuration doesn't change, QEMU will not
ask Xen to update it.

> 
> >> > +    GET_IREMAP_ENTRY(ir_ctrl->iremap_maddr, remap_index,
> >> iremap_entries, p);
> >> > +    new_ire = *p;
> >> > +
> >> > +    /* Setup/Update interrupt remapping table entry. */
> >> > +    setup_posted_irte(&new_ire, pi_desc, gvec);
> >> > +
> >> > +    do {
> >> > +        old_ire = *(uint128_t *)p;
> >>
> >> This cast suggests that you might want to go beyond what Andrew
> >> said on cmpxchg16b()'s parameters: Perhaps they'd better be
> >> void * instead of uint128_t *.
> >
> > In that case, I need to do the cast in __cmpxchg16b(), right?
> 
> Where needed, yes. But that would limit casting to just a single place.
> 
> >> > +        ret = cmpxchg16b(p, &old_ire, &new_ire);
> >> > +    } while ( memcmp(&ret, &old_ire, sizeof(old_ire)) );
> >>
> >> Doesn't setup_posted_irte() need to move inside this loop, as it
> >> tries to preserve certain fields? Or else, what is the cmpxchg16b
> >> loop guarding against (i.e. why isn't this just a single one)?
> >
> > Why need we move setup_posted_irte() inside the loop? "new_ire"
> > will not be changed after setup, right? Here we need to make sure
> > the 128b IRTE is updated atomically, especially for the high part
> > of posted-interrupt descriptor address and the low part of it.
> 
> There are two possible scenarios:
> 
> 1) There are bits that can be updated behind the back of the code
> here. In that case you need to loop, and each iteration of the loop
> needs to re-fetch the current value (not doing so would make the
> loop infinite).

Oh, yes, I think I made a mistake here, it is too hastily these days,
Sorry for that! I think I need do it like this:

    do {
        new_ire = *p;

        /* Setup/Update interrupt remapping table entry. */
        setup_posted_irte(&new_ire, pi_desc, gvec);

        old_ire = *(uint128_t *)p;
        ret = cmpxchg16b(p, &old_ire, &new_ire);
    } while ( memcmp(&ret, &old_ire, sizeof(old_ire)) );

Thanks,
Feng

> 
> 2) No racing updates are possible; all you care about is atomicity
> of the update. In that case you don't need a loop around the
> cmpxchg16b().
> 
> Jan


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