Re: [XEN PATCH v1 2/3] x86/hvm: introduce config option for stdvga emulation

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 7 Oct 2024, Roger Pau Monné wrote:
> On Fri, Oct 04, 2024 at 02:08:10PM -0700, Stefano Stabellini wrote:
> > On Fri, 4 Oct 2024, Roger Pau Monné wrote:
> > > On Fri, Oct 04, 2024 at 12:33:53PM +0300, Sergiy Kibrik wrote:
> > > > Introduce config option X86_STDVGA so that stdvga emulation driver can 
> > > > later be
> > > > made configurable and be disabled on systems that don't need it.
> > > > 
> > > > As a first step the option is hidden from user. No functional changes 
> > > > intended.
> > > > 
> > > > Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@xxxxxxxx>
> > > > CC: Jan Beulich <jbeulich@xxxxxxxx>
> > > > ---
> > > >  xen/arch/x86/Kconfig              | 3 +++
> > > >  xen/arch/x86/hvm/Makefile         | 2 +-
> > > >  xen/arch/x86/include/asm/domain.h | 3 ++-
> > > >  xen/arch/x86/include/asm/hvm/io.h | 4 ++++
> > > >  4 files changed, 10 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> > > > index 95275dc17e..89c42ff6da 100644
> > > > --- a/xen/arch/x86/Kconfig
> > > > +++ b/xen/arch/x86/Kconfig
> > > > @@ -147,6 +147,9 @@ config INTEL_VMX
> > > >  config X86_PMTIMER
> > > >         def_bool HVM
> > > >  
> > > > +config X86_STDVGA
> > > > +       def_bool HVM
> > > 
> > > Same as previous patch, the content of patch 3 needs to be moved here.
> > > 
> > > > +
> > > >  config XEN_SHSTK
> > > >         bool "Supervisor Shadow Stacks"
> > > >         depends on HAS_AS_CET_SS
> > > > diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
> > > > index 321241f0bf..b7741b0f60 100644
> > > > --- a/xen/arch/x86/hvm/Makefile
> > > > +++ b/xen/arch/x86/hvm/Makefile
> > > > @@ -22,7 +22,7 @@ obj-$(CONFIG_X86_PMTIMER) += pmtimer.o
> > > >  obj-y += quirks.o
> > > >  obj-y += rtc.o
> > > >  obj-y += save.o
> > > > -obj-y += stdvga.o
> > > > +obj-$(CONFIG_X86_STDVGA) += stdvga.o
> > > >  obj-y += vioapic.o
> > > >  obj-y += vlapic.o
> > > >  obj-y += vm_event.o
> > > > diff --git a/xen/arch/x86/include/asm/domain.h 
> > > > b/xen/arch/x86/include/asm/domain.h
> > > > index 3f65bfd190..675a13d917 100644
> > > > --- a/xen/arch/x86/include/asm/domain.h
> > > > +++ b/xen/arch/x86/include/asm/domain.h
> > > > @@ -501,7 +501,8 @@ struct arch_domain
> > > >  #define has_vrtc(d)        (!!((d)->arch.emulation_flags & 
> > > > X86_EMU_RTC))
> > > >  #define has_vioapic(d)     (!!((d)->arch.emulation_flags & 
> > > > X86_EMU_IOAPIC))
> > > >  #define has_vpic(d)        (!!((d)->arch.emulation_flags & 
> > > > X86_EMU_PIC))
> > > > -#define has_vvga(d)        (!!((d)->arch.emulation_flags & 
> > > > X86_EMU_VGA))
> > > > +#define has_vvga(d)        (IS_ENABLED(CONFIG_X86_STDVGA) && \
> > > 
> > > You don't need the IS_ENABLED() if emulation_flags_ok() is adjusted
> > > accordingly.
> > 
> > Same as patch #1 regarding compiler DCE, we could either do this or
> > define X86_EMU_VGA to zero
> 
> Yup, seen that.  Defining iX86_EMU_VGA to 0 would be my preference,
> like it's done for the !CONFIG_HVM case.

OK, in that case I suggest we go with defining X86_EMU_VGA and
X86_EMU_PM to 0


> Note however, that like has_vpm(), has_vvga() is only used in the file
> that this patch makes optional from the build.

Yeah but it seems more robust not to rely on that