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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.03.17 at 06:11, <chao.gao@xxxxxxxxx> wrote:
> +static void update_irte(struct iremap_entry *entry,
> +                        const struct iremap_entry *new_ire)
> +{
> +    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 method to update IRTE is safe on condition that
> +         * only the high qword or the low qword is to be updated.
> +         * If entire IRTE is to be updated, callers should make sure the
> +         * IRTE is not in use.
> +         */
> +        entry->lo = new_ire->lo;
> +        entry->hi = new_ire->hi;

How is this any better than structure assignment? Furthermore
the comment here partially contradicts the commit message. I
guess callers need to be given a way (another function parameter?)
to signal the function whether the unsafe variant is okay to use.
You should then add a suitable BUG_ON() in the else path here.

Jan


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