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

[Chao Gao <chao.gao@xxxxxxxxx>]

On Fri, Apr 07, 2017 at 06:27:39AM -0600, Jan Beulich wrote:
>>>> 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.

To make it clearer, the comment you mentioned is the comment in the else()
branch or the comment before this function (or both)?  I will fix it
in a new patch.

>
>> @@ -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.

Yes, please.

Thanks
Chao

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

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