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@epam.com>
CC: Jan Beulich <jbeulich@suse.com>
---
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
+
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
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
/* 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))
#define has_vrtc(d) (!!((d)->arch.emulation_flags & X86_EMU_RTC))
#define has_vioapic(d) (!!((d)->arch.emulation_flags & X86_EMU_IOAPIC))
#define has_vpic(d) (!!((d)->arch.emulation_flags & X86_EMU_PIC))
diff --git a/xen/arch/x86/include/asm/hvm/vpt.h b/xen/arch/x86/include/asm/hvm/vpt.h
index 0b92b28625..333b346068 100644
--- a/xen/arch/x86/include/asm/hvm/vpt.h
+++ b/xen/arch/x86/include/asm/hvm/vpt.h
@@ -187,10 +187,20 @@ void rtc_deinit(struct domain *d);
void rtc_reset(struct domain *d);
void rtc_update_clock(struct domain *d);
+#ifdef CONFIG_X86_PMTIMER
void pmtimer_init(struct vcpu *v);
void pmtimer_deinit(struct domain *d);
void pmtimer_reset(struct domain *d);
int pmtimer_change_ioport(struct domain *d, uint64_t version);
+#else
+static inline void pmtimer_init(struct vcpu *v) {}
+static inline void pmtimer_deinit(struct domain *d) {}
+static inline void pmtimer_reset(struct domain *d) {}
+static inline int pmtimer_change_ioport(struct domain *d, uint64_t version)
+{
+ return -ENODEV;
+}
+#endif
void hpet_init(struct domain *d);
void hpet_deinit(struct domain *d);
--
2.25.1On 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@epam.com>
> CC: Jan Beulich <jbeulich@suse.com>
> ---
> 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.
On 04.10.2024 15:09, Roger Pau Monné wrote: > On Fri, Oct 04, 2024 at 12:31:50PM +0300, Sergiy Kibrik wrote: >> --- 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. With the IS_ENABLED() the only other approach to have the compiler DCE any code left unreachable would be to #define X86_EMU_PM to 0 in that case. I guess I'd slightly prefer that alternative, but otherwise the approach used here would still be wanted imo. Jan
On Fri, 4 Oct 2024, Jan Beulich wrote: > On 04.10.2024 15:09, Roger Pau Monné wrote: > > On Fri, Oct 04, 2024 at 12:31:50PM +0300, Sergiy Kibrik wrote: > >> --- 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. > > With the IS_ENABLED() the only other approach to have the compiler DCE any > code left unreachable would be to #define X86_EMU_PM to 0 in that case. I > guess I'd slightly prefer that alternative, but otherwise the approach used > here would still be wanted imo. The compiler DCE is important, either the approach in this patch or Jan's suggestion would work fine as far as I can tell.
On Fri, Oct 04, 2024 at 02:07:09PM -0700, Stefano Stabellini wrote: > On Fri, 4 Oct 2024, Jan Beulich wrote: > > On 04.10.2024 15:09, Roger Pau Monné wrote: > > > On Fri, Oct 04, 2024 at 12:31:50PM +0300, Sergiy Kibrik wrote: > > >> --- 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. > > > > With the IS_ENABLED() the only other approach to have the compiler DCE any > > code left unreachable would be to #define X86_EMU_PM to 0 in that case. I > > guess I'd slightly prefer that alternative, but otherwise the approach used > > here would still be wanted imo. > > The compiler DCE is important, either the approach in this patch or > Jan's suggestion would work fine as far as I can tell. I guess I was too focused on has_vpm() usage: note that has_vpm() is only used in the file that's being removed from the build, so there will be nothing to DCE afterwards. That however might not be the case for all has_* options, neither for has_vpm() moving forwards. Thanks, Roger.
© 2016 - 2026 Red Hat, Inc.