From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [RFC v1 07/15] vt-d: Add API to update IRTE when VT-d PI is used Date: Thu, 26 Mar 2015 19:17:26 +0000 Message-ID: <55145B46.2020907@citrix.com> References: <1427286717-4093-1-git-send-email-feng.wu@intel.com> <1427286717-4093-8-git-send-email-feng.wu@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1427286717-4093-8-git-send-email-feng.wu@intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Feng Wu , xen-devel@lists.xen.org Cc: yang.z.zhang@intel.com, kevin.tian@intel.com, keir@xen.org, JBeulich@suse.com List-Id: xen-devel@lists.xenproject.org On 25/03/15 12:31, Feng Wu wrote: > This patch adds an API which is used to update the IRTE > for posted-interrupt when guest changes MSI/MSI-X information. > > Signed-off-by: Feng Wu > --- > xen/drivers/passthrough/vtd/intremap.c | 83 ++++++++++++++++++++++++++++++++++ > xen/drivers/passthrough/vtd/iommu.h | 3 ++ > xen/include/asm-x86/iommu.h | 2 + > 3 files changed, 88 insertions(+) > > diff --git a/xen/drivers/passthrough/vtd/intremap.c b/xen/drivers/passthrough/vtd/intremap.c > index 0333686..f44e74d 100644 > --- a/xen/drivers/passthrough/vtd/intremap.c > +++ b/xen/drivers/passthrough/vtd/intremap.c > @@ -898,3 +898,86 @@ void iommu_disable_x2apic_IR(void) > for_each_drhd_unit ( drhd ) > disable_qinval(drhd->iommu); > } > + > +/* > + * This function is used to update the IRTE for posted-interrupt > + * when guest changes MSI/MSI-X information > + */ > +int pi_update_irte(struct vcpu *v, struct pirq *pirq, uint32_t gvec ) Stray space after gvec. I presume gvec means "guest vector", in which case it should be a uint8_t. > +{ > + struct irq_desc *desc; > + struct msi_desc *msi_desc; > + int remap_index, rc = -1; > + struct pci_dev *pci_dev; > + struct acpi_drhd_unit *drhd; > + struct iommu *iommu; > + struct ir_ctrl *ir_ctrl; > + struct iremap_entry *iremap_entries = NULL, *p = NULL; > + struct iremap_entry new_ire; > + struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc; > + unsigned long flags; > + > + desc = pirq_spin_lock_irq_desc(pirq, NULL); > + if ( !desc ) > + return -1; > + > + msi_desc = desc->msi_desc; > + if ( !msi_desc ) > + goto unlock_out; > + > + remap_index = msi_desc->remap_index; > + pci_dev = msi_desc->dev; > + if ( !pci_dev ) > + goto unlock_out; > + > + drhd = acpi_find_matched_drhd_unit(pci_dev); This does an O(n^2) walk over the dhrd_units list and the unit device_cnt. As the layout will be completely static, might it be better to stash drhd information in struct pci_dev during boot? > + if (!drhd) Spaces inside brackets. > + { > + dprintk(XENLOG_INFO VTDPREFIX, "failed to get drhd!\n"); This is a useless error message, providing no useful information to identify the issue. At the very least it should identify the domain and vcpu (%pv of v), the guest vector and pci device. Also, drop the exclamation mark - it is not useful in the log. > + goto unlock_out; > + } > + > + iommu = drhd->iommu; > + ir_ctrl = iommu_ir_ctrl(iommu); > + if ( !ir_ctrl ) > + { > + dprintk(XENLOG_INFO VTDPREFIX, "failed to get ir_ctrl!\n"); Needs simiarly extending with some information. > + goto unlock_out; > + } > + > + spin_lock_irqsave(&ir_ctrl->iremap_lock, flags); > + > + GET_IREMAP_ENTRY(ir_ctrl->iremap_maddr, remap_index, iremap_entries, p); > + > + memcpy(&new_ire, p, sizeof(struct iremap_entry)); sizeof(new_ire) please > + > + /* Setup/Update interrupt remapping table entry */ > + new_ire.lo_intpost.urg = 0; > + new_ire.lo_intpost.vector = gvec; > + new_ire.lo_intpost.pda_l = (((u64)virt_to_maddr(pi_desc)) >> > + (32 - PDA_LOW_BIT)) & ~(-1UL << PDA_LOW_BIT); > + new_ire.hi_intpost.pda_h = (((u64)virt_to_maddr(pi_desc)) >> 32) & > + ~(-1UL << PDA_HIGH_BIT); Is it possible to hide this bit manipulation in side a static inline function which takes an ire and pi_desc pointer rather than open coding it? > + > + new_ire.lo_intpost.res_1 = 0; > + new_ire.lo_intpost.res_2 = 0; > + new_ire.lo_intpost.res_3 = 0; > + new_ire.hi_intpost.res_1 = 0; > + > + new_ire.lo_intpost.im = 1; > + > + memcpy(p, &new_ire, sizeof(struct iremap_entry)); > + iommu_flush_cache_entry(p, sizeof(struct iremap_entry)); Same here regarding sizeof. Furthermore, is the memcpy() safe to update the live descriptor? If it is, why do you not update the descriptor in place using p-> rather than copying it to the stack and back? ~Andrew > + iommu_flush_iec_index(iommu, 0, remap_index); > + > + if ( iremap_entries ) > + unmap_vtd_domain_page(iremap_entries); > + > + spin_unlock_irqrestore(&ir_ctrl->iremap_lock, flags); > + > + rc = 0; > + unlock_out: > + spin_unlock_irq(&desc->lock); > + > + return rc; > +} > diff --git a/xen/drivers/passthrough/vtd/iommu.h b/xen/drivers/passthrough/vtd/iommu.h > index cd61e12..ffa72c8 100644 > --- a/xen/drivers/passthrough/vtd/iommu.h > +++ b/xen/drivers/passthrough/vtd/iommu.h > @@ -334,6 +334,9 @@ struct iremap_entry { > }; > }; > > +#define PDA_LOW_BIT 26 > +#define PDA_HIGH_BIT 32 > + > /* Max intr remapping table page order is 8, as max number of IRTEs is 64K */ > #define IREMAP_PAGE_ORDER 8 > > diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h > index e7a65da..d233621 100644 > --- a/xen/include/asm-x86/iommu.h > +++ b/xen/include/asm-x86/iommu.h > @@ -32,6 +32,8 @@ int iommu_supports_eim(void); > int iommu_enable_x2apic_IR(void); > void iommu_disable_x2apic_IR(void); > > +int pi_update_irte(struct vcpu *v, struct pirq *pirq, uint32_t gvec); > + > #endif /* !__ARCH_X86_IOMMU_H__ */ > /* > * Local variables: