Re: [PULL 5/9] char: add goldfish-tty

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 11 Mar 2021 at 21:22, Laurent Vivier <laurent@vivier.eu> 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>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Message-Id: <20210309195941.763896-2-laurent@vivier.eu>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>

I didn't notice this earlier, but this looks odd:

> +static uint64_t goldfish_tty_read(void *opaque, hwaddr addr,
> +                                  unsigned size)
> +{
> +    GoldfishTTYState *s = opaque;
> +    uint64_t value = 0;
> +
> +    switch (addr) {
> +    case REG_BYTES_READY:
> +        value = fifo8_num_used(&s->rx_fifo);
> +        break;
> +    case REG_VERSION:
> +        value = 0;

You report as a version 0 Goldfish TTY device.
This is the old kind that used guest virtual addresses,
unlike the more sensible version 1 ("ranchu") kind that uses
physical addresses.

You can see this in the kernel driver code:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/goldfish.c
where it looks at qtty->version.

> +    case CMD_WRITE_BUFFER:
> +        len = s->data_len;
> +        ptr = s->data_ptr;
> +        while (len) {
> +            to_copy = MIN(GOLFISH_TTY_BUFFER_SIZE, len);
> +
> +            address_space_rw(&address_space_memory, ptr,
> +                             MEMTXATTRS_UNSPECIFIED, data_out, to_copy, 0);
> +            qemu_chr_fe_write_all(&s->chr, data_out, to_copy);
> +
> +            len -= to_copy;
> +            ptr += to_copy;
> +        }
> +        break;
> +    case CMD_READ_BUFFER:
> +        len = s->data_len;
> +        ptr = s->data_ptr;
> +        while (len && !fifo8_is_empty(&s->rx_fifo)) {
> +            buf = (uint8_t *)fifo8_pop_buf(&s->rx_fifo, len, &to_copy);
> +            address_space_rw(&address_space_memory, ptr,
> +                            MEMTXATTRS_UNSPECIFIED, buf, to_copy, 1);
> +
> +            len -= to_copy;
> +            ptr += to_copy;
> +        }

...but here you're treating the data pointer value from the
guest like a physical address. I'm not sure how this works.

(This is one of the areas where you need to be really cautious about
using the goldfish devices -- "device model gets virtual addresses from
guest OS" is a really bad design.)

thanks
-- PMM