|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v8 4/7] VT-d: Use one function to update both remapped and posted IRTE
> From: Wu, Feng
> Sent: Friday, November 18, 2016 9:57 AM
>
> Use one function to update both remapped IRTE and posted IRET.
>
> Signed-off-by: Feng Wu <feng.wu@xxxxxxxxx>
> ---
> v8:
> - Newly added
>
> xen/drivers/passthrough/vtd/intremap.c | 162
> ++++++++++++++-------------------
> 1 file changed, 66 insertions(+), 96 deletions(-)
>
> diff --git a/xen/drivers/passthrough/vtd/intremap.c
> b/xen/drivers/passthrough/vtd/intremap.c
> index bfd468b..fd2a49a 100644
> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -420,7 +420,7 @@ void io_apic_write_remap_rte(
> __ioapic_write_entry(apic, ioapic_pin, 1, old_rte);
> }
>
> -static void set_msi_source_id(struct pci_dev *pdev, struct iremap_entry *ire)
> +static void set_msi_source_id(const struct pci_dev *pdev, struct
> iremap_entry *ire)
> {
> u16 seg;
> u8 bus, devfn, secbus;
> @@ -548,11 +548,12 @@ static int remap_entry_to_msi_msg(
> }
>
> static int msi_msg_to_remap_entry(
> - struct iommu *iommu, struct pci_dev *pdev,
> - struct msi_desc *msi_desc, struct msi_msg *msg)
> + struct iommu *iommu, const struct pci_dev *pdev,
> + struct msi_desc *msi_desc, struct msi_msg *msg,
> + const struct pi_desc *pi_desc, const uint8_t gvec)
> {
> struct iremap_entry *iremap_entry = NULL, *iremap_entries;
> - struct iremap_entry new_ire;
> + struct iremap_entry new_ire, old_ire;
old_ire is used only in later 'else' branch. move definition there.
> struct msi_msg_remap_entry *remap_rte;
> unsigned int index, i, nr = 1;
> unsigned long flags;
> @@ -597,31 +598,50 @@ static int msi_msg_to_remap_entry(
>
> 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;
> + if ( !pi_desc )
> + {
> + /* 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;
> +
> + new_ire.remap.res_3 = 0;
> + new_ire.remap.res_4 = 0;
> + new_ire.remap.p = 1; /* finally, set present bit */
> + }
> else
> - new_ire.remap.dst = ((msg->address_lo >> MSI_ADDR_DEST_ID_SHIFT)
> - & 0xff) << 8;
> + {
> + new_ire.post.fpd = 0;
> + new_ire.post.res_1 = 0;
> + new_ire.post.res_2 = 0;
> + new_ire.post.urg = 0;
> + new_ire.post.im = 1;
> + new_ire.post.vector = gvec;
> + new_ire.post.res_3 = 0;
> + new_ire.post.res_4 = 0;
> + new_ire.post.res_5 = 0;
> + new_ire.post.pda_l = virt_to_maddr(pi_desc) >> (32 - PDA_LOW_BIT);
> + new_ire.post.pda_h = virt_to_maddr(pi_desc) >> 32;
> + new_ire.post.p = 1; /* finally, set present bit */
> + }
It's a little confusing here:
- you first copy current remapping entry to new_ire and then do some
initialization.
Is it necessary? If there are some fields we'd like to inherit, better to
describe them
in comment so others can have a clear understanding of what exactly fields must
be updated here;
- in original code of setup_posted_irte, you actually do conditional assignment
based on format of old_ire. Above you changed to non-conditional assignment,
not relying on old_ire. Is it desired?
>
> 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,7 +657,23 @@ 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));
> + if ( !pi_desc )
> + memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry));
> + else
> + {
> + __uint128_t ret;
> +
> + old_ire = *iremap_entry;
> + ret = cmpxchg16b(iremap_entry, &old_ire, &new_ire);
> +
> + /*
> + * In the above, we use cmpxchg16 to atomically update the 128-bit
> IRTE,
> + * and the hardware cannot update the IRTE behind us, so the return
> value
> + * of cmpxchg16 should be the same as old_ire. This ASSERT validate
> it.
> + */
> + ASSERT(ret == old_ire.val);
> + }
> +
> iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry));
> iommu_flush_iec_index(iommu, 0, index);
>
> @@ -668,7 +704,8 @@ int msi_msg_write_remap_rte(
>
> drhd = pdev ? acpi_find_matched_drhd_unit(pdev)
> : hpet_to_drhd(msi_desc->hpet_id);
> - return drhd ? msi_msg_to_remap_entry(drhd->iommu, pdev, msi_desc, msg)
> + return drhd ? msi_msg_to_remap_entry(drhd->iommu, pdev,
> + msi_desc, msg, NULL, 0)
> : -EINVAL;
> }
>
> @@ -902,42 +939,6 @@ void iommu_disable_x2apic_IR(void)
> disable_qinval(drhd->iommu);
> }
>
> -static void setup_posted_irte(
> - struct iremap_entry *new_ire, const struct iremap_entry *old_ire,
> - const struct pi_desc *pi_desc, const uint8_t gvec)
> -{
> - memset(new_ire, 0, sizeof(*new_ire));
> -
> - /*
> - * 'im' filed decides whether the irte is in posted format (with value 1)
> - * or remapped format (with value 0), if the old irte is in remapped
> format,
> - * we copy things from remapped part in 'struct iremap_entry', otherwise,
> - * we copy from posted part.
> - */
> - if ( !old_ire->remap.im )
> - {
> - new_ire->post.p = old_ire->remap.p;
> - new_ire->post.fpd = old_ire->remap.fpd;
> - new_ire->post.sid = old_ire->remap.sid;
> - new_ire->post.sq = old_ire->remap.sq;
> - new_ire->post.svt = old_ire->remap.svt;
> - }
> - else
> - {
> - new_ire->post.p = old_ire->post.p;
> - new_ire->post.fpd = old_ire->post.fpd;
> - new_ire->post.sid = old_ire->post.sid;
> - new_ire->post.sq = old_ire->post.sq;
> - new_ire->post.svt = old_ire->post.svt;
> - new_ire->post.urg = old_ire->post.urg;
> - }
> -
> - new_ire->post.im = 1;
> - new_ire->post.vector = gvec;
> - new_ire->post.pda_l = virt_to_maddr(pi_desc) >> (32 - PDA_LOW_BIT);
> - new_ire->post.pda_h = virt_to_maddr(pi_desc) >> 32;
> -}
> -
> /*
> * This function is used to update the IRTE for posted-interrupt
> * when guest changes MSI/MSI-X information.
> @@ -946,17 +947,12 @@ int pi_update_irte(const struct vcpu *v, const struct
> pirq *pirq,
> const uint8_t gvec)
> {
> struct irq_desc *desc;
> - const struct msi_desc *msi_desc;
> - int remap_index;
> + struct msi_desc *msi_desc;
> int rc = 0;
> const struct pci_dev *pci_dev;
> const 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, old_ire;
> const struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
> - __uint128_t ret;
>
> desc = pirq_spin_lock_irq_desc(pirq, NULL);
> if ( !desc )
> @@ -976,8 +972,6 @@ int pi_update_irte(const struct vcpu *v, const struct
> pirq *pirq,
> goto unlock_out;
> }
>
> - remap_index = msi_desc->remap_index;
> -
> spin_unlock_irq(&desc->lock);
>
> ASSERT(pcidevs_locked());
> @@ -992,35 +986,11 @@ int pi_update_irte(const struct vcpu *v, const struct
> pirq *pirq,
> return -ENODEV;
>
> iommu = drhd->iommu;
> - ir_ctrl = iommu_ir_ctrl(iommu);
> - if ( !ir_ctrl )
> + if ( !iommu_ir_ctrl(iommu) )
> return -ENODEV;
>
> - spin_lock_irq(&ir_ctrl->iremap_lock);
> -
> - GET_IREMAP_ENTRY(ir_ctrl->iremap_maddr, remap_index, iremap_entries, p);
> -
> - old_ire = *p;
> -
> - /* Setup/Update interrupt remapping table entry. */
> - setup_posted_irte(&new_ire, &old_ire, pi_desc, gvec);
> - ret = cmpxchg16b(p, &old_ire, &new_ire);
> -
> - /*
> - * In the above, we use cmpxchg16 to atomically update the 128-bit IRTE,
> - * and the hardware cannot update the IRTE behind us, so the return value
> - * of cmpxchg16 should be the same as old_ire. This ASSERT validate it.
> - */
> - ASSERT(ret == old_ire.val);
> -
> - iommu_flush_cache_entry(p, sizeof(*p));
> - iommu_flush_iec_index(iommu, 0, remap_index);
> -
> - unmap_vtd_domain_page(iremap_entries);
> -
> - spin_unlock_irq(&ir_ctrl->iremap_lock);
> -
> - return 0;
> + return msi_msg_to_remap_entry(iommu, pci_dev, msi_desc, &msi_desc->msg,
> + pi_desc, gvec);
>
> unlock_out:
> spin_unlock_irq(&desc->lock);
> --
> 2.1.0
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |