Re: [PATCH v2 33/35] x86/domain: implement domain_has_vuart()

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

On Fri, Dec 13, 2024 at 12:45:34PM -0800, Stefano Stabellini wrote:
> On Fri, 13 Dec 2024, Roger Pau Monné wrote:
> > On Thu, Dec 05, 2024 at 08:42:03PM -0800, Denis Mukhin via B4 Relay wrote:
> > > From: Denis Mukhin <dmukhin@xxxxxxxx>
> > > 
> > > Add new emulation flag for virtual UART on x86 and plumb it through the 
> > > stack.
> > > 
> > > This change enables NS8250 emulator initialization.
> > > 
> > > Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> > > ---
> > >  tools/libs/light/libxl_x86.c      |  6 +++++-
> > >  tools/ocaml/libs/xc/xenctrl.ml    |  1 +
> > >  tools/ocaml/libs/xc/xenctrl.mli   |  1 +
> > >  tools/python/xen/lowlevel/xc/xc.c |  4 +---
> > >  xen/arch/x86/domain.c             |  8 +++++---
> > >  xen/arch/x86/include/asm/domain.h |  7 ++++---
> > >  xen/include/public/arch-x86/xen.h | 14 +++++++++++++-
> > >  7 files changed, 30 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
> > > index 
> > > a3164a3077fec7e1b81a34074894dc646954a49a..de5f05e18cb0671bb031b101b9a7159eb0fe0178
> > >  100644
> > > --- a/tools/libs/light/libxl_x86.c
> > > +++ b/tools/libs/light/libxl_x86.c
> > > @@ -8,7 +8,11 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> > >  {
> > >      switch(d_config->c_info.type) {
> > >      case LIBXL_DOMAIN_TYPE_HVM:
> > > -        config->arch.emulation_flags = (XEN_X86_EMU_ALL & 
> > > ~XEN_X86_EMU_VPCI);
> > > +        config->arch.emulation_flags = XEN_X86_EMU_ALL;
> > > +        config->arch.emulation_flags &= ~XEN_X86_EMU_VPCI;
> > > +        /* Virtual UART is selected at Xen build time */
> > > +        config->arch.emulation_flags &= ~XEN_X86_EMU_VUART;
> > > +
> > >          if (!libxl_defbool_val(d_config->b_info.u.hvm.pirq))
> > >              config->arch.emulation_flags &= ~XEN_X86_EMU_USE_PIRQ;
> > >          break;
> > > diff --git a/tools/ocaml/libs/xc/xenctrl.ml 
> > > b/tools/ocaml/libs/xc/xenctrl.ml
> > > index 
> > > 2690f9a92316b812ad3d3ff0e1c36823070adb4a..647239b3e55e88b00eb8e9773a5267894cbbae54
> > >  100644
> > > --- a/tools/ocaml/libs/xc/xenctrl.ml
> > > +++ b/tools/ocaml/libs/xc/xenctrl.ml
> > > @@ -47,6 +47,7 @@ type x86_arch_emulation_flags =
> > >    | X86_EMU_PIT
> > >    | X86_EMU_USE_PIRQ
> > >    | X86_EMU_VPCI
> > > +  | X86_EMU_VUART
> > >  
> > >  type x86_arch_misc_flags =
> > >    | X86_MSR_RELAXED
> > > diff --git a/tools/ocaml/libs/xc/xenctrl.mli 
> > > b/tools/ocaml/libs/xc/xenctrl.mli
> > > index 
> > > febbe1f6ae3f10c5abe45eaa3c06a8a67d9ba268..4f5f64c786e83e8a0c3dd3cdb0460f7095de4a62
> > >  100644
> > > --- a/tools/ocaml/libs/xc/xenctrl.mli
> > > +++ b/tools/ocaml/libs/xc/xenctrl.mli
> > > @@ -41,6 +41,7 @@ type x86_arch_emulation_flags =
> > >    | X86_EMU_PIT
> > >    | X86_EMU_USE_PIRQ
> > >    | X86_EMU_VPCI
> > > +  | X86_EMU_VUART
> > >  
> > >  type x86_arch_misc_flags =
> > >    | X86_MSR_RELAXED
> > > diff --git a/tools/python/xen/lowlevel/xc/xc.c 
> > > b/tools/python/xen/lowlevel/xc/xc.c
> > > index 
> > > 9feb12ae2b16e48cb5d0c3c45044ae226f152f2d..e54308956efc7061d58d2166ec9a95bc1dcd1781
> > >  100644
> > > --- 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_HVM_ALLOWABLE;
> > >  #elif defined (__arm__) || defined(__aarch64__)
> > >      config.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
> > >  #else
> > > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > > index 
> > > c88d422a64544531c1e1058fa484364bb4277d1e..439da7adc92a3a8eb481075bf834da5f9670dd54
> > >  100644
> > > --- a/xen/arch/x86/domain.c
> > > +++ b/xen/arch/x86/domain.c
> > > @@ -752,10 +752,10 @@ static bool emulation_flags_ok(const struct domain 
> > > *d, uint32_t emflags)
> > >          if ( is_hardware_domain(d) &&
> > >               emflags != (X86_EMU_VPCI | X86_EMU_LAPIC | X86_EMU_IOAPIC) )
> > >              return false;
> > > +
> > > +        emflags &= ~X86_EMU_VUART;
> > 
> > I think you want to allow X86_EMU_VUART only for domains created by
> > Xen itself, so X86_EMU_VUART can only be valid if system_state <
> > SYS_STATE_active.
> > 
> > >          if ( !is_hardware_domain(d) &&
> > > -             /* HVM PIRQ feature is user-selectable. */
> > > -             (emflags & ~X86_EMU_USE_PIRQ) !=
> > > -             (X86_EMU_ALL & ~(X86_EMU_VPCI | X86_EMU_USE_PIRQ)) &&
> > > +             xen_emflags_allowable(emflags) != XEN_X86_EMU_HVM_ALLOWABLE 
> > > &&
> > >               emflags != X86_EMU_LAPIC )
> > >              return false;
> > >      }
> > > @@ -806,6 +806,8 @@ int arch_domain_create(struct domain *d,
> > >  
> > >      emflags = config->arch.emulation_flags;
> > >  
> > > +    if ( IS_ENABLED(CONFIG_HAS_VUART_NS8250) && is_hvm_domain(d) )
> > > +        emflags |= XEN_X86_EMU_VUART;
> > 
> > Doesn't this need to be limited to domains created by Xen itself, as
> > otherwise it's the toolstack that should specify the XEN_X86_EMU_VUART
> > flag, and even then the recommendation would be to use the vUART from
> > QEMU?
> 
> While I agree with you that this feature is really useful mostly for the
> domains created by Xen, as for those there is no other way to get early
> output, I think Denis has been also testing successfully this feature
> with PVH or HVM domains created by the toolstack.
> 
> I'll let you decide whether you want to expose the feature to xl created
> domains, but yes my understanding is that they already work with this
> series. One benefit would be that for PVH domains you could get early
> output without having to start QEMU, but I'll leave this to you.

I'm not opposed to allowing usage of the Xen emulated uart for
toolstack created domains, but then the option needs to be integrated
with xl/libxl IMO, so that it can be specified in the xl.cfg file, and
propagated from the toolstack into Xen using the emulation_flags
field.  Just like all other emulated devices that are controlled by
emulation_flags.

Thanks, Roger.