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

[Chao Gao <chao.gao@xxxxxxxxx>]

On Thu, Mar 16, 2017 at 04:29:29AM -0600, Jan Beulich wrote:
>>>> On 15.03.17 at 23:39, <chao.gao@xxxxxxxxx> wrote:
>> On Wed, Mar 15, 2017 at 10:48:25AM -0600, Jan Beulich wrote:
>>>>>> On 15.03.17 at 06:11, <chao.gao@xxxxxxxxx> wrote:
>>>> +        /*
>>>> +         * 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
>> 
>> Indeed, not better. when using structure assignment, the assembly code is
>> 48 8b 06                mov    (%rsi),%rax                    
>> 48 8b 56 08             mov    0x8(%rsi),%rdx                 
>> 48 89 07                mov    %rax,(%rdi)                    
>> 48 89 57 08             mov    %rdx,0x8(%rdi)
>> Using the code above, the assembly code is
>> 48 8b 06                mov    (%rsi),%rax                  
>> 48 89 07                mov    %rax,(%rdi)                    
>> 48 8b 46 08             mov    0x8(%rsi),%rax                 
>> 48 89 47 08             mov    %rax,0x8(%rdi)
>> 
>> I thought structure assignment maybe ultilize memcpy considering structure
>> of a big size, so I made this change. I will change this back. Although
>> that, this patch is trying to make the change safer when cmpxchg16() is
>> supported. 
>
>Perhaps you've really meant to use write_atomic()?

I don't understand what you mean. But I think write_atomic may be not related
to the problem how to update a 16 byte memory atomically if cmpxchg16() is not
supported.

>
>>>the comment here partially contradicts the commit message. I
>> 
>> Yes.
>> 
>>>guess callers need to be given a way (another function parameter?)
>>>to signal the function whether the unsafe variant is okay to use.
>> 
>> This means we need to add the new parameter to iommu ops for only
>> IOAPIC/MSI know the entry they want to change is masked. Is there
>> any another reasonable and correct solution?
>
>Well, users you convert in this patch must be okay to use the
>non-atomic variant. The PI user(s) know(s) that cmpxchg16b is
>available, so could always request the safe variant. No need for
>a new parameter higher up in the call trees afaics.
>
>> How about...
>> 
>>>You should then add a suitable BUG_ON() in the else path here.
>> 
>> just add a BUG_ON() like this
>> BUG_ON( (entry->hi != new_ire->hi) && (entry->lo != new_ire->lo) );
>> Adding this BUG_ON() means update_irte() can't be used for initializing
>> or clearing IRTE which are not bugs.
>
>Yes, that's an option too, albeit then I'd suggest (pseudo code)
>
>    if ( high_up_to_date )
>        update_low;
>    else if ( low_up_to_date )
>        update_high;
>    else
>       BUG();
>
>But you'll want to have the okay from Kevin as the maintainer for
>something like this.

ok. I will wait for comments of Kevin.

Thank,
Chao
>
>Jan
>

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