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....@citrix.com>
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
