[XEN PATCH v1 0/3] configurable stdvga & pmtimer emulation

[XEN PATCH v1 0/3] configurable stdvga & pmtimer emulation

[Sergiy Kibrik]

This series presents previously posted patches [1,2] with review comments
addressed, Patches introduce build options for stdvga and ACPI PM timer
emulation drivers to make them configurable with possibility to be disabled for
finer-tuned setups that don't need this code.

New config options are now separated in a sub-menu, checks of emulation flags
are added so that domain can't be created if it requires devices that had been
disabled, patches were re-shaped a bit and other comments has been addressed.

  -Sergiy

  1. https://lore.kernel.org/xen-devel/20240912085709.858052-1-Sergiy_Kibrik@epam.com/
  2. https://lore.kernel.org/xen-devel/20240916063757.990070-1-Sergiy_Kibrik@epam.com/


Sergiy Kibrik (3):
  x86/hvm: introduce config option for ACPI PM timer
  x86/hvm: introduce config option for stdvga emulation
  x86/hvm: make ACPI PM & stdvga emulation optional

 xen/arch/x86/Kconfig               | 23 +++++++++++++++++++++++
 xen/arch/x86/domain.c              |  6 ++++++
 xen/arch/x86/hvm/Makefile          |  4 ++--
 xen/arch/x86/include/asm/acpi.h    |  5 +++++
 xen/arch/x86/include/asm/domain.h  |  6 ++++--
 xen/arch/x86/include/asm/hvm/io.h  |  4 ++++
 xen/arch/x86/include/asm/hvm/vpt.h | 10 ++++++++++
 7 files changed, 54 insertions(+), 4 deletions(-)

-- 
2.25.1

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

[Sergiy Kibrik]

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

[XEN PATCH v1 2/3] x86/hvm: introduce config option for stdvga emulation

[Sergiy Kibrik]

Introduce config option X86_STDVGA so that stdvga 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. No functional changes intended.

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/domain.h | 3 ++-
 xen/arch/x86/include/asm/hvm/io.h | 4 ++++
 4 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 95275dc17e..89c42ff6da 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -147,6 +147,9 @@ config INTEL_VMX
 config X86_PMTIMER
 	def_bool HVM
 
+config X86_STDVGA
+	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 321241f0bf..b7741b0f60 100644
--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -22,7 +22,7 @@ obj-$(CONFIG_X86_PMTIMER) += pmtimer.o
 obj-y += quirks.o
 obj-y += rtc.o
 obj-y += save.o
-obj-y += stdvga.o
+obj-$(CONFIG_X86_STDVGA) += stdvga.o
 obj-y += vioapic.o
 obj-y += vlapic.o
 obj-y += vm_event.o
diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
index 3f65bfd190..675a13d917 100644
--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -501,7 +501,8 @@ struct arch_domain
 #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))
-#define has_vvga(d)        (!!((d)->arch.emulation_flags & X86_EMU_VGA))
+#define has_vvga(d)        (IS_ENABLED(CONFIG_X86_STDVGA) && \
+                            !!((d)->arch.emulation_flags & X86_EMU_VGA))
 #define has_viommu(d)      (!!((d)->arch.emulation_flags & X86_EMU_IOMMU))
 #define has_vpit(d)        (!!((d)->arch.emulation_flags & X86_EMU_PIT))
 #define has_pirq(d)        (!!((d)->arch.emulation_flags & X86_EMU_USE_PIRQ))
diff --git a/xen/arch/x86/include/asm/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index f2b8431fac..32a2490fbc 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -108,7 +108,11 @@ struct vpci_arch_msix_entry {
     int pirq;
 };
 
+#ifdef CONFIG_X86_STDVGA
 void stdvga_init(struct domain *d);
+#else
+static inline void stdvga_init(struct domain *d) {}
+#endif
 
 extern void hvm_dpci_msi_eoi(struct domain *d, int vector);
 
-- 
2.25.1

[XEN PATCH v1 3/3] x86/hvm: make ACPI PM & stdvga emulation optional

[Sergiy Kibrik]

Make it possible to build a configuration without code for PM timer or stdvga
drivers. This may help to reduce code's footprint a bit for systems that don't
run HVM guests and also don't need these devices emulated.

Introduced additional check of domain's emulation flags, to cover the case
when user explicitly states the requirement of emulated devices that are
disabled in the build. HVM always require these devices to be present so domains
of this type can't be created when stdvga or pmtimer are disabled.

Options X86_STDVGA & X86_PMTIMER are dependant on HVM option, because these
drivers are part of HVM support code.

Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
---
 xen/arch/x86/Kconfig  | 21 +++++++++++++++++++--
 xen/arch/x86/domain.c |  6 ++++++
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 89c42ff6da..05551bd722 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -144,11 +144,28 @@ config INTEL_VMX
 	  If your system includes a processor with Intel VT-x support, say Y.
 	  If in doubt, say Y.
 
+menu "Emulated devices support"
+	visible if EXPERT
+	depends on HVM
+
 config X86_PMTIMER
-	def_bool HVM
+	bool "ACPI PM timer emulation support"
+	default y
+	help
+	  Build pmtimer driver that emulates ACPI PM timer for HVM/PVH guests.
+
+	  If unsure, say Y.
 
 config X86_STDVGA
-	def_bool HVM
+	bool "Standard VGA card emulation support"
+	default y
+	help
+	  Build stdvga driver that emulates standard VGA card with VESA BIOS
+	  Extensions for HVM/PVH guests.
+
+	  If unsure, say Y.
+
+endmenu
 
 config XEN_SHSTK
 	bool "Supervisor Shadow Stacks"
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 89aad7e897..32dd3f84cd 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -742,11 +742,17 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
 
 static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
 {
+    const uint32_t disabled_emu_mask =
+        (IS_ENABLED(CONFIG_X86_PMTIMER) ? 0 : X86_EMU_PM) |
+        (IS_ENABLED(CONFIG_X86_STDVGA)  ? 0 : X86_EMU_VGA);
 #ifdef CONFIG_HVM
     /* This doesn't catch !CONFIG_HVM case but it is better than nothing */
     BUILD_BUG_ON(X86_EMU_ALL != XEN_X86_EMU_ALL);
 #endif
 
+    if ( emflags & disabled_emu_mask )
+        return false;
+
     if ( is_hvm_domain(d) )
     {
         if ( is_hardware_domain(d) &&
-- 
2.25.1

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

[Roger Pau Monné]

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.

Re: [XEN PATCH v1 2/3] x86/hvm: introduce config option for stdvga emulation

[Roger Pau Monné]

On Fri, Oct 04, 2024 at 12:33:53PM +0300, Sergiy Kibrik wrote:
> Introduce config option X86_STDVGA so that stdvga 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. No functional changes intended.
> 
> 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/domain.h | 3 ++-
>  xen/arch/x86/include/asm/hvm/io.h | 4 ++++
>  4 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 95275dc17e..89c42ff6da 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -147,6 +147,9 @@ config INTEL_VMX
>  config X86_PMTIMER
>  	def_bool HVM
>  
> +config X86_STDVGA
> +	def_bool HVM

Same as previous patch, the content of patch 3 needs to be moved here.

> +
>  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 321241f0bf..b7741b0f60 100644
> --- a/xen/arch/x86/hvm/Makefile
> +++ b/xen/arch/x86/hvm/Makefile
> @@ -22,7 +22,7 @@ obj-$(CONFIG_X86_PMTIMER) += pmtimer.o
>  obj-y += quirks.o
>  obj-y += rtc.o
>  obj-y += save.o
> -obj-y += stdvga.o
> +obj-$(CONFIG_X86_STDVGA) += stdvga.o
>  obj-y += vioapic.o
>  obj-y += vlapic.o
>  obj-y += vm_event.o
> diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
> index 3f65bfd190..675a13d917 100644
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -501,7 +501,8 @@ struct arch_domain
>  #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))
> -#define has_vvga(d)        (!!((d)->arch.emulation_flags & X86_EMU_VGA))
> +#define has_vvga(d)        (IS_ENABLED(CONFIG_X86_STDVGA) && \

You don't need the IS_ENABLED() if emulation_flags_ok() is adjusted
accordingly.

Thanks, Roger.

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

[Jan Beulich]

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

Re: [XEN PATCH v1 2/3] x86/hvm: introduce config option for stdvga emulation

[Jan Beulich]

On 04.10.2024 11:33, Sergiy Kibrik wrote:
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -147,6 +147,9 @@ config INTEL_VMX
>  config X86_PMTIMER
>  	def_bool HVM
>  
> +config X86_STDVGA
> +	def_bool HVM

For both if these I'd also question the naming: They way they're named now,
I for one would infer they're about Xen support for respective hardware. I
think they both want to be X86_HVM_... to indicate they're about support for
HVM guests. The same concern then applies to the menu title in patch 3.

Jan

Re: [XEN PATCH v1 3/3] x86/hvm: make ACPI PM & stdvga emulation optional

[Jan Beulich]

On 04.10.2024 11:35, Sergiy Kibrik wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -742,11 +742,17 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>  
>  static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
>  {
> +    const uint32_t disabled_emu_mask =
> +        (IS_ENABLED(CONFIG_X86_PMTIMER) ? 0 : X86_EMU_PM) |
> +        (IS_ENABLED(CONFIG_X86_STDVGA)  ? 0 : X86_EMU_VGA);
>  #ifdef CONFIG_HVM
>      /* This doesn't catch !CONFIG_HVM case but it is better than nothing */
>      BUILD_BUG_ON(X86_EMU_ALL != XEN_X86_EMU_ALL);

Nit: Blank line between declaration(s) and statement(s) please.

Jan

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

[Stefano Stabellini]

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

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.

Re: [XEN PATCH v1 2/3] x86/hvm: introduce config option for stdvga emulation

[Stefano Stabellini]

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

On Fri, 4 Oct 2024, Roger Pau Monné wrote:
> On Fri, Oct 04, 2024 at 12:33:53PM +0300, Sergiy Kibrik wrote:
> > Introduce config option X86_STDVGA so that stdvga 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. No functional changes intended.
> > 
> > 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/domain.h | 3 ++-
> >  xen/arch/x86/include/asm/hvm/io.h | 4 ++++
> >  4 files changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> > index 95275dc17e..89c42ff6da 100644
> > --- a/xen/arch/x86/Kconfig
> > +++ b/xen/arch/x86/Kconfig
> > @@ -147,6 +147,9 @@ config INTEL_VMX
> >  config X86_PMTIMER
> >  	def_bool HVM
> >  
> > +config X86_STDVGA
> > +	def_bool HVM
> 
> Same as previous patch, the content of patch 3 needs to be moved here.
> 
> > +
> >  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 321241f0bf..b7741b0f60 100644
> > --- a/xen/arch/x86/hvm/Makefile
> > +++ b/xen/arch/x86/hvm/Makefile
> > @@ -22,7 +22,7 @@ obj-$(CONFIG_X86_PMTIMER) += pmtimer.o
> >  obj-y += quirks.o
> >  obj-y += rtc.o
> >  obj-y += save.o
> > -obj-y += stdvga.o
> > +obj-$(CONFIG_X86_STDVGA) += stdvga.o
> >  obj-y += vioapic.o
> >  obj-y += vlapic.o
> >  obj-y += vm_event.o
> > diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
> > index 3f65bfd190..675a13d917 100644
> > --- a/xen/arch/x86/include/asm/domain.h
> > +++ b/xen/arch/x86/include/asm/domain.h
> > @@ -501,7 +501,8 @@ struct arch_domain
> >  #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))
> > -#define has_vvga(d)        (!!((d)->arch.emulation_flags & X86_EMU_VGA))
> > +#define has_vvga(d)        (IS_ENABLED(CONFIG_X86_STDVGA) && \
> 
> You don't need the IS_ENABLED() if emulation_flags_ok() is adjusted
> accordingly.

Same as patch #1 regarding compiler DCE, we could either do this or
define X86_EMU_VGA to zero

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

[Roger Pau Monné]

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.

Re: [XEN PATCH v1 2/3] x86/hvm: introduce config option for stdvga emulation

[Roger Pau Monné]

On Fri, Oct 04, 2024 at 02:08:10PM -0700, Stefano Stabellini wrote:
> On Fri, 4 Oct 2024, Roger Pau Monné wrote:
> > On Fri, Oct 04, 2024 at 12:33:53PM +0300, Sergiy Kibrik wrote:
> > > Introduce config option X86_STDVGA so that stdvga 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. No functional changes intended.
> > > 
> > > 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/domain.h | 3 ++-
> > >  xen/arch/x86/include/asm/hvm/io.h | 4 ++++
> > >  4 files changed, 10 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> > > index 95275dc17e..89c42ff6da 100644
> > > --- a/xen/arch/x86/Kconfig
> > > +++ b/xen/arch/x86/Kconfig
> > > @@ -147,6 +147,9 @@ config INTEL_VMX
> > >  config X86_PMTIMER
> > >  	def_bool HVM
> > >  
> > > +config X86_STDVGA
> > > +	def_bool HVM
> > 
> > Same as previous patch, the content of patch 3 needs to be moved here.
> > 
> > > +
> > >  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 321241f0bf..b7741b0f60 100644
> > > --- a/xen/arch/x86/hvm/Makefile
> > > +++ b/xen/arch/x86/hvm/Makefile
> > > @@ -22,7 +22,7 @@ obj-$(CONFIG_X86_PMTIMER) += pmtimer.o
> > >  obj-y += quirks.o
> > >  obj-y += rtc.o
> > >  obj-y += save.o
> > > -obj-y += stdvga.o
> > > +obj-$(CONFIG_X86_STDVGA) += stdvga.o
> > >  obj-y += vioapic.o
> > >  obj-y += vlapic.o
> > >  obj-y += vm_event.o
> > > diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
> > > index 3f65bfd190..675a13d917 100644
> > > --- a/xen/arch/x86/include/asm/domain.h
> > > +++ b/xen/arch/x86/include/asm/domain.h
> > > @@ -501,7 +501,8 @@ struct arch_domain
> > >  #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))
> > > -#define has_vvga(d)        (!!((d)->arch.emulation_flags & X86_EMU_VGA))
> > > +#define has_vvga(d)        (IS_ENABLED(CONFIG_X86_STDVGA) && \
> > 
> > You don't need the IS_ENABLED() if emulation_flags_ok() is adjusted
> > accordingly.
> 
> Same as patch #1 regarding compiler DCE, we could either do this or
> define X86_EMU_VGA to zero

Yup, seen that.  Defining iX86_EMU_VGA to 0 would be my preference,
like it's done for the !CONFIG_HVM case.

Note however, that like has_vpm(), has_vvga() is only used in the file
that this patch makes optional from the build.

Thanks, Roger.

Re: [XEN PATCH v1 2/3] x86/hvm: introduce config option for stdvga emulation

[Stefano Stabellini]

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

On Mon, 7 Oct 2024, Roger Pau Monné wrote:
> On Fri, Oct 04, 2024 at 02:08:10PM -0700, Stefano Stabellini wrote:
> > On Fri, 4 Oct 2024, Roger Pau Monné wrote:
> > > On Fri, Oct 04, 2024 at 12:33:53PM +0300, Sergiy Kibrik wrote:
> > > > Introduce config option X86_STDVGA so that stdvga 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. No functional changes intended.
> > > > 
> > > > 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/domain.h | 3 ++-
> > > >  xen/arch/x86/include/asm/hvm/io.h | 4 ++++
> > > >  4 files changed, 10 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> > > > index 95275dc17e..89c42ff6da 100644
> > > > --- a/xen/arch/x86/Kconfig
> > > > +++ b/xen/arch/x86/Kconfig
> > > > @@ -147,6 +147,9 @@ config INTEL_VMX
> > > >  config X86_PMTIMER
> > > >  	def_bool HVM
> > > >  
> > > > +config X86_STDVGA
> > > > +	def_bool HVM
> > > 
> > > Same as previous patch, the content of patch 3 needs to be moved here.
> > > 
> > > > +
> > > >  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 321241f0bf..b7741b0f60 100644
> > > > --- a/xen/arch/x86/hvm/Makefile
> > > > +++ b/xen/arch/x86/hvm/Makefile
> > > > @@ -22,7 +22,7 @@ obj-$(CONFIG_X86_PMTIMER) += pmtimer.o
> > > >  obj-y += quirks.o
> > > >  obj-y += rtc.o
> > > >  obj-y += save.o
> > > > -obj-y += stdvga.o
> > > > +obj-$(CONFIG_X86_STDVGA) += stdvga.o
> > > >  obj-y += vioapic.o
> > > >  obj-y += vlapic.o
> > > >  obj-y += vm_event.o
> > > > diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
> > > > index 3f65bfd190..675a13d917 100644
> > > > --- a/xen/arch/x86/include/asm/domain.h
> > > > +++ b/xen/arch/x86/include/asm/domain.h
> > > > @@ -501,7 +501,8 @@ struct arch_domain
> > > >  #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))
> > > > -#define has_vvga(d)        (!!((d)->arch.emulation_flags & X86_EMU_VGA))
> > > > +#define has_vvga(d)        (IS_ENABLED(CONFIG_X86_STDVGA) && \
> > > 
> > > You don't need the IS_ENABLED() if emulation_flags_ok() is adjusted
> > > accordingly.
> > 
> > Same as patch #1 regarding compiler DCE, we could either do this or
> > define X86_EMU_VGA to zero
> 
> Yup, seen that.  Defining iX86_EMU_VGA to 0 would be my preference,
> like it's done for the !CONFIG_HVM case.

OK, in that case I suggest we go with defining X86_EMU_VGA and
X86_EMU_PM to 0


> Note however, that like has_vpm(), has_vvga() is only used in the file
> that this patch makes optional from the build.

Yeah but it seems more robust not to rely on that