Re: [PATCH v4 1/5] char: add goldfish-tty

From: Laurent Vivier <laurent@vivier.eu>
To: "Philippe Mathieu-Daudé" <f4bug@amsat.org>, qemu-devel@nongnu.org
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"Paolo Bonzini" <pbonzini@redhat.com>
Subject: Re: [PATCH v4 1/5] char: add goldfish-tty
Date: Sun, 7 Mar 2021 20:57:27 +0100	[thread overview]
Message-ID: <c823a8ae-4060-02c4-1bbd-688793c0b18f@vivier.eu> (raw)
In-Reply-To: <6909cb99-6058-472e-bfa2-62172c448b1d@amsat.org>

Le 07/03/2021 à 20:24, Philippe Mathieu-Daudé a écrit :
> On 3/7/21 5:48 PM, Laurent Vivier 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>
>> ---
>>  include/hw/char/goldfish_tty.h |  35 +++++
>>  hw/char/goldfish_tty.c         | 272 +++++++++++++++++++++++++++++++++
>>  hw/char/Kconfig                |   3 +
>>  hw/char/meson.build            |   2 +
>>  hw/char/trace-events           |  10 ++
>>  5 files changed, 322 insertions(+)
>>  create mode 100644 include/hw/char/goldfish_tty.h
>>  create mode 100644 hw/char/goldfish_tty.c
> 
>> +static void goldfish_tty_write(void *opaque, hwaddr addr,
>> +                               uint64_t value, unsigned size)
>> +{
>> +    GoldfishTTYState *s = opaque;
>> +    unsigned char c;
>> +
>> +    trace_goldfish_tty_write(s, addr, size, value);
>> +
>> +    switch (addr) {
>> +    case REG_PUT_CHAR:
>> +        c = value;
>> +        qemu_chr_fe_write_all(&s->chr, &c, sizeof(c));
> 
> No IRQ here described in the spec... So this is correct.
> 
>> +        break;
>> +    case REG_CMD:
>> +        goldfish_tty_cmd(s, value);
>> +        break;
>> +    case REG_DATA_PTR:
>> +        s->data_ptr = value;
>> +        break;
>> +    case REG_DATA_PTR_HIGH:
>> +        s->data_ptr = (value << 32) | (uint32_t)s->data_ptr;
> 
> Matter of taste, I find it easier to review as:
> 
>            s->data_ptr = deposit64(s->data_ptr, value, 32, 32);

I agree.

> 
> The spec says this register is only available on 64-bit targets.
> 
> Should we restrict this "#if TARGET_LONG_SIZE == 8"?
> else log_mask(GUEST_ERROR)...
> 
> This seems overkill to me but that would respect the spec.

TARGET_LONG_SIZE depends on CPU, I don't think we can use it here.

>> +        break;
>> +    case REG_DATA_LEN:
>> +        s->data_len = value;
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP,
>> +                      "%s: unimplemented register write 0x%02"HWADDR_PRIx"\n",
>> +                      __func__, addr);
>> +        break;
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps goldfish_tty_ops = {
>> +    .read = goldfish_tty_read,
>> +    .write = goldfish_tty_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +    .valid.max_access_size = 4,
> 
> Again, I think you also want:
> 
>        .impl.min_access_size = 4,
> 
>> +    .impl.max_access_size = 4,
>> +};

OK

> 
> With min_access_size:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 

Thanks,
Laurent