| Message ID | 3ef3724e21b196edd277c94598ebe6624813ede4.1728032664.git.Sergiy_Kibrik@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | configurable stdvga & pmtimer emulation | expand |
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@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.
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);
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(-)