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

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

On 1/12/23 13:37, Jan Beulich wrote:
> 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.
I moved it to the x86-specific header because acpi_dmar_init() was 
declared there.
I can move all of them to the common header.

> > +#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.
--
Xenia