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

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

On Thu, Jul 31, 2025 at 07:22:12PM +0000, dmkhn@xxxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx> 
> 
> Enable UART emulator to be individually configured per HVM-domain.
> 
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
> Changes since v3:
> - new patch
> ---
>  docs/man/xl.cfg.5.pod.in             |  9 ++++--
>  tools/golang/xenlight/helpers.gen.go |  4 +--
>  tools/golang/xenlight/types.gen.go   |  3 +-
>  tools/libs/light/libxl_arm.c         | 26 ++++++++++++-----
>  tools/libs/light/libxl_create.c      |  2 +-
>  tools/libs/light/libxl_types.idl     |  3 +-
>  tools/libs/light/libxl_x86.c         | 42 ++++++++++++++++++++++++++++
>  tools/ocaml/libs/xc/xenctrl.ml       |  1 +
>  tools/ocaml/libs/xc/xenctrl.mli      |  1 +
>  tools/xl/xl_parse.c                  |  2 +-
>  xen/arch/x86/domain.c                |  5 ++--
>  11 files changed, 80 insertions(+), 18 deletions(-)
> 
> 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" ]>
>  
>  To enable vuart console, user must specify the following option in the
> -VM config file:
> +VM config file, e.g:
>  
> +```
>  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/golang/xenlight/helpers.gen.go 
> b/tools/golang/xenlight/helpers.gen.go
> index b43aad7d0064..e56af8a8a8c5 100644
> --- a/tools/golang/xenlight/helpers.gen.go
> +++ b/tools/golang/xenlight/helpers.gen.go
> @@ -1160,7 +1160,6 @@ x.TypeUnion = &typePvh
>  default:
>  return fmt.Errorf("invalid union key '%v'", x.Type)}
>  x.ArchArm.GicVersion = GicVersion(xc.arch_arm.gic_version)
> -x.ArchArm.Vuart = VuartType(xc.arch_arm.vuart)
>  x.ArchArm.SveVl = SveType(xc.arch_arm.sve_vl)
>  x.ArchArm.NrSpis = uint32(xc.arch_arm.nr_spis)
>  if err := x.ArchX86.MsrRelaxed.fromC(&xc.arch_x86.msr_relaxed);err != nil {
> @@ -1169,6 +1168,7 @@ return fmt.Errorf("converting field ArchX86.MsrRelaxed: 
> %v", err)
>  x.Altp2M = Altp2MMode(xc.altp2m)
>  x.Altp2MCount = uint32(xc.altp2m_count)
>  x.VmtraceBufKb = int(xc.vmtrace_buf_kb)
> +x.Vuart = VuartType(xc.vuart)
>  if err := x.Vpmu.fromC(&xc.vpmu);err != nil {
>  return fmt.Errorf("converting field Vpmu: %v", err)
>  }
> @@ -1695,7 +1695,6 @@ break
>  default:
>  return fmt.Errorf("invalid union key '%v'", x.Type)}
>  xc.arch_arm.gic_version = C.libxl_gic_version(x.ArchArm.GicVersion)
> -xc.arch_arm.vuart = C.libxl_vuart_type(x.ArchArm.Vuart)
>  xc.arch_arm.sve_vl = C.libxl_sve_type(x.ArchArm.SveVl)
>  xc.arch_arm.nr_spis = C.uint32_t(x.ArchArm.NrSpis)
>  if err := x.ArchX86.MsrRelaxed.toC(&xc.arch_x86.msr_relaxed); err != nil {
> @@ -1704,6 +1703,7 @@ return fmt.Errorf("converting field ArchX86.MsrRelaxed: 
> %v", err)
>  xc.altp2m = C.libxl_altp2m_mode(x.Altp2M)
>  xc.altp2m_count = C.uint32_t(x.Altp2MCount)
>  xc.vmtrace_buf_kb = C.int(x.VmtraceBufKb)
> +xc.vuart = C.libxl_vuart_type(x.Vuart)
>  if err := x.Vpmu.toC(&xc.vpmu); err != nil {
>  return fmt.Errorf("converting field Vpmu: %v", err)
>  }
> diff --git a/tools/golang/xenlight/types.gen.go 
> b/tools/golang/xenlight/types.gen.go
> index 4777f528b52c..2f4153d2510b 100644
> --- a/tools/golang/xenlight/types.gen.go
> +++ b/tools/golang/xenlight/types.gen.go
> @@ -253,6 +253,7 @@ type VuartType int
>  const(
>  VuartTypeUnknown VuartType = 0
>  VuartTypeSbsaUart VuartType = 1
> +VuartTypeNs16550 VuartType = 2
>  )
>  
>  type VkbBackend int
> @@ -596,7 +597,6 @@ Type DomainType
>  TypeUnion DomainBuildInfoTypeUnion
>  ArchArm struct {
>  GicVersion GicVersion
> -Vuart VuartType
>  SveVl SveType
>  NrSpis uint32
>  }
> @@ -608,6 +608,7 @@ Altp2MCount uint32
>  VmtraceBufKb int
>  Vpmu Defbool
>  TrapUnmappedAccesses Defbool
> +Vuart VuartType
>  }
>  
>  type DomainBuildInfoTypeUnion interface {
> 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);
> +        abort();
> +        break;
> +
> +    case LIBXL_VUART_TYPE_UNKNOWN:
> +    default:
> +        break;
>      }
>  
>      for (i = 0; i < d_config->num_disks; i++) {
> @@ -1372,7 +1384,7 @@ next_resize:
>          FDT( make_timer_node(gc, fdt, ainfo, state->clock_frequency) );
>          FDT( make_hypervisor_node(gc, fdt, vers) );
>  
> -        if (info->arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART)
> +        if (info->vuart == LIBXL_VUART_TYPE_SBSA_UART)
>              FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) );
>  
>          if (info->tee == LIBXL_TEE_TYPE_OPTEE)
> @@ -1725,7 +1737,7 @@ int libxl__arch_build_dom_finish(libxl__gc *gc,
>  {
>      int rc = 0, ret;
>  
> -    if (info->arch_arm.vuart != LIBXL_VUART_TYPE_SBSA_UART) {
> +    if (info->vuart != LIBXL_VUART_TYPE_SBSA_UART) {
>          rc = 0;
>          goto out;
>      }
> diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
> index 4042ae1a8957..cfd7e827867a 100644
> --- a/tools/libs/light/libxl_create.c
> +++ b/tools/libs/light/libxl_create.c
> @@ -1815,7 +1815,7 @@ static void domcreate_launch_dm(libxl__egc *egc, 
> libxl__multidev *multidev,
>                                &d_config->vfbs[i]);
>          }
>  
> -        if (d_config->b_info.arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART) {
> +        if (d_config->b_info.vuart == LIBXL_VUART_TYPE_SBSA_UART) {
>              init_console_info(gc, &vuart, 0);
>              vuart.backend_domid = state->console_domid;
>              libxl__device_vuart_add(gc, domid, &vuart, state);
> 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),
>                                 ("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();
> +}
> +
> +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);
> +}
> +
> +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);

As mentioned in the previous commit, I don't think this works as
expected.  You have added XEN_X86_EMU_NS16550 to XEN_X86_EMU_ALL, and
hence you need to subtract it from the mask as it's done with VPCI.

>          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();
> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
> index 7e1aabad6cba..4539e78bb283 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_NS16550
>  
>  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 f44dba61aeab..66a98180d99b 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_NS16550
>  
>  type x86_arch_misc_flags =
>    | X86_MSR_RELAXED
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 28cdbf07c213..b0d266b5bf63 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1498,7 +1498,7 @@ void parse_config_data(const char *config_source,
>          b_info->max_vcpus = l;
>  
>      if (!xlu_cfg_get_string(config, "vuart", &buf, 0)) {
> -        if (libxl_vuart_type_from_string(buf, &b_info->arch_arm.vuart)) {
> +        if (libxl_vuart_type_from_string(buf, &b_info->vuart)) {
>              fprintf(stderr, "ERROR: invalid value \"%s\" for \"vuart\"\n",
>                      buf);
>              exit(1);
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 7fd4f7a831dc..6a010a509a60 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -780,9 +780,10 @@ static bool emulation_flags_ok(const struct domain *d, 
> uint32_t emflags)
>          /* HVM domU */
>          {
>              .caps   = CAP_HVM | CAP_DOMU,
> -            .min    = X86_EMU_ALL & ~(X86_EMU_VPCI | X86_EMU_USE_PIRQ),
> +            .min    = X86_EMU_ALL & ~(X86_EMU_VPCI | X86_EMU_USE_PIRQ |
> +                                      X86_EMU_NS16550),
>              /* HVM PIRQ feature is user-selectable. */
> -            .opt    = X86_EMU_USE_PIRQ,
> +            .opt    = X86_EMU_USE_PIRQ | X86_EMU_NS16550,

Does this need to be part of the patch that adds X86_EMU_NS16550 into
X86_EMU_ALL, as to not break domain creation in the interim?

Thanks, Roger.