Re: [PATCH v7 12/16] emul/ns16550: implement dump_state() hook

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Denis,

Thank you for the patch.

On Tue, Sep 9, 2025 at 12:12 AM <dmukhin@xxxxxxx> wrote:
>
> From: Denis Mukhin <dmukhin@xxxxxxxx>
>
> Implement dump_state() vUART hook for debugging UART state machine over Xen
> console. dump_state() prints state of all emulated registers (including
> state-less IIR) and state of RX/TX FIFOs.
>
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
> Changes since v6:
> - n/a
> ---
>  xen/common/emul/vuart/ns16x50.c | 59 ++++++++++++++++++++++++++++++++-
>  1 file changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/emul/vuart/ns16x50.c b/xen/common/emul/vuart/ns16x50.c
> index a92df6923aa5..c341f012d005 100644
> --- a/xen/common/emul/vuart/ns16x50.c
> +++ b/xen/common/emul/vuart/ns16x50.c
> @@ -640,6 +640,58 @@ static int ns16x50_io_read(
>      return rc;
>  }
>
> +static void cf_check ns16x50_dump_state(void *arg)
> +{
> +#ifdef CONFIG_VUART_NS16X50_DEBUG
> +    struct vuart_ns16x50 *vdev = arg;
> +    const struct domain *d = vdev->owner;
> +    const struct vuart_info *info = vdev->info;
> +    const struct xencons_interface *cons;
> +    const uint8_t *regs;
> +
> +    if ( !vdev )

Is this NULL check actually useful here? At this point we’ve already
dereferenced vdev (vdev->owner / vdev->info), so if arg could be NULL
we’d already be in UB. Either the hook never receives NULL (and we can
drop the check or turn it into ASSERT(vdev)), or the check should be
moved before the first dereference.

> +        return;
> +
> +    /* Allow printing state in case of a deadlock. */
> +    if ( !spin_trylock(&vdev->lock) )
> +        return;
> +
> +    cons = &vdev->cons;
> +    regs = &vdev->regs[0];
> +
> +    printk("Virtual " pr_prefix " (%s) I/O port 0x%04x IRQ#%d owner %pd\n",
> +            vdev->name, info->base_addr, info->irq, d);
> +
> +    printk("  RX FIFO size %ld in_prod %d in_cons %d used %d\n",
> +            ARRAY_SIZE(cons->in), cons->in_prod, cons->in_cons,
> +            cons->in_prod - cons->in_cons);
> +
> +    printk("  TX FIFO size %ld out_prod %d out_cons %d used %d\n",
> +            ARRAY_SIZE(cons->out), cons->out_prod, cons->out_cons,
> +            cons->out_prod - cons->out_cons);
> +
> +    printk("  %02"PRIx8" RBR %02"PRIx8" THR %02"PRIx8" DLL %02"PRIx8" DLM 
> %02"PRIx8"\n",
> +            UART_RBR,

Should this be using cons->in / cons->out instead of cons?

> +            cons->in[MASK_XENCONS_IDX(cons->in_prod, cons)],
> +            cons->out[MASK_XENCONS_IDX(cons->out_prod, cons)],

As written, MASK_XENCONS_IDX() gets &vdev->cons (struct pointer), not the
RX/TX arrays themselves, so its size/index calculation will use the size
of the pointer/struct rather than the in[]/out[] ring size. I think this
should be:

    cons->in[MASK_XENCONS_IDX(cons->in_prod, cons->in)],
    cons->out[MASK_XENCONS_IDX(cons->out_prod, cons->out)],


Best regards,
Mykola


> +            regs[NS16X50_REGS_NUM + UART_DLL],
> +            regs[NS16X50_REGS_NUM + UART_DLM]);
> +
> +    printk("  %02"PRIx8" IER %02"PRIx8"\n", UART_IER, regs[UART_IER]);
> +
> +    printk("  %02"PRIx8" FCR %02"PRIx8" IIR %02"PRIx8"\n",
> +            UART_FCR, regs[UART_FCR], ns16x50_iir_get(vdev));
> +
> +    printk("  %02"PRIx8" LCR %02"PRIx8"\n", UART_LCR, regs[UART_LCR]);
> +    printk("  %02"PRIx8" MCR %02"PRIx8"\n", UART_MCR, regs[UART_MCR]);
> +    printk("  %02"PRIx8" LSR %02"PRIx8"\n", UART_LSR, regs[UART_LSR]);
> +    printk("  %02"PRIx8" MSR %02"PRIx8"\n", UART_MSR, regs[UART_MSR]);
> +    printk("  %02"PRIx8" SCR %02"PRIx8"\n", UART_SCR, regs[UART_SCR]);
> +
> +    spin_unlock(&vdev->lock);
> +#endif /* CONFIG_VUART_NS16X50_DEBUG */
> +}
> +
>  /*
>   * Emulate I/O access to ns16x50 register.
>   * Note, emulation always returns X86EMUL_OKAY, once I/O port trap is 
> enabled.
> @@ -709,6 +761,9 @@ static int cf_check ns16x50_io_handle(
>
>      spin_unlock(&vdev->lock);
>
> +    if ( ns16x50_log_level >= 3 )
> +        ns16x50_dump_state(vdev);
> +
>      if ( rc == 0 )
>          ns16x50_debug(vdev, "%c 0x%04x %d: DLAB=%d %02"PRIx32" 
> 0x%08"PRIx32"\n",
>                        op, addr, size, dlab, reg, *data);
> @@ -844,6 +899,8 @@ static int cf_check ns16x50_put_rx(void *arg, char ch)
>      }
>
>      spin_unlock(&vdev->lock);
> +    if ( ns16x50_log_level >= 3 )
> +        ns16x50_dump_state(vdev);
>
>      return rc;
>  }
> @@ -853,7 +910,7 @@ static int cf_check ns16x50_put_rx(void *arg, char ch)
>      .compatible = "ns16550",            \
>      .alloc      = ns16x50_alloc,        \
>      .free       = ns16x50_free,         \
> -    .dump_state = NULL,                 \
> +    .dump_state = ns16x50_dump_state,   \
>      .put_rx     = ns16x50_put_rx,       \
>  }
>
> --
> 2.51.0
>
>