[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v10 4/6] VT-d: introduce update_irte to update irte safely
>>> 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. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |