Re: [PATCH v2 2/7] char: add goldfish-tty

[Philippe Mathieu-Daudé]

On 12/20/20 12:26 PM, Laurent Vivier wrote:
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  include/hw/char/goldfish_tty.h |  36 +++++
>  hw/char/goldfish_tty.c         | 266 +++++++++++++++++++++++++++++++++
>  hw/char/Kconfig                |   3 +
>  hw/char/meson.build            |   2 +
>  hw/char/trace-events           |   9 ++
>  5 files changed, 316 insertions(+)
>  create mode 100644 include/hw/char/goldfish_tty.h
>  create mode 100644 hw/char/goldfish_tty.c
...

> +static void goldfish_tty_cmd(GoldfishTTYState *s, uint32_t cmd)
> +{
> +    int to_copy;
> +
> +    switch (cmd) {
> +    case CMD_INT_DISABLE:
> +        if (s->int_enabled) {
> +            if (s->data_in_count) {
> +                qemu_set_irq(s->irq, 0);
> +            }
> +            s->int_enabled = false;
> +        }
> +        break;
> +    case CMD_INT_ENABLE:
> +        if (!s->int_enabled) {
> +            if (s->data_in_count) {
> +                qemu_set_irq(s->irq, 1);
> +            }
> +            s->int_enabled = true;
> +        }
> +        break;
> +    case CMD_WRITE_BUFFER:
> +        to_copy = s->data_len;
> +        while (to_copy) {
> +            int len;
> +
> +            len = MIN(GOLFISH_TTY_BUFFER_SIZE, to_copy);
> +
> +            address_space_rw(&address_space_memory, s->data_ptr,
> +                             MEMTXATTRS_UNSPECIFIED, s->data_out, len, 0);

Could this fail, no need to check return value?

> +            to_copy -= len;
> +            qemu_chr_fe_write_all(&s->chr, s->data_out, len);
> +        }
> +        break;
> +    case CMD_READ_BUFFER:
> +        to_copy = MIN(s->data_len, s->data_in_count);
> +        address_space_rw(&address_space_memory, s->data_ptr,
> +                         MEMTXATTRS_UNSPECIFIED, s->data_in, to_copy, 1);

Ditto.

> +        s->data_in_count -= to_copy;
> +        memmove(s->data_in, s->data_in + to_copy, s->data_in_count);
> +        if (s->int_enabled && !s->data_in_count) {
> +            qemu_set_irq(s->irq, 0);
> +        }
> +        break;
> +    }
> +}
> +
> +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));
> +        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;
> +        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,
> +    .impl.max_access_size = 4,

Missing:

  .impl.min = 4,

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

> +};
> +