Re: [PATCH v4 6/8] tools/xl: enable NS16550-compatible UART emulator for HVM (x86)

[dmkhn@xxxxxxxxx]

On Mon, Aug 25, 2025 at 04:49:58PM +0200, Anthony PERARD wrote:
> On Thu, Jul 31, 2025 at 07:22:12PM +0000, dmkhn@xxxxxxxxx wrote:
> > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> > index 5362fb0e9a6f..e1d012274eaf 100644
> > --- a/docs/man/xl.cfg.5.pod.in
> > +++ b/docs/man/xl.cfg.5.pod.in
> > @@ -3032,14 +3032,17 @@ the domain was created.
> >  This requires hardware compatibility with the requested version, either
> >  natively or via hardware backwards compatibility support.
> >
> > -=item B<vuart="uart">
> > +=item B<vuart=[ "sbsa_uart", "ns16550" ]>
> 
> This syntax here would inditace that `vuart` takes a list of items. You
> could write instead:
> 
>     vuart="UART"
> 
> which seems more in line with the rest of the man page. Then you can add
> some thing like "with UART been one of "sbsa_uart" or "ns16550". It's
> possible to also have a sublist, like the `tee` option have.

OK, will do.

> 
> 
> >  To enable vuart console, user must specify the following option in the
> > -VM config file:
> > +VM config file, e.g:
> >
> > +```
> 
> This file isn't in markdown, it's in perlpod.

Whoops, muscle memory. Thanks.

> 
> >  vuart = "sbsa_uart"
> > +```
> >
> > -Currently, only the "sbsa_uart" model is supported for ARM.
> > +Currently, "sbsa_uart" (ARM) and "ns16550" (x86) are the only supported
> > +UART models.
> >
> >  =back
> >
> > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> > index 4a19a8d22bdf..f4721b24763c 100644
> > --- a/tools/libs/light/libxl_arm.c
> > +++ b/tools/libs/light/libxl_arm.c
> > @@ -92,14 +92,26 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> >      uint32_t virtio_mmio_irq = GUEST_VIRTIO_MMIO_SPI_FIRST;
> >      int rc;
> >
> > -    /*
> > -     * If pl011 vuart is enabled then increment the nr_spis to allow 
> > allocation
> > -     * of SPI VIRQ for pl011.
> > -     */
> > -    if (d_config->b_info.arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART) {
> > +    switch ( d_config->b_info.vuart )
> > +    {
> > +    case LIBXL_VUART_TYPE_SBSA_UART:
> > +        /*
> > +         * If pl011 vuart is enabled then increment the nr_spis to allow
> > +         * allocation of SPI VIRQ for pl011.
> > +         */
> >          nr_spis += (GUEST_VPL011_SPI - 32) + 1;
> >          vuart_irq = GUEST_VPL011_SPI;
> >          vuart_enabled = true;
> > +        break;
> > +
> > +    case LIBXL_VUART_TYPE_NS16550:
> > +        LOG(ERROR, "unsupported UART emulator %d\n", 
> > d_config->b_info.vuart);
> 
> This seems too late in libxl.  I think checking if the config value is
> correct could be done in one of the *_setdefault() like many other
> config check are done. There's
> libxl__arch_domain_build_info_setdefault() that could be used.

Thanks for the pointer.

> 
> > +        abort();
> > +        break;
> > +
> > +    case LIBXL_VUART_TYPE_UNKNOWN:
> > +    default:
> > +        break;
> >      }
> >
> >      for (i = 0; i < d_config->num_disks; i++) {
> > diff --git a/tools/libs/light/libxl_types.idl 
> > b/tools/libs/light/libxl_types.idl
> > index fe251649f346..fd60c2b26764 100644
> > --- a/tools/libs/light/libxl_types.idl
> > +++ b/tools/libs/light/libxl_types.idl
> > @@ -276,6 +276,7 @@ libxl_checkpointed_stream = 
> > Enumeration("checkpointed_stream", [
> >  libxl_vuart_type = Enumeration("vuart_type", [
> >      (0, "unknown"),
> >      (1, "sbsa_uart"),
> > +    (2, "ns16550"),
> >      ])
> >
> >  libxl_vkb_backend = Enumeration("vkb_backend", [
> > @@ -722,7 +723,6 @@ libxl_domain_build_info = Struct("domain_build_info",[
> >
> >
> >      ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
> > -                               ("vuart", libxl_vuart_type),
> 
> arch_arm.vuart is part of libxl's API, it can't be removed. There's some
> explanation about "libxl API compatibility" at the top of "libxl.h".
> But for this change, you could add `vuart` to `arch_x86`, or if you want
> to add `vuart` at the root like you did, you'll need to check that both
> `arch_arm.vuart` and `vuart` aren't set at the same time, and have one
> of the *_setdefault() function do the work of migrating the option.
> 
> You'll need also a LIBXL_HAVE_* macro in libxl.h, probably named
> LIBXL_HAVE_VUART_NS16550.

Thanks for details, I will proceed with option 2, adding `vuart` at the root.

> 
> >                                 ("sve_vl", libxl_sve_type),
> >                                 ("nr_spis", uint32, {'init_val': 
> > 'LIBXL_NR_SPIS_DEFAULT'}),
> >                                ])),
> > @@ -739,6 +739,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> >
> >      ("vpmu", libxl_defbool),
> >      ("trap_unmapped_accesses", libxl_defbool),
> > +    ("vuart", libxl_vuart_type),
> >
> >      ], dir=DIR_IN,
> >         copy_deprecated_fn="libxl__domain_build_info_copy_deprecated",
> > diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
> > index 60d4e8661c93..0f039ca65a88 100644
> > --- a/tools/libs/light/libxl_x86.c
> > +++ b/tools/libs/light/libxl_x86.c
> > @@ -2,6 +2,45 @@
> >  #include "libxl_arch.h"
> >  #include <xen/arch-x86/cpuid.h>
> >
> > +static void libxl__arch_domain_vuart_assert(
> > +    libxl__gc *gc,
> > +    libxl_domain_config *d_config,
> > +    struct xen_domctl_createdomain *config)
> > +{
> > +    LOG(ERROR, "unsupported UART emulator %d\n", d_config->b_info.vuart);
> > +    abort();
> 
> The name of the function is wrong. It doens't assert anything, and just
> abort...
> I don't think this function is useful.

Yeah, I mostly added it to avoid typing the same error message several times. 
Will remove.

> 
> Also, don't abort() for configuration error, you need to return an error
> instead.

Ack.

> 
> > +}
> > +
> > +static void libxl__arch_domain_vuart_unsupported(
> > +    libxl__gc *gc,
> > +    libxl_domain_config *d_config,
> > +    struct xen_domctl_createdomain *config)
> > +{
> > +    if ( d_config->b_info.vuart != LIBXL_VUART_TYPE_UNKNOWN )
> > +        libxl__arch_domain_vuart_assert(gc, d_config, config);
> 
> This function have also a bad name, it doesn't check if a uart is
> unsupported.

Will rework.

> 
> > +}
> > +
> > +static void libxl__arch_domain_vuart_enable(
> > +    libxl__gc *gc,
> > +    libxl_domain_config *d_config,
> > +    struct xen_domctl_createdomain *config)
> > +{
> > +    switch ( d_config->b_info.vuart )
> > +    {
> > +    case LIBXL_VUART_TYPE_SBSA_UART:
> > +        libxl__arch_domain_vuart_assert(gc, d_config, config);
> > +        break;
> > +
> > +    case LIBXL_VUART_TYPE_NS16550:
> > +        config->arch.emulation_flags |= XEN_X86_EMU_NS16550;
> > +        break;
> > +
> > +    case LIBXL_VUART_TYPE_UNKNOWN:
> > +    default:
> > +        break;
> > +    }
> > +}
> > +
> >  int libxl__arch_domain_prepare_config(libxl__gc *gc,
> >                                        libxl_domain_config *d_config,
> >                                        struct xen_domctl_createdomain 
> > *config)
> > @@ -9,14 +48,17 @@ 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);
> > +        libxl__arch_domain_vuart_enable(gc, d_config, config);
> >          if (!libxl_defbool_val(d_config->b_info.u.hvm.pirq))
> >              config->arch.emulation_flags &= ~XEN_X86_EMU_USE_PIRQ;
> >          break;
> >      case LIBXL_DOMAIN_TYPE_PVH:
> >          config->arch.emulation_flags = XEN_X86_EMU_LAPIC;
> > +        libxl__arch_domain_vuart_unsupported(gc, d_config, config);
> >          break;
> >      case LIBXL_DOMAIN_TYPE_PV:
> >          config->arch.emulation_flags = 0;
> > +        libxl__arch_domain_vuart_unsupported(gc, d_config, config);
> >          break;
> >      default:
> >          abort();
> 
> Thanks,

Thanks for review!

--
Denis

> 
> --
> Anthony PERARD