Le 11/03/2021 à 23:34, Philippe Mathieu-Daudé a écrit : > On 3/11/21 11:04 PM, Laurent Vivier wrote: >> Le 11/03/2021 à 22:57, Peter Maydell a écrit : >>> On Thu, 11 Mar 2021 at 21:22, Laurent Vivier <laur...@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 <laur...@vivier.eu> >>>> Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org> >>>> Tested-by: Philippe Mathieu-Daudé <f4...@amsat.org> >>>> Message-Id: <20210309195941.763896-2-laur...@vivier.eu> >>>> Signed-off-by: Laurent Vivier <laur...@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.) >> >> Thank you Peter. >> >> I will resend the pull request without the virt m68k machine part. > > Laurent, if the issue Peter reported is only for the goldfish-tty, > you might consider merging the m68k-virt with only virtio-serial > console for 6.0... >
It's a good idea, I will test if the machine is usable without it, and I will send a PR before the softfreeze. Thanks, Laurent
