> -----Original Message----- > From: Andrew Cooper [mailto:andrew.coop...@citrix.com] > Sent: Friday, March 27, 2015 3:17 AM > To: Wu, Feng; xen-devel@lists.xen.org > Cc: Zhang, Yang Z; Tian, Kevin; k...@xen.org; jbeul...@suse.com > Subject: Re: [Xen-devel] [RFC v1 07/15] vt-d: Add API to update IRTE when VT-d > PI is used > > 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 <feng...@intel.com> > > --- > > 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?
This might be a good suggestion. Since acpi_find_matched_drhd_unit() is widely used in the current code for this purpose, I think we can define this as an independent work if this is really doable. Thanks, Feng > > > + 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: _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel