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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.11.2024 11:14, Sergiy Kibrik wrote:
> Introduce config option X86_HVM_PMTIMER and make pmtimer emulation driver
> configurable and possible to disable on systems that don't need it.
> Option X86_X86_HVM_PMTIMER depends on HVM option, because this driver is part
> of HVM support code.
> 
> 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 pmtimer or any other emulated device are
> disabled.
> 
> Suggested-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

What exactly was it that Roger suggested? I don't think it was what the patch
does overall, but just _how_ it is being done? That makes quite a bit of a
difference, as the former could be read as kind of an implicit ack to what is
being done here (and also in the other patch). Issue is: I remain unconvinced
that this conditionalizing is actually something we really want/need.

> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -144,6 +144,19 @@ config INTEL_VMX
>         If your system includes a processor with Intel VT-x support, say Y.
>         If in doubt, say Y.
>  
> +menu "Emulated HVM devices support"
> +       visible if EXPERT
> +       depends on HVM
> +
> +config X86_HVM_PMTIMER
> +     bool "ACPI PM timer emulation support"
> +     default y
> +     help
> +       Build pmtimer driver that emulates ACPI PM timer for HVM/PVH guests.

Does this really affect PVH guests? Isn't the whole point of the change
that in a PVH-only environment this wouldn't be needed in Xen?

I wonder how meaningful "pmtimer" is to someone reading this help test in
isolation. I'd just drop the word.

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -742,11 +742,16 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>  
>  static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
>  {
> -#ifdef CONFIG_HVM
> +    const uint32_t disabled_emu_mask = X86_EMU_PM;
> +
> +#if defined(CONFIG_X86_HVM_PMTIMER)
>      /* 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) &&

While you commented on this hunk, it didn't become clear what exactly the
resulting new hunk would be. I question in particular the change to the
#ifdef: If that's changed and the BUILD_BUG_ON() kept as is, the comment
also needs adjusting. Yet it would perhaps be better of the BUILD_BUG_ON()
was split accordingly.

Jan