Re: [PATCH v4 1/5] char: add goldfish-tty

[Philippe Mathieu-Daudé]

On 3/7/21 5:48 PM, Laurent Vivier wrote:
> Implement the goldfish tty device as defined in
> 
> https://android.googlesource.com/platform/external/qemu/+/master/docs/GOLDFISH-VIRTUAL-HARDWARE.TXT
> 
> and based on the kernel driver code:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/goldfish.c
> 
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  include/hw/char/goldfish_tty.h |  35 +++++
>  hw/char/goldfish_tty.c         | 272 +++++++++++++++++++++++++++++++++
>  hw/char/Kconfig                |   3 +
>  hw/char/meson.build            |   2 +
>  hw/char/trace-events           |  10 ++
>  5 files changed, 322 insertions(+)
>  create mode 100644 include/hw/char/goldfish_tty.h
>  create mode 100644 hw/char/goldfish_tty.c

> +static void goldfish_tty_write(void *opaque, hwaddr addr,
> +                               uint64_t value, unsigned size)
> +{
> +    GoldfishTTYState *s = opaque;
> +    unsigned char c;
> +
> +    trace_goldfish_tty_write(s, addr, size, value);
> +
> +    switch (addr) {
> +    case REG_PUT_CHAR:
> +        c = value;
> +        qemu_chr_fe_write_all(&s->chr, &c, sizeof(c));

No IRQ here described in the spec... So this is correct.

> +        break;
> +    case REG_CMD:
> +        goldfish_tty_cmd(s, value);
> +        break;
> +    case REG_DATA_PTR:
> +        s->data_ptr = value;
> +        break;
> +    case REG_DATA_PTR_HIGH:
> +        s->data_ptr = (value << 32) | (uint32_t)s->data_ptr;

Matter of taste, I find it easier to review as:

           s->data_ptr = deposit64(s->data_ptr, value, 32, 32);

The spec says this register is only available on 64-bit targets.

Should we restrict this "#if TARGET_LONG_SIZE == 8"?
else log_mask(GUEST_ERROR)...

This seems overkill to me but that would respect the spec.

> +        break;
> +    case REG_DATA_LEN:
> +        s->data_len = value;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP,
> +                      "%s: unimplemented register write 
> 0x%02"HWADDR_PRIx"\n",
> +                      __func__, addr);
> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps goldfish_tty_ops = {
> +    .read = goldfish_tty_read,
> +    .write = goldfish_tty_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid.max_access_size = 4,

Again, I think you also want:

       .impl.min_access_size = 4,

> +    .impl.max_access_size = 4,
> +};

With min_access_size:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>