Re: [Xen-devel] [PATCH v4 5/6] VT-d: No need to set irq affinity for posted format IRTE

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.09.16 at 08:51, <feng.wu@xxxxxxxxx> wrote:

> 
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: Monday, September 26, 2016 8:58 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 5/6] VT-d: No need to set irq affinity for posted 
> format
>> IRTE
>> 
>> >>> On 21.09.16 at 04:37, <feng.wu@xxxxxxxxx> wrote:
>> > We don't set the affinity for posted format IRTE, since the
>> > destination of these interrupts is vCPU and the vCPU affinity
>> > is set during vCPU scheduling.
>> 
>> I'm quite sure I did point out before that you talk about just affinity
>> changes here, yet ...
>> 
>> > --- a/xen/drivers/passthrough/vtd/intremap.c
>> > +++ b/xen/drivers/passthrough/vtd/intremap.c
>> > @@ -637,9 +637,12 @@ static int msi_msg_to_remap_entry(
>> >      remap_rte->address_hi = 0;
>> >      remap_rte->data = index - i;
>> >
>> > -    memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
>> > -    iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
>> > -    iommu_flush_iec_index(iommu, 0, index);
>> > +    if ( !iremap_entry->remap.p || !iremap_entry->remap.im )
>> > +    {
>> > +        memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
>> > +        iommu_flush_cache_entry(iremap_entry, sizeof(struct 
> iremap_entry));
>> > +        iommu_flush_iec_index(iommu, 0, index);
>> > +    }
>> 
>> ... you suppress the update also in other cases. This _may_ be safe
>> at present, but you're digging a hole for someone else to fall into
>> down the road. Hence at the very least you should, in a to be added
>> "else" path, ASSERT() that nothing in the descriptor changed except
>> the bits representing affinity. Even better would be if in fact you
>> suppressed the update+flush only when nothing other than dst
>> changed.
> 
> I am a little confused about the above comments, Posted IRTE and
> Remapped IRTE has different format, and when the IRTE is in posted
> format, typically, the updated information (delivery mode, dest mode,
> vector, dest, etc) has no meaning for posted IRTE. However, there are
> indeed some shared part between the two format as below:
> - p
> - fpd
> - im
> - sid
> - sq
> - svt
> 
> Bits like sid, sq, and svt are not changed in this function,

How do you know? Judging just from current callers is - as said
before - calling for trouble down the road. And the function clearly
creates a brand new IRTE, which fully replaces the previous one.

> I am not sure
> this is what you mean by " you suppress the update also in other cases.",
> and I am not quite clear about what is your requirement, it would be
> appreciated if you can describe a bit more! Thanks!

My requirement is for the function to not get changed in ways
making (hidden) assumptions about its callers. If you want to
suppress the actual update, you should conditionalize this in a
way which can be proven correct by looking at just this one
function. That'll presumably entail comparing fields of the
currently in place and the new IRTE.

Jan


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