qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Philippe Mathieu-Daudé
Subject: Re: [PULL 5/9] char: add goldfish-tty
Date: Thu, 11 Mar 2021 23:34:31 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]