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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.04.17 at 21:12, <chao.gao@xxxxxxxxx> wrote:
> On Fri, Mar 31, 2017 at 04:01:31AM -0600, Jan Beulich wrote:
>>>>> On 29.03.17 at 07:11, <chao.gao@xxxxxxxxx> 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?

That depends: As long as no changes behind our backs are possible,
I think it could be left with this name.

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

Right, that's what the comment should say imo.

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

With s/optimize/translate/, yes. If you care about atomicity, you
should guarantee as much of it as is possible. As per what you've
said above, if you weren't using write_atomic() here, you'd have
to prove that byte-wise writing in any order could not lead to a
transiently inconsistent IRTE.

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

Oh, indeed. I did overlook this aspect. May I then suggest to
shorten the field name to e.g. irte_initialized?

Jan


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