Re: [PATCH v2.1 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

> --- 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().

> --- 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>

Jan