Re: [Xen-devel] [PATCH v11 5/6] VT-d: introduce update_irte to update irte safely

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.03.17 at 07:11, <chao.gao@xxxxxxxxx> wrote:
> 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.

There's nothing atomic below - between the compares and stores
the value in the table could change. Please don't make false
promises in comments.

> +         * If the caller requests a atomic update but we can't meet it, 
> +         * 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;

Best effort would still call for use of write_atomic() instead of both
of the assignments above.

> @@ -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);

Seems like you'd better call update_irte() here directly, instead of
having this if/else. Which puts under question the usefulness of the
two wrappers.

> @@ -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;
> +    }

Same here.

I also wonder whether you really need the new flag: The function
knows whether it's initializing the IRTE for the first time, as it
allocates a table index just like ioapic_rte_to_remap_entry() does
(where you get away without a new flag).

Jan

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