[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2.1 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber
On 22/02/2022 09:29, Jan Beulich wrote: > On 21.02.2022 19:03, Andrew Cooper wrote: >> Most IOMMU hooks are already altcall for performance reasons. Convert the >> rest of them so we can harden all the hooks in Control Flow Integrity >> configurations. This necessitates the use of iommu_{v,}call() in debug >> builds >> too. >> >> Move the root iommu_ops from __read_mostly to __ro_after_init now that the >> latter exists. There is no need for a forward declaration of vtd_ops any >> more, meaning that __initconst_cf_clobber can be used for VTD and AMD. > The connection between the forward declaration and the annotation addition > isn't really clear to me. > >> --- a/xen/arch/x86/include/asm/iommu.h >> +++ b/xen/arch/x86/include/asm/iommu.h >> @@ -72,7 +72,6 @@ struct arch_iommu >> >> extern struct iommu_ops iommu_ops; >> >> -#ifdef NDEBUG >> # include <asm/alternative.h> >> # define iommu_call(ops, fn, args...) ({ \ >> (void)(ops); \ >> @@ -83,7 +82,6 @@ extern struct iommu_ops iommu_ops; >> (void)(ops); \ >> alternative_vcall(iommu_ops.fn, ## args); \ >> }) >> -#endif >> >> static inline const struct iommu_ops *iommu_get_ops(void) >> { >> @@ -106,7 +104,7 @@ int iommu_setup_hpet_msi(struct msi_desc *); >> static inline int iommu_adjust_irq_affinities(void) >> { >> return iommu_ops.adjust_irq_affinities >> - ? iommu_ops.adjust_irq_affinities() >> + ? iommu_call(iommu_ops, adjust_irq_affinities) > While this (and other instances below) is x86-only code, where - with > the removal of the #ifdef above - we now know the first argument is > always ignored, I think it would still better be of the correct type > (&iommu_ops). Perhaps the "(void)(ops)" in the macro definitions would > better become "ASSERT((ops) == &iommu_ops)", which would check both > type (compile time) and value (runtime). I'm happy to fold that change if you want. It ought to optimise out completely for being > >> --- a/xen/drivers/passthrough/iommu.c >> +++ b/xen/drivers/passthrough/iommu.c >> @@ -540,7 +540,7 @@ int __init iommu_setup(void) >> int iommu_suspend() >> { >> if ( iommu_enabled ) >> - return iommu_get_ops()->suspend(); >> + return iommu_call(iommu_get_ops(), suspend); > This use of iommu_get_ops() in such constructs is a pattern we didn't > have so far. Perhaps it just looks bogus, and all is fine in reality > (apart from the whole idea being wrong for Arm, or really any > environment where multiple dissimilar IOMMUs may be in use). Or wait, > there are pre-existing cases (just not immediately visible when > grep-ing for "iommu_v?call") in iommu_get_reserved_device_memory() and > iommu_setup_hpet_msi(). I think this means your happy(ish) with the change? I agree that this is nonsense on ARM, but the codepath isn't used yet and someone's going to have to reconcile the conflicting views. >> --- a/xen/drivers/passthrough/vtd/iommu.c >> +++ b/xen/drivers/passthrough/vtd/iommu.c >> @@ -56,7 +56,6 @@ bool __read_mostly iommu_snoop = true; >> >> static unsigned int __read_mostly nr_iommus; >> >> -static struct iommu_ops vtd_ops; >> static struct tasklet vtd_fault_tasklet; >> >> static int cf_check setup_hwdom_device(u8 devfn, struct pci_dev *); >> @@ -2794,7 +2793,7 @@ static int __init cf_check >> intel_iommu_quarantine_init(struct domain *d) >> return rc; >> } >> >> -static struct iommu_ops __initdata vtd_ops = { >> +static const struct iommu_ops __initconst_cf_clobber vtd_ops = { > Ah yes, the conversion to const (and the dropping of the forward decl) > could have been part of "VT-d / x86: re-arrange cache syncing". > > With the missing &-s added and preferably with the description adjusted > a little > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx> Thanks. ~Andrew
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |