[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...
- [PULL 0/9] M68k for 6.0 patches, Laurent Vivier, 2021/03/11
- [PULL 2/9] target/m68k: don't set SSW ATC bit for physical bus errors, Laurent Vivier, 2021/03/11
- [PULL 1/9] target/m68k: implement rtr instruction, Laurent Vivier, 2021/03/11
- [PULL 4/9] target/m68k: add M68K_FEATURE_UNALIGNED_DATA feature, Laurent Vivier, 2021/03/11
- [PULL 3/9] target/m68k: reformat m68k_features enum, Laurent Vivier, 2021/03/11
- [PULL 7/9] m68k: add an interrupt controller, Laurent Vivier, 2021/03/11
- [PULL 5/9] char: add goldfish-tty, Laurent Vivier, 2021/03/11
- Re: [PULL 5/9] char: add goldfish-tty, Daniel P . Berrangé, 2021/03/15
[PULL 6/9] intc: add goldfish-pic, Laurent Vivier, 2021/03/11
[PULL 8/9] m68k: add a system controller, Laurent Vivier, 2021/03/11
[PULL 9/9] m68k: add Virtual M68k Machine, Laurent Vivier, 2021/03/11