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

[Julien Grall <julien.grall@linaro.org>]

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.

-- 
Julien