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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.04.17 at 02:30, <chao.gao@xxxxxxxxx> wrote:
> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -169,10 +169,55 @@ bool_t __init iommu_supports_eim(void)
>      return 1;
>  }
>  
> +/*
> + * Assume iremap_lock has been acquired. It is to make sure software will not
> + * change the same IRTE behind us. With this assumption, if only high qword 
> or
> + * low qword in IRTE is to be updated, this function's atomic variant can
> + * present an atomic update to VT-d hardware even when cmpxchg16b
> + * instruction is not supported.
> + */
> +static void update_irte(struct iommu *iommu, struct iremap_entry *entry,
> +                        const struct iremap_entry *new_ire, bool atomic)
> +{
> +    ASSERT(spin_is_locked(&iommu_ir_ctrl(iommu)->iremap_lock));
> +
> +    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
> +    {
> +        /*
> +         * If the caller requests a atomic update but we can't meet it, 
> +         * a bug will be raised.
> +         */
> +        if ( entry->lo == new_ire->lo )
> +            write_atomic(&entry->hi, new_ire->hi);
> +        else if ( entry->hi == new_ire->hi )
> +            write_atomic(&entry->lo, new_ire->lo);
> +        else if ( !atomic )
> +            *entry = *new_ire;
> +        else
> +            BUG();
> +    }

Sadly the comment still uses the word atomic, and there's still no
mention of whether (and if so, how) the hardware may update an
IRTE behind your back. But since Kevin gave his R-b, I guess I'll
have to give up on this.

> @@ -639,7 +689,10 @@ static int msi_msg_to_remap_entry(
>      remap_rte->address_hi = 0;
>      remap_rte->data = index - i;
>  
> -    *iremap_entry = new_ire;
> +    update_irte(iommu, iremap_entry, &new_ire, msi_desc->irte_initialized);
> +    if ( !msi_desc->irte_initialized )
> +        msi_desc->irte_initialized = true;

I don't see the point of the conditional, and I guess I'll take the
liberty to remove it, should I end up committing this patch.

x86 parts
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan


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