[XEN PATCH] x86/hvm: make ACPI PM timer support optional

[XEN PATCH] x86/hvm: make ACPI PM timer support optional

[Sergiy Kibrik]

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(-)

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);
-- 
2.25.1

Re: [XEN PATCH] x86/hvm: make ACPI PM timer support optional

[Roger Pau Monné]

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.

Re: [XEN PATCH] x86/hvm: make ACPI PM timer support optional

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 671 bytes --]

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.

Re: [XEN PATCH] x86/hvm: make ACPI PM timer support optional

[Sergiy Kibrik]

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

Re: [XEN PATCH] x86/hvm: make ACPI PM timer support optional

[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.

Thanks, Roger.

Re: [XEN PATCH] x86/hvm: make ACPI PM timer support optional

[Sergiy Kibrik]

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

Re: [XEN PATCH] x86/hvm: make ACPI PM timer support optional

[Roger Pau Monné]

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.

Re: [XEN PATCH] x86/hvm: make ACPI PM timer support optional

[Jan Beulich]

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

Re: [XEN PATCH] x86/hvm: make ACPI PM timer support optional

[Sergiy Kibrik]

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

Re: [XEN PATCH] x86/hvm: make ACPI PM timer support optional

[Jan Beulich]

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

Re: [XEN PATCH] x86/hvm: make ACPI PM timer support optional

[Sergiy Kibrik]

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

Re: [XEN PATCH] x86/hvm: make ACPI PM timer support optional

[Jan Beulich]

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

Re: [XEN PATCH] x86/hvm: make ACPI PM timer support optional

[Andrew Cooper]

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

Re: [XEN PATCH] x86/hvm: make ACPI PM timer support optional

[Sergiy Kibrik]

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