Re: [PATCH v1 2/3] x86/domain: add helpers to simplify emulation flags management

[dmkhn@xxxxxxxxx]

On Tue, Apr 08, 2025 at 05:23:17PM +0200, Jan Beulich wrote:
> On 01.04.2025 02:52, dmkhn@xxxxxxxxx wrote:
> > From: Denis Mukhin <dmukhin@xxxxxxxx>
> >
> > Introduce XEN_X86_EMU_BASELINE and XEN_X86_EMU_OPTIONAL to simplify
> > d->arch.emulation_flags management in the code.
> 
> If the simplification is limited to ...
> 
> > --- a/tools/python/xen/lowlevel/xc/xc.c
> > +++ b/tools/python/xen/lowlevel/xc/xc.c
> > @@ -159,9 +159,7 @@ static PyObject *pyxc_domain_create(XcObject *self,
> >
> >  #if defined (__i386) || defined(__x86_64__)
> >      if ( config.flags & XEN_DOMCTL_CDF_hvm )
> > -        config.arch.emulation_flags = XEN_X86_EMU_ALL &
> > -                                      ~(XEN_X86_EMU_VPCI |
> > -                                        XEN_X86_EMU_USE_PIRQ);
> > +        config.arch.emulation_flags = XEN_X86_EMU_BASELINE;
> >  #elif defined (__arm__) || defined(__aarch64__)
> >      config.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
> >  #else
> 
> ... just this, I'm not convinced that's worth introducing yet two more items
> into the namespace.

Sorry, it took me a while to get back to the series.

The X86_EMU_BASELINE mask is used in two places: in the toolstack (xc.c) and
in domain.c, emulation_flags_ok(), so adding a new flag may require updates
in two places.

Perhaps it will be better to merge this patch 2 with patch 3 to highlight that? 

> 
> > --- a/xen/include/public/arch-x86/xen.h
> > +++ b/xen/include/public/arch-x86/xen.h
> > @@ -290,6 +290,13 @@ struct xen_arch_domainconfig {
> >                                       XEN_X86_EMU_VGA | XEN_X86_EMU_IOMMU | 
> >   \
> >                                       XEN_X86_EMU_PIT | 
> > XEN_X86_EMU_USE_PIRQ |\
> >                                       XEN_X86_EMU_VPCI)
> > +
> > +#define XEN_X86_EMU_OPTIONAL        (XEN_X86_EMU_VPCI | \
> > +                                     XEN_X86_EMU_USE_PIRQ)
> > +
> > +#define XEN_X86_EMU_BASELINE        (XEN_X86_EMU_ALL & 
> > ~XEN_X86_EMU_OPTIONAL)
> > +
> > +    /* Hardware emulation flags. */
> >      uint32_t emulation_flags;
> 
> The comment isn't quite accurate here (and hence perhaps also not in the
> earlier patch): XEN_X86_EMU_USE_PIRQ isn't exactly about emulation of anything
> hardware-ish.
> 
> Jan