On Sat, Sep 06, 2025 at 11:37:28PM +0300, Mykola Kvach wrote:
> Hi Denis,
>
> On Sat, Sep 6, 2025 at 2:27 AM <dmuk...@xen.org> wrote:
> >
> > From: Denis Mukhin <dmuk...@ford.com>
> >
> > The change is the first on the way on introducing minimally functional
> > NS16550-compatible UART emulator.
> >
> > Define UART state and a set of emulated registers.
> >
> > Implement alloc/free vUART hooks.
> >
> > Stub out I/O port handler.
> >
> > Add initialization of the NS16x50-compatible UART emulator state machine.
> >
> > Plumb debug logging.
> >
> > Signed-off-by: Denis Mukhin <dmuk...@ford.com>
> > ---
> > Changes since v5:
> > - v5 feedback
> > - Link to v5:
> > https://lore.kernel.org/xen-devel/20250828235409.2835815-4-dmuk...@ford.com/
> > ---
> > xen/arch/x86/hvm/hvm.c | 21 ++
> > xen/common/emul/vuart/Kconfig | 19 ++
> > xen/common/emul/vuart/Makefile | 1 +
> > xen/common/emul/vuart/ns16x50.c | 366 ++++++++++++++++++++++++++++++++
> > 4 files changed, 407 insertions(+)
> > create mode 100644 xen/common/emul/vuart/ns16x50.c
> >
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index 23bd7f078a1d..91c971f11e14 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -28,6 +28,7 @@
> > #include <xen/softirq.h>
> > #include <xen/trace.h>
> > #include <xen/vm_event.h>
>
> I noticed that this include ...
>
> > +#include <xen/vuart.h>
>
> ... is out of alphabetical order. All other includes in this block
> are correctly sorted.
Thanks, will update.
>
> > #include <xen/vpci.h>
> > #include <xen/wait.h>
> > #include <xen/warning.h>
> > @@ -689,6 +690,22 @@ int hvm_domain_initialise(struct domain *d,
> > if ( rc != 0 )
> > goto fail1;
> >
> > + /* Limit NS16550 emulator for dom0 only for now. */
> > + if ( IS_ENABLED(CONFIG_VUART_NS16X50) && is_hardware_domain(d) )
>
> Currently, the Xen glossary defines a "hardware domain" as:
>
> A domain, commonly dom0, which shares responsibility with Xen
> about the system as a whole.
>
> It has been historically correct to treat is_hardware_domain(d) as
> equivalent to dom0. However, according to patch series [1], this is
> no longer guaranteed:
>
> "Setting hardware domain as domid 0 is removed."
>
> After these changes, the assumption that hardware domain always equals
> dom0 may not hold. As a result, the above comment in the code could
> become misleading. Consider updating it to something like:
>
> /* Limit NS16550 emulator to the hardware domain only for now */
>
> to reflect the new semantics.
I adjusted the code a bit so that it is possible to test vUART for
either hwdom or domU; will remove the comment in v7.
>
> > + {
> > + struct vuart_info info = {
> > + .name = "COM2",
> > + .compatible = "ns16550",
> > + .base_addr = 0x2f8,
> > + .size = 8,
> > + .irq = 3,
> > + };
>
> Consider defining COM2 base address and IRQ in a shared header
> (e.g., VUART_COM2_BASE and VUART_COM2_IRQ) rather than using
> the magic numbers 0x2f8 and 3 here. This would allow reuse in
> `__start_xen` and other places, and makes the code clearer and
> easier to maintain.
PC platform resources are standard and not going to change, so
I think it is satisfactory to use hardcoded numbers here:
COM1 - 0x3f8 / IRQ#4
COM2 - 0x2f8 / IRQ#3
COM3 - 0x3fe / IRQ#4
COM4 - 0x2fe / IRQ#3
Other PC I/O ports still use hardcoded values, e.g. dom0_setup_permissions().
Anyway, this code will change again, once xl support is plumbed.
So, I would keep that as is for now...
[..]
> > +/*
> > + * Emulate 8-bit read access to ns16x50 register.
> > + */
> > +static int ns16x50_io_read8(
> > + struct vuart_ns16x50 *vdev, uint32_t reg, uint8_t *data)
> > +{
> > + uint8_t val = 0xff;
>
> Instead of hardcoding 0xff here, consider using UINT8_MAX. This makes
> it clear that the value is the maximum for uint8_t and avoids magic
> numbers.
>
> Similarly, in other places where constants for 16-bit or 32-bit
> unsigned integers are used, it would be good to replace them with
> UINT16_MAX and UINT32_MAX respectively for consistency and clarity.
Agreed, will do.
Thanks.
[..]
> > +/*
> > + * Emulate I/O access to ns16x50 register.
> > + * Note, emulation always returns X86EMUL_OKAY, once I/O port trap is
> > enabled.
> > + */
> > +static int cf_check ns16x50_io_handle(
> > + int dir, unsigned int addr, unsigned int size, uint32_t *data)
> > +{
> > +#define op(dir) (((dir) == IOREQ_WRITE) ? 'W' : 'R')
>
> Instead of defining the op(dir) macro, it might be cleaner to compute
> the direction character once at the beginning, e.g.:
>
> const char dir_char = (dir == IOREQ_WRITE) ? 'W' : 'R';
>
> and then use dir_char in printk()/debug. This avoids the local macro
> and makes the code easier to follow.
Thanks for suggestion.
Will update.
>
> > + struct domain *d = rcu_lock_current_domain();
> > + struct vuart *vuart = vuart_find_by_io_range(d, addr, size);
> > + struct vuart_ns16x50 *vdev;
> > + const struct domain *owner;
> > + const struct vuart_info *info;
> > + uint32_t reg;
> > + unsigned dlab;
> > + int rc;
> > +
> > + if ( !vuart || !vuart->vdev )
> > + {
> > + printk(XENLOG_ERR "%c io 0x%04x %d: not initialized\n",
> > + op(dir), addr, size);
> > +
> > + ASSERT_UNREACHABLE();
> > + goto out;
> > + }
> > + vdev = vuart->vdev;
> > +
> > + owner = vuart->owner;
> > + ASSERT(owner);
> > + if ( d != owner )
> > + {
> > + ns16x50_err(vdev, "%c io 0x%04x %d: does not match current domain
> > %pv\n",
> > + op(dir), addr, size, d);
> > +
> > + ASSERT_UNREACHABLE();
> > + goto out;
> > + }
> > +
> > + info = vuart->info;
> > + ASSERT(info);
> > + reg = addr - info->base_addr;
> > + if ( !IS_ALIGNED(reg, size) )
> > + {
> > + ns16x50_err(vdev, "%c 0x%04x %d: unaligned access\n",
> > + op(dir), addr, size);
> > + goto out;
> > + }
> > +
> > + dlab = ns16x50_dlab_get(vdev);
> > + if ( reg >= NS16X50_REGS_NUM )
> > + {
> > + ns16x50_err(vdev, "%c io 0x%04x %d: DLAB=%d %02"PRIx32"
> > 0x%08"PRIx32": not implemented\n",
> > + op(dir), addr, size, dlab, reg, *data);
> > + goto out;
> > + }
> > +
> > + spin_lock(&vdev->lock);
> > +
> > + if ( dir == IOREQ_WRITE )
> > + {
> > + ns16x50_debug(vdev, "%c 0x%04x %d: DLAB=%d %02"PRIx32"
> > 0x%08"PRIx32"\n",
> > + op(dir), addr, size, dlab, reg, *data);
> > + rc = ns16x50_io_write(vdev, reg, size, data);
>
> AFAICT ns16x50_io_read() and ns16x50_io_write() have identical
> signatures. Would it be cleaner to store them in an array of
> function pointers and call through that, e.g.:
>
> rc = ns16x50_io_op[dir == IOREQ_WRITE](vdev, reg, size, data);
>
> This would avoid the if/else block here.
I remember I tried this way, but eventually decided to keep just direct calls
for simplicity.
>
> > + }
> > + else
> > + {
> > + rc = ns16x50_io_read(vdev, reg, size, data);
> > + ns16x50_debug(vdev, "%c 0x%04x %d: DLAB=%d %02"PRIx32"
> > 0x%08"PRIx32"\n",
> > + op(dir), addr, size, dlab, reg, *data);
>
> The ns16x50_debug() call is duplicated in both branches of the
> IOREQ_WRITE check, differing only in whether it's placed before or
> after the access. Would it make sense to move this debug print
> outside the condition, so the same code is used for both read and
> write paths (assuming the "data" is not modified during a write)?
Yep, that will work, thanks!
>
> > + }
> > + if ( rc < 0 )
> > + ns16x50_err(vdev, "%c 0x%04x %d: DLAB=%d %02"PRIx32"
> > 0x%08"PRIx32": unsupported access\n",
> > + op(dir), addr, size, dlab, reg, *data);
>
> The ns16x50_err() call doesn’t require holding vdev->lock, so it would
> be cleaner to move it after spin_unlock(). That way the critical section
> is shorter.
Ack.
>
> > +
> > + spin_unlock(&vdev->lock);
> > +
> > +out:
> > + rcu_unlock_domain(d);
> > +
> > + return X86EMUL_OKAY;
> > +#undef op
> > +}
> > +
> > +static int ns16x50_init(void *arg)
> > +{
> > + struct vuart_ns16x50 *vdev = arg;
> > + const struct vuart_info *info = vdev->info;
> > + struct domain *d = vdev->owner;
> > +
> > + ASSERT(vdev);
> > +
> > + register_portio_handler(d, info->base_addr, info->size,
> > ns16x50_io_handle);
> > +
> > + return 0;
> > +}
> > +
> > +static void cf_check ns16x50_deinit(void *arg)
> > +{
> > + struct vuart_ns16x50 *vdev = arg;
> > +
> > + ASSERT(vdev);
> > +}
> > +
> > +static void * cf_check ns16x50_alloc(struct domain *d, const struct
> > vuart_info *info)
> > +{
> > + struct vuart_ns16x50 *vdev;
> > + int rc;
> > +
> > + if ( !info )
> > + return ERR_PTR(-EINVAL);
> > +
> > + if ( vuart_find_by_io_range(d, info->base_addr, info->size) )
> > + {
> > + ns16x50_err(info, "already registered\n");
> > + return ERR_PTR(-EBUSY);
> > + }
> > +
> > + if ( !is_hvm_domain(d) )
>
> Currently, ns16x50_alloc() first calls vuart_find_by_io_range() and
> only afterwards checks if the domain is an HVM domain. Wouldn’t it
> be more logical to perform the HVM check first? If the console is
> only available for HVM domains, the extra check for an uninitialized
> vuart field seems unnecessary.
Ack.
>
> > + {
> > + ns16x50_err(info, "not an HVM domain\n");
> > + return ERR_PTR(-ENOSYS);
> > + }
> > +
> > + vdev = xvzalloc(typeof(*vdev));
> > + if ( !vdev )
> > + {
> > + ns16x50_err(info, "failed to allocate memory\n");
> > + return ERR_PTR(-ENOMEM);
> > + }
> > +
> > + spin_lock_init(&vdev->lock);
> > + vdev->name = info->name;
> > + vdev->owner = d;
> > + vdev->info = info;
> > +
> > + rc = ns16x50_init(vdev);
> > + if ( rc )
>
> If ns16x50_init(vdev) fails, vdev should be freed with xvfree() to
> avoid a memory leak.
Whoops, thanks.
>
> Currently, ns16x50_init() always returns 0. If it is not planned to
> return other values in the future, it may be simpler to make the
> function return void, avoiding the need for the rc variable and
> conditional checks.
There will be non-zero return values in the future.
>
> > + return ERR_PTR(rc);
> > +
> > + return vdev;
> > +}
> > +
> > +static void cf_check ns16x50_free(void *arg)
> > +{
> > + if ( arg )
> > + ns16x50_deinit(arg);
> > +
> > + xvfree(arg);
> > +}
> > +
> > +#define ns16x50_emulator \
> > +{ \
> > + .compatible = "ns16550", \
> > + .alloc = ns16x50_alloc, \
> > + .free = ns16x50_free, \
> > + .dump_state = NULL, \
>
> dump_state is set to NULL, but vuart_dump_state() in the previous
> commit does not check this pointer. If all commits up to this one
> are applied and domain state is dumped, this could result in a
> NULL pointer dereference and crash the hypervisor.
Thanks; will fix vuart.c
>
> Consider adding a NULL check in vuart_dump_state() or initializing
> dump_state properly.
>
> > + .put_rx = NULL, \
> > +}
> > +
> > +VUART_REGISTER(ns16x50, ns16x50_emulator);
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > --
> > 2.51.0
> >
> >
>
> [1] https://patchew.org/Xen/20250416212911.410946-1-jason.andr...@amd.com/
>
> Best regards,
> Mykola
