Re: [PATCH v2 4/8] x86/acpi: separate AMD-Vi and VT-d specific functions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.01.2023 09:44, Xenia Ragiadakou wrote:
> The functions acpi_dmar_init() and acpi_dmar_zap/reinstate() are
> VT-d specific while the function acpi_ivrs_init() is AMD-Vi specific.
> To eliminate dead code, they need to be guarded under CONFIG_INTEL_IOMMU
> and CONFIG_AMD_IOMMU, respectively.
> 
> Instead of adding #ifdef guards around the function calls, implement them
> as empty static inline functions.
> 
> Take the opportunity to move the declarations of acpi_dmar_zap/reinstate() to
> the arch specific header.
> 
> No functional change intended.
> 
> Signed-off-by: Xenia Ragiadakou <burzalodowa@xxxxxxxxx>

While I'm not opposed to ack the change in this form, I have a question
first:

> --- a/xen/arch/x86/include/asm/acpi.h
> +++ b/xen/arch/x86/include/asm/acpi.h
> @@ -140,8 +140,22 @@ extern u32 pmtmr_ioport;
>  extern unsigned int pmtmr_width;
>  
>  void acpi_iommu_init(void);
> +
> +#ifdef CONFIG_INTEL_IOMMU
>  int acpi_dmar_init(void);
> +void acpi_dmar_zap(void);
> +void acpi_dmar_reinstate(void);
> +#else
> +static inline int acpi_dmar_init(void) { return -ENODEV; }
> +static inline void acpi_dmar_zap(void) {}
> +static inline void acpi_dmar_reinstate(void) {}
> +#endif

Leaving aside my request to drop that part of patch 3, you've kept
declarations for VT-d in the common header there. Which I consider
correct, knowing that VT-d was also used on IA-64 at the time. As
a result I would suppose movement might better be done in the other
direction here.

> +#ifdef CONFIG_AMD_IOMMU
>  int acpi_ivrs_init(void);
> +#else
> +static inline int acpi_ivrs_init(void) { return -ENODEV; }
> +#endif

For AMD, otoh, without there being a 2nd architecture re-using
their IOMMU, moving into the x86-specific header is certainly fine,
no matter that there's a slim chance that this may need moving the
other direction down the road.

Jan