| Message ID | 1474425470-3629-6-git-send-email-feng.wu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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. 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
> -----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
>>> 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. > 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
> -----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
>>> On 09.10.16 at 07:35, <feng.wu@intel.com> wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Wednesday, September 28, 2016 5:59 PM >> >>> On 28.09.16 at 08:51, <feng.wu@intel.com> wrote: >> >> From: Jan Beulich [mailto:JBeulich@suse.com] >> >> Sent: Monday, September 26, 2016 8:58 PM >> >> >>> 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? 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
> >> 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? Oh, yes, it does! Sorry for the negligence. Thanks, Feng
diff --git a/xen/drivers/passthrough/vtd/intremap.c b/xen/drivers/passthrough/vtd/intremap.c index bfd468b..4537714 100644 --- 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); + } unmap_vtd_domain_page(iremap_entries); spin_unlock_irqrestore(&ir_ctrl->iremap_lock, flags);
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. Signed-off-by: Feng Wu <feng.wu@intel.com> --- v4: - Keep the construction of new_ire and only modify the hardware IRTE when it is not in posted mode. xen/drivers/passthrough/vtd/intremap.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)