| Message ID | 1472615791-8664-6-git-send-email-feng.wu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 31.08.16 at 05:56, <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. So is this based on the assumption that after initial setup the function would only ever get called for affinity changes? I'm not even sure this is the case today, but I don't think we should build in such a dependency. Assuming that the main motivation is to avoid ... > @@ -637,9 +640,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); > + } ... the actual updating here, may I suggest that you keep the construction of new_ire and modify the if() here to check whether nothing except affinity related bits changed? That would also take care of certain (older) compiler versions likely warning about new_ire potentially being used uninitialized. Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Thursday, September 1, 2016 4:39 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 v3 5/6] VT-d: No need to set irq affinity for posted format > IRTE > > >>> On 31.08.16 at 05:56, <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. > > So is this based on the assumption that after initial setup the function > would only ever get called for affinity changes? I'm not even sure > this is the case today, but I don't think we should build in such a > dependency. I don't think we have such dependency, as you mentioned below, the purpose of this patch if to void changing the affinity related bit when the IRTE is in posted mode. > > Assuming that the main motivation is to avoid ... > > > @@ -637,9 +640,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); > > + } > > ... the actual updating here, may I suggest that you keep the > construction of new_ire and modify the if() here to check whether > nothing except affinity related bits changed? That would also take > care of certain (older) compiler versions likely warning about new_ire > potentially being used uninitialized. Sure, maybe I can keep the construction of new_ire and only add this if() part, since if we find the IRTE is in posted mode ('IM' is set), we don't need to copy new_ire to iremap_entry. Thanks, Feng > > Jan
diff --git a/xen/drivers/passthrough/vtd/intremap.c b/xen/drivers/passthrough/vtd/intremap.c index bfd468b..2083ee2 100644 --- a/xen/drivers/passthrough/vtd/intremap.c +++ b/xen/drivers/passthrough/vtd/intremap.c @@ -595,33 +595,36 @@ static int msi_msg_to_remap_entry( GET_IREMAP_ENTRY(ir_ctrl->iremap_maddr, index, iremap_entries, iremap_entry); - memcpy(&new_ire, iremap_entry, sizeof(struct iremap_entry)); + if ( !iremap_entry->remap.p || !iremap_entry->remap.im ) + { + memcpy(&new_ire, iremap_entry, sizeof(struct iremap_entry)); - /* Set interrupt remapping table entry */ - new_ire.remap.fpd = 0; - new_ire.remap.dm = (msg->address_lo >> MSI_ADDR_DESTMODE_SHIFT) & 0x1; - new_ire.remap.tm = (msg->data >> MSI_DATA_TRIGGER_SHIFT) & 0x1; - new_ire.remap.dlm = (msg->data >> MSI_DATA_DELIVERY_MODE_SHIFT) & 0x1; - /* Hardware require RH = 1 for LPR delivery mode */ - new_ire.remap.rh = (new_ire.remap.dlm == dest_LowestPrio); - new_ire.remap.avail = 0; - new_ire.remap.res_1 = 0; - new_ire.remap.vector = (msg->data >> MSI_DATA_VECTOR_SHIFT) & - MSI_DATA_VECTOR_MASK; - new_ire.remap.res_2 = 0; - if ( x2apic_enabled ) - new_ire.remap.dst = msg->dest32; - else - new_ire.remap.dst = ((msg->address_lo >> MSI_ADDR_DEST_ID_SHIFT) - & 0xff) << 8; + /* Set interrupt remapping table entry */ + new_ire.remap.fpd = 0; + new_ire.remap.dm = (msg->address_lo >> MSI_ADDR_DESTMODE_SHIFT) & 0x1; + new_ire.remap.tm = (msg->data >> MSI_DATA_TRIGGER_SHIFT) & 0x1; + new_ire.remap.dlm = (msg->data >> MSI_DATA_DELIVERY_MODE_SHIFT) & 0x1; + /* Hardware require RH = 1 for LPR delivery mode */ + new_ire.remap.rh = (new_ire.remap.dlm == dest_LowestPrio); + new_ire.remap.avail = 0; + new_ire.remap.res_1 = 0; + new_ire.remap.vector = (msg->data >> MSI_DATA_VECTOR_SHIFT) & + MSI_DATA_VECTOR_MASK; + new_ire.remap.res_2 = 0; + if ( x2apic_enabled ) + new_ire.remap.dst = msg->dest32; + else + new_ire.remap.dst = ((msg->address_lo >> MSI_ADDR_DEST_ID_SHIFT) + & 0xff) << 8; - if ( pdev ) - set_msi_source_id(pdev, &new_ire); - else - set_hpet_source_id(msi_desc->hpet_id, &new_ire); - new_ire.remap.res_3 = 0; - new_ire.remap.res_4 = 0; - new_ire.remap.p = 1; /* finally, set present bit */ + if ( pdev ) + set_msi_source_id(pdev, &new_ire); + else + set_hpet_source_id(msi_desc->hpet_id, &new_ire); + new_ire.remap.res_3 = 0; + new_ire.remap.res_4 = 0; + new_ire.remap.p = 1; /* finally, set present bit */ + } /* now construct new MSI/MSI-X rte entry */ remap_rte = (struct msi_msg_remap_entry *)msg; @@ -637,9 +640,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> --- xen/drivers/passthrough/vtd/intremap.c | 62 +++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 28 deletions(-)