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

[Sergiy Kibrik <sergiy_kibrik@xxxxxxxx>]

11.11.24 11:51, Jan Beulich:
> 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.
about a half of this patch is what Roger suggested. These changes were 
in a separate patch, which Roger suggested to be merged into other 
patches. What tag should be put in this case then?
> > --- 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?
PVH guest may (depending on its configuration) still use PM timer, so 
I'd say the point is in a PVH-only environment this driver becomes optional.
> I wonder how meaningful "pmtimer" is to someone reading this help test in
> isolation. I'd just drop the word.
sure, the word is rarely mentioned anywhere, I'll remove it.

> > --- 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.
This #ifdef definitely wants nicer change. How would you suggest 
BUILD_BUG_ON() be split?
  -Sergiy