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

From: "Wu, Feng" <feng.wu@intel.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: "Tian, Kevin" <kevin.tian@intel.com>,
	"Wu, Feng" <feng.wu@intel.com>,
	"george.dunlap@eu.citrix.com" <george.dunlap@eu.citrix.com>,
	"andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>,
	"dario.faggioli@citrix.com" <dario.faggioli@citrix.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
Subject: Re: [PATCH v4 5/6] VT-d: No need to set irq affinity for posted format IRTE
Date: Wed, 28 Sep 2016 06:51:51 +0000	[thread overview]
Message-ID: <E959C4978C3B6342920538CF579893F0198804C5@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <57E93782020000780011274C@prv-mh.provo.novell.com>

> -----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, 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!

Thanks,
Feng

> 
> Also, since you already touch this, please consider switching from the
> type-unsafe memcpy() to type-safe structure assignment. And please
> in any event change the sizeof()-s to sizeof(*iremap_entry).
> 
> Jan

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