Re: [PATCH v6 25/37] board: emulation: Add QEMU sbsa support

Tom Rini
2024-10-03T19:18:02Z

[-- Attachment #1: Type: text/plain, Size: 11229 bytes --]

On Thu, Oct 03, 2024 at 10:59:40AM -0600, Simon Glass wrote:
> Hi Tom,
>
> On Thu, 3 Oct 2024 at 08:43, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Oct 03, 2024 at 07:50:45AM -0600, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Wed, 2 Oct 2024 at 19:35, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Wed, Oct 02, 2024 at 04:55:34PM -0600, Simon Glass wrote:
> > > > > Hi Patrick,
> > > > >
> > > > > On Wed, 2 Oct 2024 at 03:52, Patrick Rudolph
> > > > > <patrick.rudolph@9elements.com> wrote:
> > > > > >
> > > > > > Add support for Arm sbsa [1] v0.3+ that is supported by QEMU [2].
> > > > > >
> > > > > > Unlike other Arm based platforms the machine only provides a minimal
> > > > > > FDT that contains number of CPUs, ammount of memory and machine-version.
> > > > > > The boot firmware has to provide ACPI tables to the OS.
> > > > > > Due to this design a full DTB is added here as well that allows U-Boot's
> > > > > > driver to properly function. The DTB is appended at the end of the U-Boot
> > > > > > image and will be merged with the QEMU provided DTB.
> > > > > >
> > > > > > In addition provide documentation how to use, enable binman to fabricate both
> > > > > > ROMs that are required to boot and add ACPI tables to make it full compatible
> > > > > > to the EDK2 reference implementation.
> > > > > >
> > > > > > The board was tested using Fedora 40 Aarch64 Workstation. It's able
> > > > > > to boot from USB and AHCI or network.
> > > > > >
> > > > > > Tested and found working:
> > > > > > - serial
> > > > > > - PCI
> > > > > > - xHCI
> > > > > > - Bochs display
> > > > > > - AHCI
> > > > > > - network using e1000e
> > > > > > - CPU init
> > > > > > - Booting Fedora 40
> > > > > >
> > > > > > 1: Server Base System Architecture (SBSA)
> > > > > > 2: https://www.qemu.org/docs/master/system/arm/sbsa.html
> > > > > >
> > > > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > > > > Cc: Peter Robinson <pbrobinson@gmail.com>
> > > > > > Cc: Simon Glass <sjg@chromium.org>
> > > > > > Cc: Tom Rini <trini@konsulko.com>
> > > > > > ---
> > > > > > Changelog v3:
> > > > > > - Add GIC and GIC-ITS to devicetree
> > > > > > - Select GICv3 driver
> > > > > > - Drop acpi_fill_madt and use driver model instead
> > > > > > Changelog v4:
> > > > > > - Drop CPU platform code
> > > > > > - Enhance the DT to allow MADT generation from DT
> > > > > > Changelog v5:
> > > > > > - Add full DT and place it at the end of U-Boot
> > > > > > - Merge DT with QEMU's DT
> > > > > > - Drop DT generation code
> > > > > > - Fix flash region length
> > > > > > - Drop enable_caches()
> > > > > > - Support platforms that do not pass FDT in x0
> > > > > > Changelog v6:
> > > > > > - Update header order
> > > > > > - Drop pad-byte from DT
> > > > > > - select BINMAN_FDT
> > > > > > - select E1000_NO_NVM
> > > > > > - drop config.h include
> > > > > > - drop a few CFG_ defines that were used for SPL
> > > > > >
> > > > > > ---
> > > > > > arch/arm/Kconfig | 3 +-
> > > > > > arch/arm/dts/qemu-sbsa.dts | 136 ++++++
> > > > > > arch/arm/include/asm/arch-qemu-sbsa/boot0.h | 34 ++
> > > > > > arch/arm/mach-qemu/Kconfig | 36 +-
> > > > > > board/emulation/qemu-arm/MAINTAINERS | 2 +
> > > > > > board/emulation/qemu-sbsa/Kconfig | 57 +++
> > > > > > board/emulation/qemu-sbsa/Makefile | 8 +
> > > > > > board/emulation/qemu-sbsa/acpi.c | 192 ++++++++
> > > > > > board/emulation/qemu-sbsa/dsdt.asl | 483 ++++++++++++++++++++
> > > > > > board/emulation/qemu-sbsa/lowlevel_init.S | 22 +
> > > > > > board/emulation/qemu-sbsa/qemu-sbsa.c | 311 +++++++++++++
> > > > > > board/emulation/qemu-sbsa/qemu-sbsa.env | 14 +
> > > > > > board/emulation/qemu-sbsa/qemu-sbsa.h | 38 ++
> > > > > > board/emulation/qemu-sbsa/smc.c | 71 +++
> > > > > > configs/qemu-arm-sbsa_defconfig | 10 +
> > > > > > doc/board/emulation/index.rst | 1 +
> > > > > > doc/board/emulation/qemu-sbsa.rst | 98 ++++
> > > > > > doc/develop/driver-model/virtio.rst | 1 +
> > > > > > include/configs/qemu-sbsa.h | 95 ++++
> > > > > > 19 files changed, 1605 insertions(+), 7 deletions(-)
> > > > > > create mode 100644 arch/arm/dts/qemu-sbsa.dts
> > > > > > create mode 100644 arch/arm/include/asm/arch-qemu-sbsa/boot0.h
> > > > > > create mode 100644 board/emulation/qemu-sbsa/Kconfig
> > > > > > create mode 100644 board/emulation/qemu-sbsa/Makefile
> > > > > > create mode 100644 board/emulation/qemu-sbsa/acpi.c
> > > > > > create mode 100644 board/emulation/qemu-sbsa/dsdt.asl
> > > > > > create mode 100644 board/emulation/qemu-sbsa/lowlevel_init.S
> > > > > > create mode 100644 board/emulation/qemu-sbsa/qemu-sbsa.c
> > > > > > create mode 100644 board/emulation/qemu-sbsa/qemu-sbsa.env
> > > > > > create mode 100644 board/emulation/qemu-sbsa/qemu-sbsa.h
> > > > > > create mode 100644 board/emulation/qemu-sbsa/smc.c
> > > > > > create mode 100644 configs/qemu-arm-sbsa_defconfig
> > > > > > create mode 100644 doc/board/emulation/qemu-sbsa.rst
> > > > > > create mode 100644 include/configs/qemu-sbsa.h
> > > > >
> > > > > With the tweaks below:
> > > > >
> > > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > >
> > > > > [..]
> > > > >
> > > > > > diff --git a/arch/arm/dts/qemu-sbsa.dts b/arch/arm/dts/qemu-sbsa.dts
> > > > > > new file mode 100644
> > > > > > index 0000000000..f743ea3863
> > > > > > --- /dev/null
> > > > > > +++ b/arch/arm/dts/qemu-sbsa.dts
> > > > > > @@ -0,0 +1,136 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0+ OR MIT
> > > > > > +/*
> > > > > > + * Devicetree with onboard devices for qemu_sbsa-ref for internal use only!
> > > > > > + * DO NOT PASS TO THE OS!
> > > > > > + *
> > > > > > + * As QEMU provides only a minimal devicetree this one is merged with
> > > > > > + * it and then fixed at runtime.
> > > > > > + *
> > > > > > + * Copyright 2024 9elements GmbH
> > > > > > + */
> > > > > > +#include "configs/qemu-sbsa.h"
> > > > > > +
> > > > > > +/dts-v1/;
> > > > > > +
> > > > > > +#define MAKE_PROP(x) (x>>32) (x&0xffffffff)
> > > > > > +
> > > > > > +/ {
> > > > > > + #address-cells = <2>;
> > > > > > + #size-cells = <2>;
> > > > > > + interrupt-parent = <&intc>;
> > > > > > + compatible = "linux,sbsa-ref";
> > > > > > +
> > > > > > + binman: binman {
> > > > > > + multiple-images;
> > > > > > + };
> > > > > > +
> > > > > > + cpus {
> > > > > > + /* Filled by fdtdec_board_setup() */
> > > > > > + };
> > > > > > +
> > > > > > + memory {
> > > > > > + /* Filled by fdtdec_board_setup() */
> > > > > > + };
> > > > > > +
> > > > > > + soc {
> > > > > > + compatible = "simple-bus";
> > > > > > + #address-cells = <2>;
> > > > > > + #size-cells = <2>;
> > > > > > + ranges;
> > > > > > +
> > > > > > + uart0 {
> > > > > > + compatible = "arm,pl011";
> > > > > > + status = "okay";
> > > > > > + reg = <MAKE_PROP(SBSA_UART_BASE_ADDR)
> > > > >
> > > > > Can you use
> > > > >
> > > > > reg = /bits 64/ <0x60000000>
> > > > >
> > > > > instead?
> > > > >
> > > > > Same for those below.
> > > >
> > > > Isn't that less readable ?
> > >
> > > Which bit of it is less readable?
> >
> > Having hex spelled out rather than a macro that's clear what's being
> > generated.
>
> To my eye it seems odd to define a base address in a macro only to
> have it used once in a devicetree. We don't do that elsewhere.

We don't, but this isn't quite a normal board?

> Breaking down the SBSA_UART_BASE_ADDR:
>
> - this is in the uart0 node so obviously relates to the UART
> - it is a reg property, so BASE_ADDR is obvious
> - SBSA is the board name, which is in the filename, so don't really add anything
>
> Normally where the devicetree needs a value defined, we put it in the
> dt-bindings directory. But even then, we don't typically put addresses
> there. It is just for cases where we need the code and the devicetree
> to be in sync about a property value. With addresses, the code reads
> them from the devicetree, so doesn't need to have a #define to access
> them...in fact that would defeat the purpose.
>
> If we did this sort of thing with other boards, we would have a heap
> of binding files containing addresses that are only used in one place.
> I just don't see the point.
>
> However for SBSA_PCIE_MMIO_BASE_ADDR I see that it is used in the .asl
> and the memory map, so it seems fair enough to define that, at least
> unless/until we can refer to devicetree addresses in the .asl file.
> Even with this one though, I would expect something like:
>
> ranges = <0x02000000>, /bits/ 64 <SBSA_PCIE_MMIO_BASE_ADDR
>
> SBSA_PCIE_MMIO_BASE_ADDR>;
>
> But in this case I can see how a macro might be more convenient and
> I'm not sure I have a strong opinion here.

If you're strongly against using the macros here, OK, fine. But since
this tree is only going to be read by humans in U-Boot, and consumed by
U-Boot, I think it makes sense to deviate from the norms a little, in
favour of human readability.

> > > But I very much doubt upstream would accept it with a macro. Other
> > > 64-bit boards seem to use '/bits/ 64'.
> >
> > What upstream are you referring to here? I'm not sure what other project
> > will want a device tree for the "no device tree only acpi" virtual
> > reference platform.
>
> It depends if they want to run U-Boot or not. U-Boot always needs a
> devicetree, as you know. It cannot use ACPI tables to configure itself
> and I would rather it doesn't start.
>
> I don't actually see any reason why this devicetree could not be
> upstreamed, except that QEMU actually creates a Linux devicetree right
> now (on the ARM side) and takes no notice of what U-Boot needs... I
> don't see any U-Boot patches from the maintainers, for example,
> although I may have missed some. It seems very focussed on UEFI. I
> would love that to change, as QEMU is widely used in U-Boot on ARM...

I feel like we're lost the point here. This platform in question is the
"never use device tree, only use ACPI" reference platform. This is not
the regular "virt" platform, even. U-Boot needs a device tree because
internally we use it for configuration. I do not see the QEMU project
agreeing to have this platform generate a device tree for us to consume
in this case, declaring instead that it's a U-Boot internal need and
that no, it's fine if U-Boot can't use it. _We_ want it because it makes
testing of the aarch64 + ACPI use case easy because it provides us with
the ability to use the reference platform too and make sure that we
aren't breaking the OS in unexpected ways.

--
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]