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

[Chao Gao <chao.gao@intel.com>]

On Fri, Mar 31, 2017 at 04:01:31AM -0600, Jan Beulich wrote:
>>>> On 29.03.17 at 07:11, <chao.gao@intel.com> wrote:
>> +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.

Ok. I agree. Then do you think the parameter 'atomic' of this function is proper?

I think this atomic means the caller wants this update to be presented to VT-d hardware
as a atomic update. That's to say, no intermediate, invalid IRTE can be seen by hardware.

>
>> +         * 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.

Will fix. Your concern is compiler would wrongly optimize the assignments?

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

agree. Will remove 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).

For multi-vector msi case, I don't have a clean solution to get away this flag.
The problem here is that it allocates several table indexes for multi-vector msi
and only initialize the first one. For others, it isn't aware whether the IRTE
is initialized or not.

Thank
Chao

>
>Jan

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