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

[Laurent Vivier]

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