Re: [PATCH v2 32/35] x86/hvm: add debugging facility to NS8250 UART emulator

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Dec 05, 2024 at 08:42:02PM -0800, Denis Mukhin via B4 Relay wrote:
> From: Denis Mukhin <dmukhin@ford.com>
> 
> Enable keyhandler mechanism for dumping state of emulated NS8250 on the
> console.
> 
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
>  xen/arch/x86/hvm/vuart_ns8250.c | 122 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 122 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/vuart_ns8250.c b/xen/arch/x86/hvm/vuart_ns8250.c
> index 779dbd80d7be4e070ea9df3ae736ecdc662a527a..c8c75afaf2b2419d1dae999da1d1e400fd367791 100644
> --- a/xen/arch/x86/hvm/vuart_ns8250.c
> +++ b/xen/arch/x86/hvm/vuart_ns8250.c
> @@ -25,6 +25,7 @@
>  
>  /* Development debugging */
>  #define NS8250_LOG_LEVEL    0
> +#undef NS8250_DEBUG
>  
>  #include <xen/types.h>
>  #include <xen/event.h>
> @@ -35,6 +36,9 @@
>  #include <xen/resource.h>
>  #include <xen/ctype.h>
>  #include <xen/param.h>
> +#if defined(NS8250_DEBUG)
> +#include <xen/keyhandler.h>
> +#endif
>  #include <xen/console.h> /* console_input_domid() */
>  #include <asm/setup.h>   /* max_init_domid */
>  #include <asm/iocap.h>
> @@ -625,6 +629,121 @@ static const char *ns8250_regname(
>      return reg_names[reg][dir];
>  }
>  
> +#if defined(NS8250_DEBUG)

I don't think the keyhandler should be gated on NS8250_DEBUG, it
should always be present if Xen is built with vUART support.

> +static void ns8250_dump(struct vuart_ns8250 *vdev)
> +{
> +    struct xencons_interface *cons = vdev->cons;

const for both.

> +    uint8_t val;
> +
> +    printk("I/O port %02"PRIx64" IRQ %d flags %"PRIx32" owner %d\n",

I think you want 04 for the io_addr field width?  So that the width is
always fixed, and %pd for owner.

> +            vdev->io_addr, vdev->irq,
> +            vdev->flags, vdev->owner->domain_id);
> +
> +    printk("RX size %ld in_prod %d in_cons %d used %d\n",
> +            sizeof(cons->in),
> +            cons->in_prod, cons->in_cons,
> +            cons->in_prod - cons->in_cons);
> +
> +    printk("TX size %ld out_prod %d out_cons %d used %d\n",
> +            sizeof(cons->out),
> +            cons->out_prod, cons->out_cons,
> +            cons->out_prod - cons->out_cons);
> +
> +    printk("%02x RBR [ %c ] THR [ %c ] DLL %02x DLM %02x\n",
> +            UART_RBR,
> +            cons->in[MASK_XENCONS_IDX(cons->in_prod, cons)],
> +            cons->out[MASK_XENCONS_IDX(cons->out_prod, cons)],
> +            vdev->dl & 0xFFU, vdev->dl >> 8);
> +
> +    printk("%02"PRIx8" IER %02"PRIx8"\n", UART_IER, vdev->regs[UART_IER]);
> +
> +    val = (vdev->regs[UART_FCR] & UART_FCR_ENABLE) ? UART_IIR_FE_MASK : 0;
> +    val |= ns8250_irq_reason(vdev);
> +    printk("%02"PRIx8" FCR %02"PRIx8" IIR %02"PRIx8"\n",
> +            UART_FCR, vdev->regs[UART_FCR], val);
> +
> +    printk("%02"PRIx8" LCR %02"PRIx8"\n", UART_LCR, vdev->regs[UART_LCR]);
> +    printk("%02"PRIx8" MCR %02"PRIx8"\n", UART_MCR, vdev->regs[UART_MCR]);
> +    printk("%02"PRIx8" LSR %02"PRIx8"\n", UART_LSR, vdev->regs[UART_LSR]);
> +    printk("%02"PRIx8" MSR %02"PRIx8"\n", UART_MSR, vdev->regs[UART_MSR]);
> +}
> +
> +static struct domain *rcu_find_first_domain_with_vuart(void)
> +{
> +    struct domain *d = NULL;
> +    domid_t i;
> +
> +    for ( i = 0; i < max_init_domid + 1; i++ )
> +    {
> +        d = rcu_lock_domain_by_id(i);
> +        if ( d == NULL )
> +            continue;
> +
> +        if ( domain_has_vuart(d) )
> +            break;
> +
> +        rcu_unlock_domain(d);
> +    }
> +
> +    return d;
> +}
> +
> +static void cf_check ns8250_keyhandler_show(unsigned char key)
> +{
> +    struct vuart_ns8250 *vdev;
> +    struct domain *d;
> +
> +    d = rcu_find_first_domain_with_vuart();
> +    if ( d == NULL )
> +        return;

I wonder whether you should dump the state of all domains with a
vUART, rather than just a single domain?

> +
> +    printk("'%c' pressed -> dumping virtual NS8250 state (d%d)\n",
> +            key, d->domain_id);
> +
> +    vdev = &d->arch.hvm.vuart;
> +    spin_lock(&vdev->lock);

This should likely be a trylock, so that you can still print the
console state in case of a deadlock.

> +    ns8250_dump(vdev);
> +    spin_unlock(&vdev->lock);
> +
> +    rcu_unlock_domain(d);
> +}
> +
> +static void cf_check ns8250_keyhandler_irq(unsigned char key)
> +{
> +    struct vuart_ns8250 *vdev;
> +    struct domain *d;
> +
> +    d = rcu_find_first_domain_with_vuart();
> +    if ( d == NULL )
> +        return;
> +
> +    printk("'%c' pressed -> triggering IRQ on virtual NS8250 (d%d)\n",
> +            key, d->domain_id);
> +
> +    vdev = &d->arch.hvm.vuart;
> +    spin_lock(&vdev->lock);
> +    ns8250_irq_assert(vdev);
> +    spin_unlock(&vdev->lock);
> +
> +    rcu_unlock_domain(d);
> +}
> +
> +static void ns8250_keyhandler_init(void)
> +{
> +    register_keyhandler('1', ns8250_keyhandler_show,
> +                        "dump virtual NS8250 state", 0);
> +    register_keyhandler('2', ns8250_keyhandler_irq,
> +                        "trigger IRQ from virtual NS8250", 0);
> +}
> +#else
> +static inline void ns8250_keyhandler_init(void)
> +{
> +}
> +static inline void ns8250_dump(struct vuart_ns8250 *vdev)
> +{
> +}
> +#endif /* #if defined(NS8250_DEBUG) */
> +
>  /*
>   * Emulate I/O access to NS8250 register.
>   */
> @@ -688,6 +807,7 @@ static int cf_check ns8250_io_handle(
>      rc = X86EMUL_OKAY;
>  
>  out:
> +    ns8250_dump(vdev);

Likely a remaining of some debugging?  Printing the state for every
access is too verbose.

>      spin_unlock(&vdev->lock);
>  
>      return rc;
> @@ -786,6 +906,7 @@ static int ns8250_init(struct domain *d, const struct resource *r)
>      }
>  
>      spin_lock_init(&vdev->lock);
> +    ns8250_keyhandler_init();

The keyhandler init should be in a __initcall(), otherwise you are
calling it for each domain creation that has a vUART.

Thanks, Roger.