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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.04.17 at 09:51, <chao.gao@xxxxxxxxx> wrote:
> 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.

Specifically the one in the else branch.

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

And that's another thing I've already noticed I forgot.

Jan


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