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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.10.16 at 07:35, <feng.wu@xxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: Wednesday, September 28, 2016 5:59 PM
>> >>> On 28.09.16 at 08:51, <feng.wu@xxxxxxxxx> wrote:
>> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> >> Sent: Monday, September 26, 2016 8:58 PM
>> >> >>> 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.
> 
> This function copy the old IRTE to 'new_ire' and it doesn't touch
> fields like 'sid', 'sq', and 'svt', so how could these bits get changed?

The function calls set_msi_source_id(), doesn't it? In fact it looks
like the initial memcpy() is (almost) pointless, and might better be
replaced by explicit copying of existing fields (if any). And the
"if any" is quite relevant here, considering that the function gets
called also for installing brand new IRTEs, which then obviously
can't have any fields to retain.

Jan


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