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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Thursday, March 16, 2017 6:29 PM
> 
> >>> 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()?
> 
> >>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.
> 

Just had a discussion with Chao. Here is a summary:

- PI implies availability of cmpxchg16b, so can always request atomicity

- The problem is really about remap mode. cmpxchg16b might be not
available on those platforms:

        * for init/clear operations, atomicity is not a requirement. We
can have high/low parts updated separately;

        * for reprogram operations (e.g. irq balance), atomicity
is a requirement. But in reality only lower 64bit should be touched.

Then:

- for remapping IRTE
        * do update_irte(high) and update_irte(low) separately
when IRTE is newly allocated
        * Then only do update_irte(low) in case an IRTE is found

- for posting IRTE
        * always do update_irte(high&low)

Then we can use the pseudo code you suggested.

Thanks
Kevin

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