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

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Friday, December 13th, 2024 at 4:08 AM, Roger Pau Monné 
<roger.pau@xxxxxxxxxx> wrote:

>
>
> On Thu, Dec 05, 2024 at 08:42:02PM -0800, Denis Mukhin via B4 Relay wrote:
>
> > From: Denis Mukhin dmukhin@xxxxxxxx
> >
> > Enable keyhandler mechanism for dumping state of emulated NS8250 on the
> > console.
> >
> > Signed-off-by: Denis Mukhin dmukhin@xxxxxxxx
> > ---
> > 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.

Fixed.

>
> > + 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.

Fixed.

>
> > + 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?

Looks like 'q' handler is a better place for such printout.

>
> > +
> > + 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.

Fixed.

>
> > + 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.

That was actually helpful for debugging.
Those calls will be compiled-out in default build w/ emulator.

>
> > 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.

Thank you.
I nuked vUART keyhandlers altogether in v3.

>
> Thanks, Roger.