| Message ID | 20230104084502.61734-5-burzalodowa@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Make x86 IOMMU driver support configurable | expand |
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@gmail.com> 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
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@gmail.com> > > 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.
On 12.01.2023 13:08, Xenia Ragiadakou wrote: > On 1/12/23 13:37, Jan Beulich wrote: >> On 04.01.2023 09:44, Xenia Ragiadakou wrote: >>> --- 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. I would prefer you doing so, yes, of course unless others object. Jan
diff --git a/xen/arch/x86/include/asm/acpi.h b/xen/arch/x86/include/asm/acpi.h index c453450a74..ca8529a7c8 100644 --- 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 + +#ifdef CONFIG_AMD_IOMMU int acpi_ivrs_init(void); +#else +static inline int acpi_ivrs_init(void) { return -ENODEV; } +#endif void acpi_mmcfg_init(void); diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h index 1b9c75e68f..82b24a5ef0 100644 --- a/xen/include/xen/acpi.h +++ b/xen/include/xen/acpi.h @@ -206,9 +206,6 @@ static inline int acpi_get_pxm(acpi_handle handle) void acpi_reboot(void); -void acpi_dmar_zap(void); -void acpi_dmar_reinstate(void); - #endif /* __ASSEMBLY__ */ #endif /*_LINUX_ACPI_H*/
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@gmail.com> --- Changes in v2: - replace CONFIG_INTEL_VTD with CONFIG_INTEL_IOMMU xen/arch/x86/include/asm/acpi.h | 14 ++++++++++++++ xen/include/xen/acpi.h | 3 --- 2 files changed, 14 insertions(+), 3 deletions(-)