| Message ID | 1477622259-3476-6-git-send-email-feng.wu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 28.10.16 at 04:37, <feng.wu@intel.com> wrote: > v6: > - Make pi_can_suppress_irte_update() a check-only function > - Introduce another function pi_get_new_irte() to update the 'new_ire' if > needed I don't see what you need this function for. My earlier comments were not meant to make you split the function, but to drop all this secondary modification (unless there's actually a reason for this, which so far I haven't seen any proof of). Apart from that there already is setup_posted_irte(), which seems to do most if not all of what you really need. In any event, the sequence of operations should be 1) create full new entry 2) check whether update is needed (i.e. whether old and new entries have meaningful differences) 3) do update 4) flush. > --- a/xen/drivers/passthrough/vtd/intremap.c > +++ b/xen/drivers/passthrough/vtd/intremap.c > @@ -547,6 +547,54 @@ static int remap_entry_to_msi_msg( > return 0; > } > > +static bool pi_can_suppress_irte_update(const struct iremap_entry *new, > + const struct iremap_entry *old) The two parameter declarations should be aligned with one another. > +{ > + ASSERT( old && new ); > + > + if ( !old->remap.p || !old->remap.im || !new->remap.p || new->remap.im ) > + return false; The asymmetry regarding the IM bit is confusing, and imo indicates a problem with the logic. > + /* > + * We are updating posted IRTE to remapped one, check whether > + * the common fields are going to be changed. > + */ > + if ( ( new->remap.fpd != old->post.fpd ) || > + ( new->remap.sid != old->post.sid ) || > + ( new->remap.sq != old->post.sq ) || > + ( new->remap.svt != old->post.svt ) ) Stray blanks inside the inner parentheses. Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Monday, October 31, 2016 10:57 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 v6 5/7] VT-d: No need to set irq affinity for posted format > IRTE > > >>> On 28.10.16 at 04:37, <feng.wu@intel.com> wrote: > > v6: > > - Make pi_can_suppress_irte_update() a check-only function > > - Introduce another function pi_get_new_irte() to update the 'new_ire' if > > needed > > I don't see what you need this function for. My earlier comments > were not meant to make you split the function, but to drop all this > secondary modification (unless there's actually a reason for this, > which so far I haven't seen any proof of). Apart from that there > already is setup_posted_irte(), which seems to do most if not all > of what you really need. > > In any event, the sequence of operations should be > 1) create full new entry > 2) check whether update is needed (i.e. whether old and new entries > have meaningful differences) > 3) do update > 4) flush. Your early comment suggest we only suppress the affinity related field. So the modification here is to handle the following case: When the IRTE is currently in posted mode and this function tries to update it he remapped mode, then we need to suppress the affinity related bit, however, if the common fields (fpd, sid sq, svt) in the new remapped IRTE is different from the value in the old posted IRTE, what do you suggest we should do? Here I copy these fields to the new IRTE and update them accordingly. > > > --- a/xen/drivers/passthrough/vtd/intremap.c > > +++ b/xen/drivers/passthrough/vtd/intremap.c > > @@ -547,6 +547,54 @@ static int remap_entry_to_msi_msg( > > return 0; > > } > > > > +static bool pi_can_suppress_irte_update(const struct iremap_entry *new, > > + const struct iremap_entry *old) > > The two parameter declarations should be aligned with one another. > > > +{ > > + ASSERT( old && new ); > > + > > + if ( !old->remap.p || !old->remap.im || !new->remap.p || new->remap.im ) > > + return false; > > The asymmetry regarding the IM bit is confusing, and imo indicates > a problem with the logic. Would you please share what the problem is here? If we get here, that means we are updating a posted IRTE to remapped one. Thanks, Feng > > > + /* > > + * We are updating posted IRTE to remapped one, check whether > > + * the common fields are going to be changed. > > + */ > > + if ( ( new->remap.fpd != old->post.fpd ) || > > + ( new->remap.sid != old->post.sid ) || > > + ( new->remap.sq != old->post.sq ) || > > + ( new->remap.svt != old->post.svt ) ) > > Stray blanks inside the inner parentheses. > > Jan
>>> On 03.11.16 at 08:46, <feng.wu@intel.com> wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Monday, October 31, 2016 10:57 PM >> >>> On 28.10.16 at 04:37, <feng.wu@intel.com> wrote: >> > v6: >> > - Make pi_can_suppress_irte_update() a check-only function >> > - Introduce another function pi_get_new_irte() to update the 'new_ire' if >> > needed >> >> I don't see what you need this function for. My earlier comments >> were not meant to make you split the function, but to drop all this >> secondary modification (unless there's actually a reason for this, >> which so far I haven't seen any proof of). Apart from that there >> already is setup_posted_irte(), which seems to do most if not all >> of what you really need. >> >> In any event, the sequence of operations should be >> 1) create full new entry >> 2) check whether update is needed (i.e. whether old and new entries >> have meaningful differences) >> 3) do update >> 4) flush. > > Your early comment suggest we only suppress the affinity related > field. So the modification here is to handle the following case: > > When the IRTE is currently in posted mode and this function tries > to update it he remapped mode, then we need to suppress > the affinity related bit, however, if the common fields (fpd, sid > sq, svt) in the new remapped IRTE is different from the value > in the old posted IRTE, what do you suggest we should do? > Here I copy these fields to the new IRTE and update them > accordingly. You continue to look at this from what I think is the wrong angle: msi_msg_to_remap_entry() should not make assumptions or special case either of the formats. It should construct the new entry (whichever format that's supposed to be) and compare all relevant (for the given format) fields with those of the old entry. If they all match, skip the flush. >> > --- a/xen/drivers/passthrough/vtd/intremap.c >> > +++ b/xen/drivers/passthrough/vtd/intremap.c >> > @@ -547,6 +547,54 @@ static int remap_entry_to_msi_msg( >> > return 0; >> > } >> > >> > +static bool pi_can_suppress_irte_update(const struct iremap_entry *new, >> > + const struct iremap_entry *old) >> >> The two parameter declarations should be aligned with one another. >> >> > +{ >> > + ASSERT( old && new ); Btw, stray blanks here (in case I didn't point this out before). >> > + if ( !old->remap.p || !old->remap.im || !new->remap.p || new->remap.im ) >> > + return false; >> >> The asymmetry regarding the IM bit is confusing, and imo indicates >> a problem with the logic. > > Would you please share what the problem is here? If we get here, that means > we are updating a posted IRTE to remapped one. It's the assumption expressed by the second sentence of your reply which is the problem: You're making an assumption here which you shouldn't make - I don't see the caller guaranteeing that assumption, and even if this one caller did, a function like the one here should be generic enough so that future callers won't need to worry about preconditions. Jan
diff --git a/xen/drivers/passthrough/vtd/intremap.c b/xen/drivers/passthrough/vtd/intremap.c index bfd468b..97e80a6 100644 --- a/xen/drivers/passthrough/vtd/intremap.c +++ b/xen/drivers/passthrough/vtd/intremap.c @@ -547,6 +547,54 @@ static int remap_entry_to_msi_msg( return 0; } +static bool pi_can_suppress_irte_update(const struct iremap_entry *new, + const struct iremap_entry *old) +{ + ASSERT( old && new ); + + if ( !old->remap.p || !old->remap.im || !new->remap.p || new->remap.im ) + return false; + + /* + * We are updating posted IRTE to remapped one, check whether + * the common fields are going to be changed. + */ + if ( ( new->remap.fpd != old->post.fpd ) || + ( new->remap.sid != old->post.sid ) || + ( new->remap.sq != old->post.sq ) || + ( new->remap.svt != old->post.svt ) ) + return false; + + return true; +} + +static void pi_get_new_irte(struct iremap_entry *new, + const struct iremap_entry *old) +{ + u16 fpd, sid, sq, svt; + + ASSERT( old && new ); + + fpd = new->remap.fpd; + sid = new->remap.sid; + sq = new->remap.sq; + svt = new->remap.svt; + + *new = *old; + + if ( fpd != old->post.fpd ) + new->post.fpd = fpd; + + if ( sid != old->post.sid ) + new->post.sid = sid; + + if ( sq != old->post.sq ) + new->post.sq = sq; + + if ( svt != old->post.svt ) + new->post.svt = svt; +} + static int msi_msg_to_remap_entry( struct iommu *iommu, struct pci_dev *pdev, struct msi_desc *msi_desc, struct msi_msg *msg) @@ -637,9 +685,16 @@ 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 ( !pi_can_suppress_irte_update(&new_ire, iremap_entry) ) + { + if ( iremap_entry->remap.p && iremap_entry->remap.im && + new_ire.remap.p && !new_ire.remap.im ) + pi_get_new_irte(&new_ire, iremap_entry); + + 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> --- v6: - Make pi_can_suppress_irte_update() a check-only function - Introduce another function pi_get_new_irte() to update the 'new_ire' if needed v5: - Only suppress affinity related IRTE updates for PI 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 | 61 ++++++++++++++++++++++++++++++++-- 1 file changed, 58 insertions(+), 3 deletions(-)