| Message ID | 20241205-vuart-ns8250-v1-32-e9aa923127eb@ford.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce NS8250 UART emulator | expand |
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.
On 06.12.2024 05:42, Denis Mukhin via B4 Relay wrote: > +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); Characters for key handlers are a pretty scarce resource. I'm afraid I wouldn't want to see this committed, even if it may be helpful during (initial) debugging. Even more so when both handlers only ever act on a single domain. Imo both want to be domctl-s instead. Jan
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) +static void ns8250_dump(struct vuart_ns8250 *vdev) +{ + struct xencons_interface *cons = vdev->cons; + uint8_t val; + + printk("I/O port %02"PRIx64" IRQ %d flags %"PRIx32" owner %d\n", + 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; + + printk("'%c' pressed -> dumping virtual NS8250 state (d%d)\n", + key, d->domain_id); + + vdev = &d->arch.hvm.vuart; + spin_lock(&vdev->lock); + 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); 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(); hvm_irq_lower(d, vdev->irq); vdev->dl = (UART_CLOCK_HZ / 115200) >> 4; /* Report 115200 baud rate */ @@ -852,6 +973,7 @@ int vuart_putchar(struct vuart_ns8250 *vdev, char ch) ns8250_irq_assert(vdev); out: + ns8250_dump(vdev); spin_unlock(&vdev->lock); return rc;