|
[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
>>> On 18.11.16 at 02:57, <feng.wu@xxxxxxxxx> wrote:
> @@ -597,31 +598,50 @@ static int msi_msg_to_remap_entry(
Considering you basically re-do most of the function, I think there's
some more adjustment necessary (or at least very desirable) here.
>
> 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;
These two "& 0x1" seem unnecessary, as the fields are 1 bit only
anyway.
> + new_ire.remap.dlm = (msg->data >> MSI_DATA_DELIVERY_MODE_SHIFT) &
> 0x1;
This masking, however, has always been puzzling me: dlm is a 3 bit
field, and the MSI message field is a 3-bit one too. Hence the
masking should also be dropped here - if the message field is wrong
(i.e. holding an unsupported value), there's no good reason to try
to compensate for it here. If at all the function should refuse to do
the requested translation.
> + /* 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 */
I don't understand this comment. The order of operations does not
matter at all, and in fact you now set p before being done with all
other fields. Please drop such misleading comments, or make them
useful/correct.
Also, the only field you don't explicitly set here is .im. Why?
> + }
> 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 */
> + }
Along the lines of the comment above, you don't fill avail here. Why?
Taking both together, I don't see why - after adding said initialization -
new_ire needs to start out as a copy of the live IRTE. Instead you
could memset() the whole structure to zero, or simply give it an empty
initializer (saving you from initializing all the reserved fields, plus some
more).
And of course, along the lines of ...
> if ( pdev )
> set_msi_source_id(pdev, &new_ire);
> else
> set_hpet_source_id(msi_desc->hpet_id, &new_ire);
... this, I see little reason for common fields to be initialized separately
on each path. According to the code above that's only p (leaving
aside fields which get zeroed), but anyway. Perhaps there should
even be a common sub-structure of the union ...
> @@ -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);
> + }
Could you remind me again please why posted format updates need
to use cmpxchg16b, but remapped format ones don't? (As a aside,
with the code structure as you have it you should move the old_irte
declaration here, or omit it altogether, as you could as well pass
*iremap_entry directly afaict.)
> @@ -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)
Is this unconditional passing of NULL here really correct?
> @@ -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);
There are few changes here which appear to have the potential of
affecting behavior: Previously you didn't alter msi_desc or the MSI
message contained therein (as documented by the pointer having
been const). Is this possible updating of message and remap index
really benign? In any event any such changes should be reasoned
about in the commit message.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |