[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v12 6/7] VT-d: introduce update_irte to update irte safely
>>> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |