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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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