Re: [Xen-devel] [PATCH 1/5] IOMMU: iommu_intremap is x86-only

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/02/2020 12:26, Jan Beulich wrote:
> Provide a #define for other cases; it didn't seem worthwhile to me to
> introduce an IOMMU_INTREMAP Kconfig option at this point.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1299,6 +1299,8 @@ boolean (e.g. `iommu=no`) can override t
>      generation of IOMMUs only supported DMA remapping, and Interrupt 
> Remapping
>      appeared in the second generation.
>  
> +    This option is not valid on Arm.

The longevity of this comment would be greater if it were phrased as "is
only valid on x86", especially given the RFC RISCV series on list.

> +
>  *   The `intpost` boolean controls the Posted Interrupt sub-feature.  In
>      combination with APIC acceleration (VT-x APICV, SVM AVIC), the IOMMU can
>      be configured to deliver interrupts from assigned PCI devices directly
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -35,7 +35,6 @@ bool __read_mostly iommu_quarantine = tr
>  bool_t __read_mostly iommu_igfx = 1;
>  bool_t __read_mostly iommu_snoop = 1;
>  bool_t __read_mostly iommu_qinval = 1;
> -enum iommu_intremap __read_mostly iommu_intremap = iommu_intremap_full;
>  bool_t __read_mostly iommu_crash_disable;
>  
>  static bool __hwdom_initdata iommu_hwdom_none;
> @@ -90,8 +89,10 @@ static int __init parse_iommu_param(cons
>              iommu_snoop = val;
>          else if ( (val = parse_boolean("qinval", s, ss)) >= 0 )
>              iommu_qinval = val;
> +#ifndef iommu_intremap
>          else if ( (val = parse_boolean("intremap", s, ss)) >= 0 )
>              iommu_intremap = val ? iommu_intremap_full : iommu_intremap_off;
> +#endif

The use of ifndef in particular makes the result very weird to read. 
There appear to be no uses of iommu_intremap outside of x86 code, other
than in this setup, so having it false in the !CONFIG_X86 case isn't
helpful.

How about just guarding uses of the variable with IS_ENABLED(CONFIG_X86)
and a common extern?  We use this DCE trick already to reduce the
ifdefary in the code.

The result would certainly be easier to follow

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel