[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v11 5/6] VT-d: introduce update_irte to update irte safely
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? 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. > >> + * 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? > >> @@ -353,7 +409,11 @@ static int ioapic_rte_to_remap_entry(struct iommu >> *iommu, >> remap_rte->format = 1; /* indicate remap format */ >> } >> >> - *iremap_entry = new_ire; >> + if ( init ) >> + update_irte_non_atomic(iremap_entry, &new_ire); >> + else >> + update_irte_atomic(iremap_entry, &new_ire); > >Seems like you'd better call update_irte() here directly, instead of >having this if/else. Which puts under question the usefulness of the >two wrappers. agree. Will remove the two wrappers. > >> @@ -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. Thank Chao > >Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |