Re: [Xen-devel] [PATCH v2 5/8] xen/arm: Implement a virtual UART

[Tim Deegan <tim@xxxxxxx>]

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@xxxxxxxxxx>
[...]
> @@ -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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel