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...
