| Message ID | 20220513040220.3657135-3-pdel@fb.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw: aspeed: Init all UART's with serial devices | expand |
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. > 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. > + if (uart == s->uart_default) { > + uart++; > + } Shouldn't we test serial_hd(i) validity ? 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),
> 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),
On 5/13/22 23:08, Peter Delevoryas wrote: > > >> 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. The problem is that it is breaking compatibility with previous QEMUs. We can not change the command line to : qemu-system-arm -machine palmetto-bmc \ -drive file=wedge100.mtd,format=raw,if=mtd \ -serial null -serial null -serial null -serial stdio -display none What about adding a machine "uart" option ? like we did for the fmc/spi models Thanks, C.
> On May 14, 2022, at 12:30 AM, Cédric Le Goater <clg@kaod.org> wrote: > > On 5/13/22 23:08, Peter Delevoryas wrote: >>> 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. > > The problem is that it is breaking compatibility with previous QEMUs. It is? We can still run things the old way too, I specifically wrote this with the intention that it would support backwards compatibility. The first “-serial” argument is connected to the “s->uart_default” UART. Then the remaining “-serial” arguments (if they are present at all) are connected sequentially to the remaining UART’s. If only one -serial argument is provided, or a user uses -nographic/etc, then it just does what it has always done. It should be completely optional. The only change in behavior is that now more UART’s will be initialized, so if some guest OS was depending on the line status register for an unconnected UART always reading “transmit buffer not-empty”, then they will be in trouble. But that’s not how the hardware would work normally anyways. > We can not change the command line to : > > qemu-system-arm -machine palmetto-bmc \ > -drive file=wedge100.mtd,format=raw,if=mtd \ > -serial null -serial null -serial null -serial stdio -display none > > What about adding a machine "uart" option ? like we did for the fmc/spi > models > > Thanks, > > C.
[ ... ] >> The problem is that it is breaking compatibility with previous QEMUs. > > It is? We can still run things the old way too, I specifically > wrote this with the intention that it would support backwards > compatibility. You are right. Let's start with your patchset. We can add the "uart" machine option when the need arises. I have sent a small cleanup of aspeed_soc_get_irq() that should avoid the duplication of the serial init in the different SoC models. Please give it a try. Thanks, C.
> On May 15, 2022, at 2:19 PM, Cédric Le Goater <clg@kaod.org> wrote: > > [ ... ] > >>> The problem is that it is breaking compatibility with previous QEMUs. >> It is? We can still run things the old way too, I specifically >> wrote this with the intention that it would support backwards >> compatibility. > > You are right. Let's start with your patchset. We can add the "uart" > machine option when the need arises. > > I have sent a small cleanup of aspeed_soc_get_irq() that should avoid > the duplication of the serial init in the different SoC models. Please > give it a try. > Oh this is fantastic, thanks! I’ll gladly put the serial init code into a function declared in aspeed_soc.h then. > Thanks, > > C. >
On Fri, 13 May 2022 at 22:09, Peter Delevoryas <pdel@fb.com> wrote > 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”. Yes, this is deliberate. We added these in commit 12051d82f0040 because otherwise lots of board/SoC code would have to create NullChardev dummy backends (or forget to and then crash depending on the user's commandline). thanks -- PMM
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), 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); + } /* 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),
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(-)