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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.