Re: [PATCH 2/2] hw: aspeed: Init all UART's with serial devices

[Peter Delevoryas]

> On May 12, 2022, at 10:31 PM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> On 5/13/22 06:02, Peter Delevoryas wrote:
>> Usually, QEMU users just provide one serial device on the command line,
>> either through "-nographic" or "-serial stdio -display none", or just using
>> VNC and popping up a window. We try to match what the user expects, which is
>> to connect the first (and usually only) serial device to the UART a board is
>> using as serial0.
>> Most Aspeed machines in hw/arm/aspeed.c use UART5 for serial0 in their
>> device tree, so we connect UART5 to the first serial device. Some machines
>> use UART1 though, or UART3, so the uart_default property lets us specify
>> that in a board definition.
>> In order to specify a nonstandard serial0 UART, a user basically *must* add
>> a new board definition in hw/arm/aspeed.c. There's no way to do this without
>> recompiling QEMU, besides constructing the machine completely from scratch
>> on the command line.
>> To provide more flexibility, we can also support the user specifying more
>> serial devices, and connect them to the UART memory regions if possible.
>> Even if a user doesn't specify any extra serial devices, it's useful to
>> initialize these memory regions as UART's, so that they respond to the guest
>> OS more naturally. At the moment, they will just always return zero's for
>> everything, and some UART registers have a default non-zero state.
>> With this change, if a new OpenBMC image uses UART3 or some other
>> nonstandard UART for serial0, you can still use it with the EVB without
>> recompiling QEMU, even though uart-default=UART5 for the EVB.
>> For example, Facebook's Wedge100 BMC uses UART3: you can fetch an image from
>> Github[1] and get the serial console output even while running the palmetto
>> machine type, because we explicitly specify that we want UART3 to be
>> connected to stdio.
>> qemu-system-arm -machine palmetto-bmc \
>> -drive file=wedge100.mtd,format=raw,if=mtd \
>> -serial null -serial null -serial null -serial stdio -display none
>> Similarly, you can boot a Fuji BMC image[2], which uses UART1, using the
>> AST2600 EVB machine:
>> qemu-system-arm -machine ast2600-evb \
>> -drive file=fuji.mtd,format=raw,if=mtd \
>> -serial null -serial stdio -display none
>> This is kind of complicated, of course: it might be more natural to get rid
>> of the uart_default attribute completely, and initialize UART's
>> sequentially. But, keeping backward compatibility and the way most users
>> know how to use QEMU in mind, this seems to make the most sense.
>> [1] 
>> https://github.com/facebook/openbmc/releases/download/v2021.49.0/wedge100.mtd
>> [2] https://github.com/facebook/openbmc/releases/download/v2021.49.0/fuji.mtd
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>> ---
>> hw/arm/aspeed_ast10x0.c | 14 +++++++++++---
>> hw/arm/aspeed_ast2600.c | 10 +++++++++-
>> hw/arm/aspeed_soc.c | 10 +++++++++-
>> 3 files changed, 29 insertions(+), 5 deletions(-)
>> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
>> index f65dc139da..5e6f3a8fed 100644
>> --- a/hw/arm/aspeed_ast10x0.c
>> +++ b/hw/arm/aspeed_ast10x0.c
>> @@ -215,10 +215,18 @@ static void aspeed_soc_ast1030_realize(DeviceState 
>> *dev_soc, Error **errp)
>> qdev_get_gpio_in(DEVICE(&s->armv7m),
>> sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_4));
>> - /* UART5 - attach an 8250 to the IO space as our UART */
>> - serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
>> - aspeed_soc_get_irq(s, ASPEED_DEV_UART5),
>> + /* UART - attach 8250's to the IO space for each UART */
>> + serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
>> + aspeed_soc_get_irq(s, s->uart_default),
> 
> That's a fix for aspeed_ast10x0 that should come first.

Good point, I’ll separate this into another patch in the series instead
of doing it right here.

> 
>> 38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
>> + for (int i = 1, uart = ASPEED_DEV_UART1; i < 13; i++, uart++) {
> 
> '13' should be a AspeecSoCClass attribute. The number of uarts varies
> depending on the SoC model and we might want to model that one day.

True, I’ll add a patch to the series that includes that.

> 
>> + if (uart == s->uart_default) {
>> + uart++;
>> + }
> 
> Shouldn't we test serial_hd(i) validity ?

I was actually intentionally skipping that. If serial_hd(i)
doesn’t exist, the function will return NULL.

Chardev *serial_hd(int i)
{
    assert(i >= 0);
    if (i < num_serial_hds) {
        return serial_hds[i];
    }
    return NULL;
}

So then, the serial device’s CharBackend’s “Chardev *chr”
will be initialized as NULL. Looking at all of the
usage of this attribute in “hw/char/serial.c”, I think
that’s ok, the read/write functions will just be no-ops.
They all have guards for “chr == NULL”. Take this one
as an example:

int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len)
{
    Chardev *s = be->chr;

    if (!s) {
        return 0;
    }

    return qemu_chr_write(s, buf, len, false);
}

On the other hand, most of the rest of the serial device
code will run, include setting and clearing the line
status register and stuff like that. In some FB code[1] using
UART’s, processes will actually go to 100% CPU usage in QEMU
polling the line status register if it doesn’t have the
transmitter-empty bit set, mostly because the author didn’t write
fault-tolerant code, but also because it doesn’t align with how
the hardware behaves by default (I think). So, that was my
motivation for initializing serial devices, but not always
connecting a chardev backend. But I’m open to other
interpretations of how things should be setup too.

If you’d like me to only initialize a UART if a chardev backend
is provided for it, then to satisfy my use case, I would
just always make sure our test infrastructure runs QEMU
with all serial devices connected to chardevs. So, either way,
this change is still useful, and will satisfy my requirements.

Thanks,
Peter

[1] 
https://github.com/facebook/openbmc/blob/036586d1995ea8ce06b9fa56cc357741e9f0bb90/common/recipes-core/rackmon/rackmon/modbus.c#L69

> 
> Thanks,
> 
> C.
> 
>> + serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
>> + aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
>> + DEVICE_LITTLE_ENDIAN);
>> + }
>> /* Timer */
>> object_property_set_link(OBJECT(&s->timerctrl), "scu", OBJECT(&s->scu),
>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
>> index 1b72800682..cbeca7f655 100644
>> --- a/hw/arm/aspeed_ast2600.c
>> +++ b/hw/arm/aspeed_ast2600.c
>> @@ -372,10 +372,18 @@ static void aspeed_soc_ast2600_realize(DeviceState 
>> *dev, Error **errp)
>> sysbus_connect_irq(SYS_BUS_DEVICE(&s->adc), 0,
>> aspeed_soc_get_irq(s, ASPEED_DEV_ADC));
>> - /* UART - attach an 8250 to the IO space as our UART */
>> + /* UART - attach 8250's to the IO space for each UART */
>> serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
>> aspeed_soc_get_irq(s, s->uart_default), 38400,
>> serial_hd(0), DEVICE_LITTLE_ENDIAN);
>> + for (int i = 1, uart = ASPEED_DEV_UART1; i < 13; i++, uart++) {
>> + if (uart == s->uart_default) {
>> + uart++;
>> + }
>> + serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
>> + aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
>> + DEVICE_LITTLE_ENDIAN);
>> + }
>> /* I2C */
>> object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
>> index 2cd03d49da..1fc1ed808d 100644
>> --- a/hw/arm/aspeed_soc.c
>> +++ b/hw/arm/aspeed_soc.c
>> @@ -303,10 +303,18 @@ static void aspeed_soc_realize(DeviceState *dev, Error 
>> **errp)
>> sysbus_connect_irq(SYS_BUS_DEVICE(&s->adc), 0,
>> aspeed_soc_get_irq(s, ASPEED_DEV_ADC));
>> - /* UART - attach an 8250 to the IO space as our UART */
>> + /* UART - attach 8250's to the IO space for each UART */
>> serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
>> aspeed_soc_get_irq(s, s->uart_default), 38400,
>> serial_hd(0), DEVICE_LITTLE_ENDIAN);
>> + for (int i = 1, uart = ASPEED_DEV_UART1; i < 5; i++, uart++) {
>> + if (uart == s->uart_default) {
>> + uart++;
>> + }
>> + serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
>> + aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
>> + DEVICE_LITTLE_ENDIAN);
>> + }
>> /* I2C */
>> object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),