Re: [XEN PATCH v1 1/3] x86/hvm: introduce config option for ACPI PM timer

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Oct 04, 2024 at 12:31:50PM +0300, Sergiy Kibrik wrote:
> Introduce config option X86_PMTIMER so that pmtimer emulation driver can later
> be made configurable and be disabled on systems that don't need it.
> 
> As a first step the option is hidden from user, thus not making any functional
> changes here.
> 
> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@xxxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> ---
>  xen/arch/x86/Kconfig               |  3 +++
>  xen/arch/x86/hvm/Makefile          |  2 +-
>  xen/arch/x86/include/asm/acpi.h    |  5 +++++
>  xen/arch/x86/include/asm/domain.h  |  3 ++-
>  xen/arch/x86/include/asm/hvm/vpt.h | 10 ++++++++++
>  5 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 9cdd04721a..95275dc17e 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -144,6 +144,9 @@ config INTEL_VMX
>         If your system includes a processor with Intel VT-x support, say Y.
>         If in doubt, say Y.
>  
> +config X86_PMTIMER
> +     def_bool HVM

The chunk in patch 3 that fill this option needs to be moved here,
together with the updated checks in emulation_flags_ok().

>  config XEN_SHSTK
>       bool "Supervisor Shadow Stacks"
>       depends on HAS_AS_CET_SS
> diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
> index 4c1fa5c6c2..321241f0bf 100644
> --- a/xen/arch/x86/hvm/Makefile
> +++ b/xen/arch/x86/hvm/Makefile
> @@ -18,7 +18,7 @@ obj-y += irq.o
>  obj-y += monitor.o
>  obj-y += mtrr.o
>  obj-y += nestedhvm.o
> -obj-y += pmtimer.o
> +obj-$(CONFIG_X86_PMTIMER) += pmtimer.o

I think you can also make the hvm_hw_acpi field in struct hvm_domain
presence dependent on CONFIG_X86_PMTIMER being enabled.

>  obj-y += quirks.o
>  obj-y += rtc.o
>  obj-y += save.o
> diff --git a/xen/arch/x86/include/asm/acpi.h b/xen/arch/x86/include/asm/acpi.h
> index 217819dd61..8d92014ae9 100644
> --- a/xen/arch/x86/include/asm/acpi.h
> +++ b/xen/arch/x86/include/asm/acpi.h
> @@ -150,8 +150,13 @@ void acpi_mmcfg_init(void);
>  /* Incremented whenever we transition through S3. Value is 1 during boot. */
>  extern uint32_t system_reset_counter;
>  
> +#ifdef CONFIG_X86_PMTIMER
>  void hvm_acpi_power_button(struct domain *d);
>  void hvm_acpi_sleep_button(struct domain *d);
> +#else
> +static inline void hvm_acpi_power_button(struct domain *d) {}
> +static inline void hvm_acpi_sleep_button(struct domain *d) {}
> +#endif

It would be best if those functions returned -ENODEV when the
interface is not available, but that's an existing issue, so won't
insist in you fixing it here.

>  /* suspend/resume */
>  void save_rest_processor_state(void);
> diff --git a/xen/arch/x86/include/asm/domain.h 
> b/xen/arch/x86/include/asm/domain.h
> index bdcdb8de09..3f65bfd190 100644
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -496,7 +496,8 @@ struct arch_domain
>  
>  #define has_vlapic(d)      (!!((d)->arch.emulation_flags & X86_EMU_LAPIC))
>  #define has_vhpet(d)       (!!((d)->arch.emulation_flags & X86_EMU_HPET))
> -#define has_vpm(d)         (!!((d)->arch.emulation_flags & X86_EMU_PM))
> +#define has_vpm(d)         (IS_ENABLED(CONFIG_X86_PMTIMER) && \
> +                            !!((d)->arch.emulation_flags & X86_EMU_PM))

Do you really need the IS_ENABLED() here?  If you modify
emulation_flags_ok() to reject the flag if not available it won't be
possible for any domain to have it set.

Thanks, Roger.