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 <jbeul...@suse.com>
Thanks.
~Andrew
