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

[Laurent Vivier]

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