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

["Roger Pau Monné" <roger.pau@citrix.com>]

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.