| Message ID | 1375285109-3053-6-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
At 16:38 +0100 on 31 Jul (1375288705), Julien Grall wrote: > This code is based on the previous vuart0 implementation. Unlike the latter, > it's intend to replace UART stolen by XEN to DOM0 via dtuart=... on its > command line. > > It's useful when the kernel is compiled with early printk enabled or for a > single platform. Most of the time, the hardcoded code to handle the UART > will need 2 registers: status and data, the others registers can be > implemented as RAZ/WI. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> [...] > @@ -525,8 +525,12 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags) > if ( (rc = vcpu_domain_init(d)) != 0 ) > goto fail; > > - /* Domain 0 gets a real UART not an emulated one */ > - if ( d->domain_id && (rc = domain_uart0_init(d)) != 0 ) > + /* > + * Virtual UART is only used by linux early printk and decompress code. > + * Only use it for dom0 because the linux kernel may not support > + * multi-platform. > + */ > + if ( (d->domain_id == 0) && (rc = domain_vuart_init(d)) ) > goto fail; So this is changing from: - dom0 gets a real UART, domU gets an emulated one; to - dom0 gets an emulated UART (and a real on), domU gets nothing. I think that domU losing its UART should be mentioned in the changeset description. And won't domU kernels need an emulated UART too? Can an admin not use the same kernel image for dom0 and domU? [...] > + * This emulator uses the information from dtuart. This is not intended to be > + * a full emulation of an UART device. Rather it is intended to provide a > + * sufficient veneer of one that early code (such as Linux's boot time > + * decompressor) which hardcodes output directly to such a device are able to > + * make progress. > + * > + * The mininal register set to emulate an UART are: s/mininal/minimal/ [...] > -static int uart0_mmio_write(struct vcpu *v, mmio_info_t *info) > +static int vuart_mmio_write(struct vcpu *v, mmio_info_t *info) > { > + struct domain *d = v->domain; > struct hsr_dabt dabt = info->dabt; > struct cpu_user_regs *regs = guest_cpu_user_regs(); > register_t *r = select_user_reg(regs, dabt.reg); > - int offset = (int)(info->gpa - UART0_START); > + paddr_t offset = (int)(info->gpa - d->arch.vuart.info->base_addr); Please drop the cast to int here. AFAICT it's just confusing. > > - switch ( offset ) > - { > - case UARTDR: > + if ( offset == d->arch.vuart.info->data_off ) > /* ignore any status bits */ > - uart0_print_char((int)((*r) & 0xFF)); > - return 1; > - case UARTFR: > - /* Silently ignore */ > - return 1; > - default: > - printk("VPL011: unhandled write r%d=%"PRIregister" offset %#08x\n", > - dabt.reg, *r, offset); > - domain_crash_synchronous(); > - } > + vuart_print_char((int)((*r) & 0xFF)); What's this cast to int for? The argument is implicitly cast to char in any case. It's also a bity surprising that we've got a struct vcpu *v in our hands this far down the stack but vuart_print_char() uses current. (I realise that both these things are issues in the existing code but since you're already touching it it might be worth cleaning up.) Cheers, Tim.
On 08/01/2013 12:22 PM, Tim Deegan wrote: > At 16:38 +0100 on 31 Jul (1375288705), Julien Grall wrote: >> This code is based on the previous vuart0 implementation. Unlike the latter, >> it's intend to replace UART stolen by XEN to DOM0 via dtuart=... on its >> command line. >> >> It's useful when the kernel is compiled with early printk enabled or for a >> single platform. Most of the time, the hardcoded code to handle the UART >> will need 2 registers: status and data, the others registers can be >> implemented as RAZ/WI. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > [...] >> @@ -525,8 +525,12 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags) >> if ( (rc = vcpu_domain_init(d)) != 0 ) >> goto fail; >> >> - /* Domain 0 gets a real UART not an emulated one */ >> - if ( d->domain_id && (rc = domain_uart0_init(d)) != 0 ) >> + /* >> + * Virtual UART is only used by linux early printk and decompress code. >> + * Only use it for dom0 because the linux kernel may not support >> + * multi-platform. >> + */ >> + if ( (d->domain_id == 0) && (rc = domain_vuart_init(d)) ) >> goto fail; > > So this is changing from: > - dom0 gets a real UART, domU gets an emulated one; to > - dom0 gets an emulated UART (and a real on), domU gets nothing. > > I think that domU losing its UART should be mentioned in the changeset > description. > > And won't domU kernels need an emulated UART too? Can an admin not use > the same kernel image for dom0 and domU? In theory, it's possible to use the same kernel for dom0 and domU only if the kernel support multi-platform (CONFIG_ARCH_MULTIPLATFORM). In this case the early printk should be disabled by default, unless for debugging. I don't like the solution to expose the UART to guest because the memory layout might be different between the real hardware and the guest. I would prefer to implement a specific xen early printk in the kernel. > > [...] >> + * This emulator uses the information from dtuart. This is not intended to be >> + * a full emulation of an UART device. Rather it is intended to provide a >> + * sufficient veneer of one that early code (such as Linux's boot time >> + * decompressor) which hardcodes output directly to such a device are able to >> + * make progress. >> + * >> + * The mininal register set to emulate an UART are: > > s/mininal/minimal/ > [...] >> -static int uart0_mmio_write(struct vcpu *v, mmio_info_t *info) >> +static int vuart_mmio_write(struct vcpu *v, mmio_info_t *info) >> { >> + struct domain *d = v->domain; >> struct hsr_dabt dabt = info->dabt; >> struct cpu_user_regs *regs = guest_cpu_user_regs(); >> register_t *r = select_user_reg(regs, dabt.reg); >> - int offset = (int)(info->gpa - UART0_START); >> + paddr_t offset = (int)(info->gpa - d->arch.vuart.info->base_addr); > > Please drop the cast to int here. AFAICT it's just confusing. > >> >> - switch ( offset ) >> - { >> - case UARTDR: >> + if ( offset == d->arch.vuart.info->data_off ) >> /* ignore any status bits */ >> - uart0_print_char((int)((*r) & 0xFF)); >> - return 1; >> - case UARTFR: >> - /* Silently ignore */ >> - return 1; >> - default: >> - printk("VPL011: unhandled write r%d=%"PRIregister" offset %#08x\n", >> - dabt.reg, *r, offset); >> - domain_crash_synchronous(); >> - } >> + vuart_print_char((int)((*r) & 0xFF)); > > What's this cast to int for? The argument is implicitly cast to char in > any case. It's also a bity surprising that we've got a struct vcpu *v > in our hands this far down the stack but vuart_print_char() uses current. > (I realise that both these things are issues in the existing code but > since you're already touching it it might be worth cleaning up.) I don't see any reason for the cast, the code come from Ian Campbell. Ian, do know why? I will address all your comments in the next patch series.
On Thu, 2013-08-01 at 13:43 +0100, Julien Grall wrote: > >> > >> - switch ( offset ) > >> - { > >> - case UARTDR: > >> + if ( offset == d->arch.vuart.info->data_off ) > >> /* ignore any status bits */ > >> - uart0_print_char((int)((*r) & 0xFF)); > >> - return 1; > >> - case UARTFR: > >> - /* Silently ignore */ > >> - return 1; > >> - default: > >> - printk("VPL011: unhandled write r%d=%"PRIregister" offset %#08x\n", > >> - dabt.reg, *r, offset); > >> - domain_crash_synchronous(); > >> - } > >> + vuart_print_char((int)((*r) & 0xFF)); > > > > What's this cast to int for? The argument is implicitly cast to char in > > any case. It's also a bity surprising that we've got a struct vcpu *v > > in our hands this far down the stack but vuart_print_char() uses current. > > (I realise that both these things are issues in the existing code but > > since you're already touching it it might be worth cleaning up.) > > I don't see any reason for the cast, the code come from Ian Campbell. > Ian, do know why? Nope ;-) If there was a reason I've no idea what it was... Ian.
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index 5c13a65..003ac84 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -27,7 +27,7 @@ obj-y += shutdown.o obj-y += traps.o obj-y += vgic.o obj-y += vtimer.o -obj-y += vpl011.o +obj-y += vuart.o obj-y += hvm.o obj-y += device.o obj-y += decode.o diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 4e9cece..cb0424d 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -32,7 +32,7 @@ #include <asm/gic.h> #include "vtimer.h" -#include "vpl011.h" +#include "vuart.h" DEFINE_PER_CPU(struct vcpu *, curr_vcpu); @@ -525,8 +525,12 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags) if ( (rc = vcpu_domain_init(d)) != 0 ) goto fail; - /* Domain 0 gets a real UART not an emulated one */ - if ( d->domain_id && (rc = domain_uart0_init(d)) != 0 ) + /* + * Virtual UART is only used by linux early printk and decompress code. + * Only use it for dom0 because the linux kernel may not support + * multi-platform. + */ + if ( (d->domain_id == 0) && (rc = domain_vuart_init(d)) ) goto fail; return 0; @@ -542,7 +546,7 @@ void arch_domain_destroy(struct domain *d) { p2m_teardown(d); domain_vgic_free(d); - domain_uart0_free(d); + domain_vuart_free(d); free_xenheap_page(d->shared_info); } diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c index ad28c26..a6db00b 100644 --- a/xen/arch/arm/io.c +++ b/xen/arch/arm/io.c @@ -25,7 +25,7 @@ static const struct mmio_handler *const mmio_handlers[] = { &vgic_distr_mmio_handler, - &uart0_mmio_handler, + &vuart_mmio_handler, }; #define MMIO_HANDLER_NR ARRAY_SIZE(mmio_handlers) diff --git a/xen/arch/arm/io.h b/xen/arch/arm/io.h index 661dce1..8d252c0 100644 --- a/xen/arch/arm/io.h +++ b/xen/arch/arm/io.h @@ -41,7 +41,7 @@ struct mmio_handler { }; extern const struct mmio_handler vgic_distr_mmio_handler; -extern const struct mmio_handler uart0_mmio_handler; +extern const struct mmio_handler vuart_mmio_handler; extern int handle_mmio(mmio_info_t *info); diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vuart.c similarity index 43% rename from xen/arch/arm/vpl011.c rename to xen/arch/arm/vuart.c index 13ba623..0d672d5 100644 --- a/xen/arch/arm/vpl011.c +++ b/xen/arch/arm/vuart.c @@ -1,8 +1,22 @@ /* - * xen/arch/arm/vpl011.c + * xen/arch/arm/vuart.c * - * ARM PL011 UART Emulator (DEBUG) + * Virtual UART Emulator. * + * This emulator uses the information from dtuart. This is not intended to be + * a full emulation of an UART device. Rather it is intended to provide a + * sufficient veneer of one that early code (such as Linux's boot time + * decompressor) which hardcodes output directly to such a device are able to + * make progress. + * + * The mininal register set to emulate an UART are: + * - Single byte transmit register + * - Single status register + * + * /!\ This device is not intended to be enumerable or exposed to the OS + * (e.g. via Device Tree). + * + * Julien Grall <julien.grall@linaro.org> * Ian Campbell <ian.campbell@citrix.com> * Copyright (c) 2012 Citrix Systems. * @@ -17,53 +31,48 @@ * GNU General Public License for more details. */ -/* - * This is not intended to be a full emulation of a PL011 - * device. Rather it is intended to provide a sufficient veneer of one - * that early code (such as Linux's boot time decompressor) which - * hardcodes output directly to such a device are able to make progress. - * - * This device is not intended to be enumerable or exposed to the OS - * (e.g. via Device Tree). - */ #include <xen/config.h> #include <xen/lib.h> #include <xen/sched.h> #include <xen/errno.h> #include <xen/ctype.h> +#include <xen/serial.h> +#include "vuart.h" #include "io.h" -#define UART0_START 0x1c090000 -#define UART0_END (UART0_START+65536) - -#define UARTDR 0x000 -#define UARTFR 0x018 +#define domain_has_vuart(d) ((d)->arch.vuart.info != NULL) -int domain_uart0_init(struct domain *d) +int domain_vuart_init(struct domain *d) { - ASSERT( d->domain_id ); + ASSERT( !d->domain_id ); - spin_lock_init(&d->arch.uart0.lock); - d->arch.uart0.idx = 0; + d->arch.vuart.info = serial_vuart_info(SERHND_DTUART); + if ( !d->arch.vuart.info ) + return 0; - d->arch.uart0.buf = xzalloc_array(char, VPL011_BUF_SIZE); - if ( !d->arch.uart0.buf ) + spin_lock_init(&d->arch.vuart.lock); + d->arch.vuart.idx = 0; + + d->arch.vuart.buf = xzalloc_array(char, VUART_BUF_SIZE); + if ( !d->arch.vuart.buf ) return -ENOMEM; return 0; - } -void domain_uart0_free(struct domain *d) +void domain_vuart_free(struct domain *d) { - xfree(d->arch.uart0.buf); + if ( !domain_has_vuart(d) ) + return; + + xfree(d->arch.vuart.buf); } -static void uart0_print_char(char c) +static void vuart_print_char(char c) { - struct vpl011 *uart = ¤t->domain->arch.uart0; + struct vuart *uart = ¤t->domain->arch.vuart; /* Accept only printable characters, newline, and horizontal tab. */ if ( !isprint(c) && (c != '\n') && (c != '\t') ) @@ -71,7 +80,7 @@ static void uart0_print_char(char c) spin_lock(&uart->lock); uart->buf[uart->idx++] = c; - if ( (uart->idx == (VPL011_BUF_SIZE - 2)) || (c == '\n') ) + if ( (uart->idx == (VUART_BUF_SIZE - 2)) || (c == '\n') ) { if ( c != '\n' ) uart->buf[uart->idx++] = '\n'; @@ -83,62 +92,51 @@ static void uart0_print_char(char c) spin_unlock(&uart->lock); } -static int uart0_mmio_check(struct vcpu *v, paddr_t addr) +static int vuart_mmio_check(struct vcpu *v, paddr_t addr) { - struct domain *d = v->domain; + const struct vuart_info *info = v->domain->arch.vuart.info; - return d->domain_id != 0 && addr >= UART0_START && addr < UART0_END; + return (domain_has_vuart(v->domain) && addr >= info->base_addr && + addr <= (info->base_addr + info->size)); } -static int uart0_mmio_read(struct vcpu *v, mmio_info_t *info) +static int vuart_mmio_read(struct vcpu *v, mmio_info_t *info) { + struct domain *d = v->domain; struct hsr_dabt dabt = info->dabt; struct cpu_user_regs *regs = guest_cpu_user_regs(); register_t *r = select_user_reg(regs, dabt.reg); - int offset = (int)(info->gpa - UART0_START); + paddr_t offset = info->gpa - d->arch.vuart.info->base_addr; - switch ( offset ) - { - case UARTDR: - *r = 0; - return 1; - case UARTFR: - *r = 0x87; /* All holding registers empty, ready to send etc */ - return 1; - default: - printk("VPL011: unhandled read r%d offset %#08x\n", - dabt.reg, offset); - domain_crash_synchronous(); - } + /* By default zeroed the register */ + *r = 0; + + if ( offset == d->arch.vuart.info->status_off ) + /* All holding registers empty, ready to send etc */ + *r = d->arch.vuart.info->status; + + return 1; } -static int uart0_mmio_write(struct vcpu *v, mmio_info_t *info) +static int vuart_mmio_write(struct vcpu *v, mmio_info_t *info) { + struct domain *d = v->domain; struct hsr_dabt dabt = info->dabt; struct cpu_user_regs *regs = guest_cpu_user_regs(); register_t *r = select_user_reg(regs, dabt.reg); - int offset = (int)(info->gpa - UART0_START); + paddr_t offset = (int)(info->gpa - d->arch.vuart.info->base_addr); - switch ( offset ) - { - case UARTDR: + if ( offset == d->arch.vuart.info->data_off ) /* ignore any status bits */ - uart0_print_char((int)((*r) & 0xFF)); - return 1; - case UARTFR: - /* Silently ignore */ - return 1; - default: - printk("VPL011: unhandled write r%d=%"PRIregister" offset %#08x\n", - dabt.reg, *r, offset); - domain_crash_synchronous(); - } + vuart_print_char((int)((*r) & 0xFF)); + + return 1; } -const struct mmio_handler uart0_mmio_handler = { - .check_handler = uart0_mmio_check, - .read_handler = uart0_mmio_read, - .write_handler = uart0_mmio_write, +const struct mmio_handler vuart_mmio_handler = { + .check_handler = vuart_mmio_check, + .read_handler = vuart_mmio_read, + .write_handler = vuart_mmio_write, }; /* diff --git a/xen/arch/arm/vpl011.h b/xen/arch/arm/vuart.h similarity index 75% rename from xen/arch/arm/vpl011.h rename to xen/arch/arm/vuart.h index f0d0a82..9445e50 100644 --- a/xen/arch/arm/vpl011.h +++ b/xen/arch/arm/vuart.h @@ -1,7 +1,7 @@ -/* - * xen/arch/arm/vpl011.h +/* + * xen/arch/arm/vuart.h * - * ARM PL011 Emulation Support + * Virtual UART Emulation Support * * Ian Campbell <ian.campbell@citrix.com> * Copyright (c) 2012 Citrix Systems. @@ -17,13 +17,13 @@ * GNU General Public License for more details. */ -#ifndef __ARCH_ARM_VPL011_H__ -#define __ARCH_ARM_VPL011_H__ +#ifndef __ARCH_ARM_VUART_H__ +#define __ARCH_ARM_VUART_H__ -extern int domain_uart0_init(struct domain *d); -extern void domain_uart0_free(struct domain *d); +int domain_vuart_init(struct domain *d); +void domain_vuart_free(struct domain *d); -#endif +#endif /* __ARCH_ARM_VUART_H__ */ /* * Local variables: diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 89f88f6..67bfbbc 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -8,6 +8,7 @@ #include <asm/p2m.h> #include <asm/vfp.h> #include <public/hvm/params.h> +#include <xen/serial.h> /* Represents state corresponding to a block of 32 interrupts */ struct vgic_irq_rank { @@ -103,12 +104,13 @@ struct arch_domain paddr_t cbase; /* CPU base address */ } vgic; - struct vpl011 { -#define VPL011_BUF_SIZE 128 - char *buf; - int idx; - spinlock_t lock; - } uart0; + struct vuart { +#define VUART_BUF_SIZE 128 + char *buf; + int idx; + const struct vuart_info *info; + spinlock_t lock; + } vuart; } __cacheline_aligned;
This code is based on the previous vuart0 implementation. Unlike the latter, it's intend to replace UART stolen by XEN to DOM0 via dtuart=... on its command line. It's useful when the kernel is compiled with early printk enabled or for a single platform. Most of the time, the hardcoded code to handle the UART will need 2 registers: status and data, the others registers can be implemented as RAZ/WI. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v2: - Merge and update comments - Use the renamed callback --- xen/arch/arm/Makefile | 2 +- xen/arch/arm/domain.c | 12 ++-- xen/arch/arm/io.c | 2 +- xen/arch/arm/io.h | 2 +- xen/arch/arm/{vpl011.c => vuart.c} | 128 ++++++++++++++++++------------------ xen/arch/arm/{vpl011.h => vuart.h} | 16 ++--- xen/include/asm-arm/domain.h | 14 ++-- 7 files changed, 90 insertions(+), 86 deletions(-) rename xen/arch/arm/{vpl011.c => vuart.c} (43%) rename xen/arch/arm/{vpl011.h => vuart.h} (75%)