Re: [Xen-devel] [PATCH v9 8/8] VT-d: Add copy_irte_{to, from}_irt for updating irte

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Gao, Chao
> Sent: Monday, February 27, 2017 9:46 AM
> 
> We used structure assignment to update irte which was not safe when a

a -> an

> interrupt happend during update. It is better to update IRTE atomically

happend -> happened

> through cmpxchg16b(). When cmpxchg16b is not supported, two 64-bit write
> operation can atomically update IRTE when only one the high dword or low

remove 'one'

> dword is intented to be changed.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
> v9:
> - Newly added.
> 
>  xen/drivers/passthrough/vtd/intremap.c | 58 ++++++++++++++++++++--------
> ------
>  1 file changed, 34 insertions(+), 24 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/intremap.c
> b/xen/drivers/passthrough/vtd/intremap.c
> index 737b886..649bebd 100644
> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -169,6 +169,37 @@ bool_t __init iommu_supports_eim(void)
>      return 1;
>  }
> 
> +static inline void copy_irte_from_irt(struct iremap_entry *dst,
> +                                      struct iremap_entry *src) {

It's not immediately clear between 'irte' and 'irt'...

> +    *dst = *src;
> +}
> +
> +static void copy_irte_to_irt(struct iremap_entry *dst, struct
> +iremap_entry *src) {
> +    if ( cpu_has_cx16 )
> +    {
> +        __uint128_t ret;
> +        struct iremap_entry old_ire;
> +
> +        copy_irte_from_irt(&old_ire, dst);
> +        ret = cmpxchg16b(dst, &old_ire, src);
> +
> +        /*
> +         * 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
> +    {
> +        dst->lo = src->lo;
> +        dst->hi = src->hi;
> +    }
> +}
> +
>  /* Mark specified intr remap entry as free */  static void
> free_remap_entry(struct iommu *iommu, int index)  { @@ -310,7 +341,7
> @@ static int ioapic_rte_to_remap_entry(struct iommu *iommu,
>      GET_IREMAP_ENTRY(ir_ctrl->iremap_maddr, index,
>                       iremap_entries, iremap_entry);
> 
> -    new_ire = *iremap_entry;
> +    copy_irte_from_irt(&new_ire, iremap_entry);
> 
>      if ( rte_upper )
>      {
> @@ -353,7 +384,7 @@ static int ioapic_rte_to_remap_entry(struct iommu
> *iommu,
>          remap_rte->format = 1;    /* indicate remap format */
>      }
> 
> -    *iremap_entry = new_ire;
> +    copy_irte_to_irt(iremap_entry, &new_ire);
>      iommu_flush_cache_entry(iremap_entry, sizeof(*iremap_entry));
>      iommu_flush_iec_index(iommu, 0, index);
> 
> @@ -617,28 +648,7 @@ static int update_irte_for_msi_common(
> 
>      if ( iremap_entry->val != new_ire.val )
>      {
> -        if ( cpu_has_cx16 )
> -        {
> -            __uint128_t ret;
> -            struct iremap_entry old_ire;
> -
> -            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);
> -        }
> -        else
> -        {
> -            iremap_entry->lo = new_ire.lo;
> -            iremap_entry->hi = new_ire.hi;
> -        }
> -
> +        copy_irte_to_irt(iremap_entry, &new_ire);
>          iommu_flush_cache_entry(iremap_entry, sizeof(*iremap_entry));
>          iommu_flush_iec_index(iommu, 0, index);
>      }
> --
> 1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel