|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v11 5/6] VT-d: introduce update_irte to update irte safely
> From: Gao, Chao
> Sent: Wednesday, March 29, 2017 1:12 PM
>
> We used structure assignment to update irte which was non-atomic when
> the
> whole IRTE was to be updated. It is unsafe when a interrupt happened
> during
> update. Furthermore, no bug or warning would be reported when this
> happened.
>
> This patch introduces two variants, atomic and non-atomic, to update
> irte. Both variants will update IRTE if possible. If the caller requests a
> atomic update but we can't meet it, we raise a bug.
>
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
> v11:
> - Add two variant function to update IRTE. Call the non-atomic one for init
> and clear operations. Call the atomic one for other cases.
> - Add a new field to indicate the remap_entry associated with msi_desc is
> initialized or not.
>
> v10:
> - rename copy_irte_to_irt to update_irte
> - remove copy_from_to_irt
> - change commmit message and add some comments to illustrate on which
> condition update_irte() is safe.
>
> xen/arch/x86/msi.c | 1 +
> xen/drivers/passthrough/vtd/intremap.c | 78
> ++++++++++++++++++++++++++++++++--
> xen/include/asm-x86/msi.h | 1 +
> 3 files changed, 76 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index 3374cd4..7ed1243 100644
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -578,6 +578,7 @@ static struct msi_desc *alloc_msi_entry(unsigned int
> nr)
> entry[nr].dev = NULL;
> entry[nr].irq = -1;
> entry[nr].remap_index = -1;
> + entry[nr].remap_entry_initialized = false;
> entry[nr].pi_desc = NULL;
> }
>
> diff --git a/xen/drivers/passthrough/vtd/intremap.c
> b/xen/drivers/passthrough/vtd/intremap.c
> index b992f23..b7f3cf1 100644
> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -169,10 +169,64 @@ bool_t __init iommu_supports_eim(void)
> return 1;
> }
>
> +static void update_irte(struct iremap_entry *entry,
> + const struct iremap_entry *new_ire,
> + bool atomic)
> +{
> + if ( cpu_has_cx16 )
> + {
> + __uint128_t ret;
> + struct iremap_entry old_ire;
> +
> + old_ire = *entry;
> + ret = cmpxchg16b(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);
> + }
> + else
> + {
> + /*
> + * The following code will update irte atomically if possible.
> + * If the caller requests a atomic update but we can't meet it,
a -> an
> + * a bug will be raised.
> + */
> + if ( entry->lo == new_ire->lo )
> + entry->hi = new_ire->hi;
> + else if ( entry->hi == new_ire->hi )
> + entry->lo = new_ire->lo;
> + else if ( !atomic )
> + {
> + entry->lo = new_ire->lo;
> + entry->hi = new_ire->hi;
> + }
> + else
> + BUG();
suppose you need same ASSERT as for cmxchg16 here in atomic
case.
> + }
> +}
> +
> +static inline void update_irte_non_atomic(struct iremap_entry *entry,
> + const struct iremap_entry *new_ire)
> +{
> + update_irte(entry, new_ire, false);
> +}
> +
> +static inline void update_irte_atomic(struct iremap_entry *entry,
> + const struct iremap_entry *new_ire)
> +{
> + update_irte(entry, new_ire, true);
> +}
> +
> +
> /* Mark specified intr remap entry as free */
> static void free_remap_entry(struct iommu *iommu, int index)
> {
> - struct iremap_entry *iremap_entry = NULL, *iremap_entries;
> + struct iremap_entry *iremap_entry = NULL, *iremap_entries, new_ire = { };
> struct ir_ctrl *ir_ctrl = iommu_ir_ctrl(iommu);
>
> if ( index < 0 || index > IREMAP_ENTRY_NR - 1 )
> @@ -183,7 +237,7 @@ static void free_remap_entry(struct iommu *iommu,
> int index)
> GET_IREMAP_ENTRY(ir_ctrl->iremap_maddr, index,
> iremap_entries, iremap_entry);
>
> - memset(iremap_entry, 0, sizeof(*iremap_entry));
> + update_irte_non_atomic(iremap_entry, &new_ire);
> iommu_flush_cache_entry(iremap_entry, sizeof(*iremap_entry));
> iommu_flush_iec_index(iommu, 0, index);
>
> @@ -286,6 +340,7 @@ static int ioapic_rte_to_remap_entry(struct iommu
> *iommu,
> int index;
> unsigned long flags;
> struct ir_ctrl *ir_ctrl = iommu_ir_ctrl(iommu);
> + bool init = false;
>
> remap_rte = (struct IO_APIC_route_remap_entry *) old_rte;
> spin_lock_irqsave(&ir_ctrl->iremap_lock, flags);
> @@ -296,6 +351,7 @@ static int ioapic_rte_to_remap_entry(struct iommu
> *iommu,
> index = alloc_remap_entry(iommu, 1);
> if ( index < IREMAP_ENTRY_NR )
> apic_pin_2_ir_idx[apic][ioapic_pin] = index;
> + init = true;
> }
>
> if ( index > IREMAP_ENTRY_NR - 1 )
> @@ -353,7 +409,11 @@ static int ioapic_rte_to_remap_entry(struct iommu
> *iommu,
> remap_rte->format = 1; /* indicate remap format */
> }
>
> - *iremap_entry = new_ire;
> + if ( init )
> + update_irte_non_atomic(iremap_entry, &new_ire);
> + else
> + update_irte_atomic(iremap_entry, &new_ire);
> +
> iommu_flush_cache_entry(iremap_entry, sizeof(*iremap_entry));
> iommu_flush_iec_index(iommu, 0, index);
>
> @@ -567,7 +627,10 @@ static int msi_msg_to_remap_entry(
> {
> /* Free specified unused IRTEs */
> for ( i = 0; i < nr; ++i )
> + {
> free_remap_entry(iommu, msi_desc->remap_index + i);
> + msi_desc[i].remap_entry_initialized = false;
> + }
> spin_unlock_irqrestore(&ir_ctrl->iremap_lock, flags);
> return 0;
> }
> @@ -639,7 +702,14 @@ static int msi_msg_to_remap_entry(
> remap_rte->address_hi = 0;
> remap_rte->data = index - i;
>
> - *iremap_entry = new_ire;
> + if ( msi_desc->remap_entry_initialized )
> + update_irte_atomic(iremap_entry, &new_ire);
> + else
> + {
> + update_irte_non_atomic(iremap_entry, &new_ire);
> + msi_desc->remap_entry_initialized = true;
> + }
> +
> iommu_flush_cache_entry(iremap_entry, sizeof(*iremap_entry));
> iommu_flush_iec_index(iommu, 0, index);
>
> diff --git a/xen/include/asm-x86/msi.h b/xen/include/asm-x86/msi.h
> index fc9ab04..a0bd3af 100644
> --- a/xen/include/asm-x86/msi.h
> +++ b/xen/include/asm-x86/msi.h
> @@ -118,6 +118,7 @@ struct msi_desc {
> struct msi_msg msg; /* Last set MSI message */
>
> int remap_index; /* index in interrupt remapping table
> */
> + bool remap_entry_initialized;
> const struct pi_desc *pi_desc; /* pointer to posted descriptor */
> uint8_t gvec; /* guest vector. valid when pi_desc
> isn't NULL */
> };
> --
> 1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |