| Message ID | 20240916063757.990070-1-Sergiy_Kibrik@epam.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [XEN] x86/hvm: make ACPI PM timer support optional | expand |
On Mon, Sep 16, 2024 at 09:37:57AM +0300, Sergiy Kibrik wrote: > Introduce config option X86_PMTIMER so that pmtimer driver can be disabled on > systems that don't need it. Same comment as in the VGA patch, you need to handle the user passing X86_EMU_PM. It's not OK to just ignore the flag if the hypervisor is built without ACPI PM timer support. Regards, Roger.
On Mon, 16 Sep 2024, Roger Pau Monné wrote: > On Mon, Sep 16, 2024 at 09:37:57AM +0300, Sergiy Kibrik wrote: > > Introduce config option X86_PMTIMER so that pmtimer driver can be disabled on > > systems that don't need it. > > Same comment as in the VGA patch, you need to handle the user passing > X86_EMU_PM. It's not OK to just ignore the flag if the hypervisor is > built without ACPI PM timer support. I also think that the flag should not be ignored. I think that Xen should return error if a user is passing a domain feature not supported by a particular version of the Xen build. I don't think that libxl needs to be changed as part of this patch necessarily.
16.09.24 22:57, Stefano Stabellini: > On Mon, 16 Sep 2024, Roger Pau Monné wrote: >> On Mon, Sep 16, 2024 at 09:37:57AM +0300, Sergiy Kibrik wrote: >>> Introduce config option X86_PMTIMER so that pmtimer driver can be disabled on >>> systems that don't need it. >> >> Same comment as in the VGA patch, you need to handle the user passing >> X86_EMU_PM. It's not OK to just ignore the flag if the hypervisor is >> built without ACPI PM timer support. > > I also think that the flag should not be ignored. I think that Xen > should return error if a user is passing a domain feature not supported > by a particular version of the Xen build. I don't think that libxl needs > to be changed as part of this patch necessarily. It looks like toolstack always leaves X86_EMU_PM bit enabled, so that part may also require changes. -Sergiy
On Wed, Sep 18, 2024 at 12:29:39PM +0300, Sergiy Kibrik wrote: > 16.09.24 22:57, Stefano Stabellini: > > On Mon, 16 Sep 2024, Roger Pau Monné wrote: > > > On Mon, Sep 16, 2024 at 09:37:57AM +0300, Sergiy Kibrik wrote: > > > > Introduce config option X86_PMTIMER so that pmtimer driver can be disabled on > > > > systems that don't need it. > > > > > > Same comment as in the VGA patch, you need to handle the user passing > > > X86_EMU_PM. It's not OK to just ignore the flag if the hypervisor is > > > built without ACPI PM timer support. > > > > I also think that the flag should not be ignored. I think that Xen > > should return error if a user is passing a domain feature not supported > > by a particular version of the Xen build. I don't think that libxl needs > > to be changed as part of this patch necessarily. > > It looks like toolstack always leaves X86_EMU_PM bit enabled, so that part > may also require changes. I think you will be unable to create HVM guests, but that's kind of expected if ACPI PM emulation is removed from the hypervisor (it won't be an HVM guest anymore if it doesn't have ACPI PM). PVH guest don't set X86_EMU_PM so you should be able to create those fine. Thanks, Roger.
18.09.24 12:41, Roger Pau Monné: > On Wed, Sep 18, 2024 at 12:29:39PM +0300, Sergiy Kibrik wrote: >> 16.09.24 22:57, Stefano Stabellini: >>> On Mon, 16 Sep 2024, Roger Pau Monné wrote: >>>> On Mon, Sep 16, 2024 at 09:37:57AM +0300, Sergiy Kibrik wrote: >>>>> Introduce config option X86_PMTIMER so that pmtimer driver can be disabled on >>>>> systems that don't need it. >>>> >>>> Same comment as in the VGA patch, you need to handle the user passing >>>> X86_EMU_PM. It's not OK to just ignore the flag if the hypervisor is >>>> built without ACPI PM timer support. >>> >>> I also think that the flag should not be ignored. I think that Xen >>> should return error if a user is passing a domain feature not supported >>> by a particular version of the Xen build. I don't think that libxl needs >>> to be changed as part of this patch necessarily. >> >> It looks like toolstack always leaves X86_EMU_PM bit enabled, so that part >> may also require changes. > > I think you will be unable to create HVM guests, but that's kind of > expected if ACPI PM emulation is removed from the hypervisor (it won't > be an HVM guest anymore if it doesn't have ACPI PM). > > PVH guest don't set X86_EMU_PM so you should be able to create those > fine. > would the check like this be enough?: --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -758,6 +758,9 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags) (X86_EMU_ALL & ~(X86_EMU_VPCI | X86_EMU_USE_PIRQ)) && emflags != X86_EMU_LAPIC ) return false; + if ( !is_hardware_domain(d) && + (emflags & X86_EMU_PM) && !IS_ENABLED(CONFIG_X86_PMTIMER)) + return false; } else if ( emflags != 0 && emflags != X86_EMU_PIT ) { (probably with X86_PMTIMER option depending on PV) -Sergiy
On Wed, Sep 18, 2024 at 04:35:21PM +0300, Sergiy Kibrik wrote: > 18.09.24 12:41, Roger Pau Monné: > > On Wed, Sep 18, 2024 at 12:29:39PM +0300, Sergiy Kibrik wrote: > > > 16.09.24 22:57, Stefano Stabellini: > > > > On Mon, 16 Sep 2024, Roger Pau Monné wrote: > > > > > On Mon, Sep 16, 2024 at 09:37:57AM +0300, Sergiy Kibrik wrote: > > > > > > Introduce config option X86_PMTIMER so that pmtimer driver can be disabled on > > > > > > systems that don't need it. > > > > > > > > > > Same comment as in the VGA patch, you need to handle the user passing > > > > > X86_EMU_PM. It's not OK to just ignore the flag if the hypervisor is > > > > > built without ACPI PM timer support. > > > > > > > > I also think that the flag should not be ignored. I think that Xen > > > > should return error if a user is passing a domain feature not supported > > > > by a particular version of the Xen build. I don't think that libxl needs > > > > to be changed as part of this patch necessarily. > > > > > > It looks like toolstack always leaves X86_EMU_PM bit enabled, so that part > > > may also require changes. > > > > I think you will be unable to create HVM guests, but that's kind of > > expected if ACPI PM emulation is removed from the hypervisor (it won't > > be an HVM guest anymore if it doesn't have ACPI PM). > > > > PVH guest don't set X86_EMU_PM so you should be able to create those > > fine. > > > > would the check like this be enough?: > > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -758,6 +758,9 @@ static bool emulation_flags_ok(const struct domain *d, > uint32_t emflags) > (X86_EMU_ALL & ~(X86_EMU_VPCI | X86_EMU_USE_PIRQ)) && > emflags != X86_EMU_LAPIC ) > return false; > + if ( !is_hardware_domain(d) && I don't think you need to gate this to the hardware domain? IOW: if it's build time disabled, it's not available for the hardware domain either. Seeing as there are several options you want to disable at build time, it might be best to keep a mask, something like: const uint32_t disabled_mask = (!IS_ENABLED(CONFIG_X86_PMTIMER) ? X86_EMU_PM : 0) | (!IS_ENABLED(CONFIG_X86_STDVGA) ? X86_EMU_VGA : 0); And then: if ( emflags & disabled_mask ) return false; You also want to adjust the has_foo() macros to short-circuit them: #define has_vpm(d) (IS_ENABLED(CONFIG_X86_PMTIMER) && !!((d)->arch.emulation_flags & X86_EMU_PM)) Also all those Kconfig options likely want to be inside of a separate Kconfig section, rather than mixed with the rest of generic x86 arch options. It would nice to have all the options grouped inside of a "Emulated device support" sub section or similar. Thanks, Roger.
On 18.09.2024 15:35, Sergiy Kibrik wrote: > 18.09.24 12:41, Roger Pau Monné: >> On Wed, Sep 18, 2024 at 12:29:39PM +0300, Sergiy Kibrik wrote: >>> 16.09.24 22:57, Stefano Stabellini: >>>> On Mon, 16 Sep 2024, Roger Pau Monné wrote: >>>>> On Mon, Sep 16, 2024 at 09:37:57AM +0300, Sergiy Kibrik wrote: >>>>>> Introduce config option X86_PMTIMER so that pmtimer driver can be disabled on >>>>>> systems that don't need it. >>>>> >>>>> Same comment as in the VGA patch, you need to handle the user passing >>>>> X86_EMU_PM. It's not OK to just ignore the flag if the hypervisor is >>>>> built without ACPI PM timer support. >>>> >>>> I also think that the flag should not be ignored. I think that Xen >>>> should return error if a user is passing a domain feature not supported >>>> by a particular version of the Xen build. I don't think that libxl needs >>>> to be changed as part of this patch necessarily. >>> >>> It looks like toolstack always leaves X86_EMU_PM bit enabled, so that part >>> may also require changes. >> >> I think you will be unable to create HVM guests, but that's kind of >> expected if ACPI PM emulation is removed from the hypervisor (it won't >> be an HVM guest anymore if it doesn't have ACPI PM). >> >> PVH guest don't set X86_EMU_PM so you should be able to create those >> fine. >> > > would the check like this be enough?: > > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -758,6 +758,9 @@ static bool emulation_flags_ok(const struct domain > *d, uint32_t emflags) > (X86_EMU_ALL & ~(X86_EMU_VPCI | X86_EMU_USE_PIRQ)) && > emflags != X86_EMU_LAPIC ) > return false; > + if ( !is_hardware_domain(d) && > + (emflags & X86_EMU_PM) && !IS_ENABLED(CONFIG_X86_PMTIMER)) > + return false; > } > else if ( emflags != 0 && emflags != X86_EMU_PIT ) > { Why the is_hardware_domain() part of the check? > (probably with X86_PMTIMER option depending on PV) HVM you mean? Jan
23.09.24 13:01, Jan Beulich: >> --- a/xen/arch/x86/domain.c >> +++ b/xen/arch/x86/domain.c >> @@ -758,6 +758,9 @@ static bool emulation_flags_ok(const struct domain >> *d, uint32_t emflags) >> (X86_EMU_ALL & ~(X86_EMU_VPCI | X86_EMU_USE_PIRQ)) && >> emflags != X86_EMU_LAPIC ) >> return false; >> + if ( !is_hardware_domain(d) && >> + (emflags & X86_EMU_PM) && !IS_ENABLED(CONFIG_X86_PMTIMER)) >> + return false; >> } >> else if ( emflags != 0 && emflags != X86_EMU_PIT ) >> { > Why the is_hardware_domain() part of the check? the idea was that since hardware domain has full hardware access it probably doesn't need emulated timer. But this check will be dropped anyway, as Roger suggested. > >> (probably with X86_PMTIMER option depending on PV) > HVM you mean? > I wanted to do it like this: --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -386,7 +386,7 @@ config ALTP2M If unsure, stay with defaults. config X86_PMTIMER - bool "ACPI PM timer emulation support" if EXPERT + bool "ACPI PM timer emulation support" if EXPERT && PV default y depends on HVM help to allow it to be disabled when PV is on and prevent situation when pvh domain can't be created because !PV and hvm domain can't be created either without emulated timer. -Sergiy
On 27.09.2024 11:42, Sergiy Kibrik wrote: > 23.09.24 13:01, Jan Beulich: >>> --- a/xen/arch/x86/domain.c >>> +++ b/xen/arch/x86/domain.c >>> @@ -758,6 +758,9 @@ static bool emulation_flags_ok(const struct domain >>> *d, uint32_t emflags) >>> (X86_EMU_ALL & ~(X86_EMU_VPCI | X86_EMU_USE_PIRQ)) && >>> emflags != X86_EMU_LAPIC ) >>> return false; >>> + if ( !is_hardware_domain(d) && >>> + (emflags & X86_EMU_PM) && !IS_ENABLED(CONFIG_X86_PMTIMER)) >>> + return false; >>> } >>> else if ( emflags != 0 && emflags != X86_EMU_PIT ) >>> { >> Why the is_hardware_domain() part of the check? > > the idea was that since hardware domain has full hardware access it > probably doesn't need emulated timer. But this check will be dropped > anyway, as Roger suggested. > >> >>> (probably with X86_PMTIMER option depending on PV) >> HVM you mean? >> > > I wanted to do it like this: > > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -386,7 +386,7 @@ config ALTP2M > If unsure, stay with defaults. > > config X86_PMTIMER > - bool "ACPI PM timer emulation support" if EXPERT > + bool "ACPI PM timer emulation support" if EXPERT && PV > default y > depends on HVM > help > > to allow it to be disabled when PV is on and prevent situation when pvh > domain can't be created because !PV and hvm domain can't be created > either without emulated timer. How does PV matter for this "depends on HVM" setting? Jan
27.09.24 12:44, Jan Beulich: >>>> (probably with X86_PMTIMER option depending on PV) >>> HVM you mean? >>> >> I wanted to do it like this: >> >> --- a/xen/arch/x86/Kconfig >> +++ b/xen/arch/x86/Kconfig >> @@ -386,7 +386,7 @@ config ALTP2M >> If unsure, stay with defaults. >> >> config X86_PMTIMER >> - bool "ACPI PM timer emulation support" if EXPERT >> + bool "ACPI PM timer emulation support" if EXPERT && PV >> default y >> depends on HVM >> help >> >> to allow it to be disabled when PV is on and prevent situation when pvh >> domain can't be created because !PV and hvm domain can't be created >> either without emulated timer. > How does PV matter for this "depends on HVM" setting? Options are dependant on HVM option, because pmtimer & stdvga drivers are part of HVM support code. At the same time these options are allowed to be de-selected when PV/PVH enabled, because only PV/PVH domains can be created when stdvga or pmtimer are disabled (if both PV & any emulated device are disabled one would end up with weird system that can't create any VM). So that's the idea behind making all these dependencies. -Sergiy
On 30.09.2024 13:03, Sergiy Kibrik wrote: > 27.09.24 12:44, Jan Beulich: >>>>> (probably with X86_PMTIMER option depending on PV) >>>> HVM you mean? >>>> >>> I wanted to do it like this: >>> >>> --- a/xen/arch/x86/Kconfig >>> +++ b/xen/arch/x86/Kconfig >>> @@ -386,7 +386,7 @@ config ALTP2M >>> If unsure, stay with defaults. >>> >>> config X86_PMTIMER >>> - bool "ACPI PM timer emulation support" if EXPERT >>> + bool "ACPI PM timer emulation support" if EXPERT && PV >>> default y >>> depends on HVM >>> help >>> >>> to allow it to be disabled when PV is on and prevent situation when pvh >>> domain can't be created because !PV and hvm domain can't be created >>> either without emulated timer. >> How does PV matter for this "depends on HVM" setting? > > Options are dependant on HVM option, because pmtimer & stdvga drivers > are part of HVM support code. > At the same time these options are allowed to be de-selected when PV/PVH > enabled, because only PV/PVH domains can be created when stdvga or > pmtimer are disabled But PV != PVH. > (if both PV & any emulated device are disabled one would end up with > weird system that can't create any VM). It would still be able to create PVH domains, wouldn't it? Also we allow e.g. both PV and HVM being turned off. That'll lead to no useful domains being possible to create, too. Conceptually no different when PV is off while HVM/PVH domains cannot be created for reasons like missing emulation. Jan
On 30/09/2024 12:27 pm, Jan Beulich wrote: > On 30.09.2024 13:03, Sergiy Kibrik wrote: >> 27.09.24 12:44, Jan Beulich: >>>>>> (probably with X86_PMTIMER option depending on PV) >>>>> HVM you mean? >>>>> >>>> I wanted to do it like this: >>>> >>>> --- a/xen/arch/x86/Kconfig >>>> +++ b/xen/arch/x86/Kconfig >>>> @@ -386,7 +386,7 @@ config ALTP2M >>>> If unsure, stay with defaults. >>>> >>>> config X86_PMTIMER >>>> - bool "ACPI PM timer emulation support" if EXPERT >>>> + bool "ACPI PM timer emulation support" if EXPERT && PV >>>> default y >>>> depends on HVM >>>> help >>>> >>>> to allow it to be disabled when PV is on and prevent situation when pvh >>>> domain can't be created because !PV and hvm domain can't be created >>>> either without emulated timer. >>> How does PV matter for this "depends on HVM" setting? >> Options are dependant on HVM option, because pmtimer & stdvga drivers >> are part of HVM support code. >> At the same time these options are allowed to be de-selected when PV/PVH >> enabled, because only PV/PVH domains can be created when stdvga or >> pmtimer are disabled > But PV != PVH. Furthermore, be aware that there's no such thing as PVH in Xen. It's toolstack level "branding" for want of a better term. In Xen, there is PV (ring privileging) and HVM (hardware virt). PVH is a toolstack level construct which passes a different set of emulate flags, and doesn't set up qemu (on ioreq server 0) by default. ~Andrew
30.09.24 14:33, Andrew Cooper: >>> Options are dependant on HVM option, because pmtimer & stdvga drivers >>> are part of HVM support code. >>> At the same time these options are allowed to be de-selected when PV/PVH >>> enabled, because only PV/PVH domains can be created when stdvga or >>> pmtimer are disabled >> But PV != PVH. > Furthermore, be aware that there's no such thing as PVH in Xen. It's > toolstack level "branding" for want of a better term. > > In Xen, there is PV (ring privileging) and HVM (hardware virt). > > PVH is a toolstack level construct which passes a different set of > emulate flags, and doesn't set up qemu (on ioreq server 0) by default. > understood, then I won't complicate configuration by linking it with PV. Thanks for explanation! -Sergiy
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 62f0b5e0f4..0a762be14e 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -385,6 +385,15 @@ config ALTP2M If unsure, stay with defaults. +config X86_PMTIMER + bool "ACPI PM timer emulation support" if EXPERT + default y + depends on HVM + help + Build pmtimer driver that emulates ACPI PM timer for HVM guests. + + If unsure, say Y. + endmenu source "common/Kconfig" 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/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 driver can be disabled on systems that don't need it. Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> --- xen/arch/x86/Kconfig | 9 +++++++++ xen/arch/x86/hvm/Makefile | 2 +- xen/arch/x86/include/asm/acpi.h | 5 +++++ xen/arch/x86/include/asm/hvm/vpt.h | 10 ++++++++++ 4 files changed, 25 insertions(+), 1 deletion(-)