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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

>> > +    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).

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