Re: [XEN PATCH v1 2/2] x86/amd: optional build of amd.c

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.08.2024 12:11, Sergiy Kibrik wrote:
> --- a/xen/arch/x86/include/asm/amd.h
> +++ b/xen/arch/x86/include/asm/amd.h
> @@ -158,13 +158,21 @@
>  #define is_zen4_uarch()   boot_cpu_has(X86_FEATURE_AUTO_IBRS)
>  
>  struct cpuinfo_x86;
> +#ifdef CONFIG_AMD
>  int cpu_has_amd_erratum(const struct cpuinfo_x86 *cpu, int osvw_id, ...);
> +#else
> +static inline int cpu_has_amd_erratum(const struct cpuinfo_x86 *cpu, int 
> osvw_id, ...)

Nit: Too long line.

> +{
> +    return false;

I wouldn't mind if you consistently changed the function to return
bool, but as long as it returns int I don't think it should be returning
false.

> @@ -173,5 +181,19 @@ extern bool amd_virt_spec_ctrl;
>  bool amd_setup_legacy_ssbd(void);
>  void amd_set_legacy_ssbd(bool enable);
>  void amd_set_cpuid_user_dis(bool enable);
> +#else
> +static inline void amd_set_cpuid_user_dis(bool enable) {}
> +static inline void amd_set_legacy_ssbd(bool enable) {}
> +static inline bool amd_setup_legacy_ssbd(void)
> +{
> +    return false;
> +}

Nit: Would be a little nicer if these were in the same order as their
corresponding declarations. However, along the lines of one of my
comments on the Intel counterpart patch ...

> +#define amd_acpi_c1e_quirk (false)
> +#define amd_virt_spec_ctrl (false)
> +#define amd_legacy_ssbd (false)
> +
> +static inline void amd_check_disable_c1e(unsigned int port, u8 value) {}
> +#endif

... question overall is how many of these stubs are really required,
once clearly AMD-only code is properly taken care of (perhaps not just
in spec_ctrl.c).

Jan