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

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, September 28, 2016 5:59 PM
> To: Wu, Feng <feng.wu@intel.com>
> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com;
> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen-
> devel@lists.xen.org
> Subject: RE: [PATCH v4 5/6] VT-d: No need to set irq affinity for posted format
> IRTE
> 
> >>> On 28.09.16 at 08:51, <feng.wu@intel.com> wrote:
> 
> >
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Monday, September 26, 2016 8:58 PM
> >> To: Wu, Feng <feng.wu@intel.com>
> >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com;
> >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen-
> >> devel@lists.xen.org
> >> 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@intel.com> 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?

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

As mentioned above, I don't think I made such assumptions about its caller.
Even without my patch, this function only changes the affinity related fields,
such as, destination, delivery mode, destination mode, etc, which are not
needed for posted mode.

Thanks,
Feng

> 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@lists.xen.org
https://lists.xen.org/xen-devel