Re: [PATCH 1/1] hw/arm/sbsa-ref: add PCIe node into DT

[Leif Lindholm]

On 2023-06-27 15:27, Peter Maydell wrote:
> > Serious question: would it be preferable if we moved to a custom DT node
> > where we stick everything in as KEY=VALUE pairs to reduce this confusion?
>
> I don't really mind, I just want it to be clear what is going on here
> so that when I'm reviewing patches I have a design I can keep in mind.

No, I totally understand that. I'm just feeling that the call we made to 
use DT to pass this information tends to put everyone's mind into the 
wrong state when reviewing.

> The way this was presented to me originally, at least as I recall
> it, was "this board will work the way that real hardware does, ie
> the firmware knows what hardware it was built for".
>
> In that
> setup QEMU doesn't need to tell the firmware anything, except
> a very limited set of things which it's more convenient to have
> flexible and specifiable on the QEMU command line, like number of
> CPUs and size of RAM. And that's what the comments in the source say
> at the moment:
>
> /*
>   * Firmware on this machine only uses ACPI table to load OS, these limited
>   * device tree nodes are just to let firmware know the info which varies from
>   * command line parameters, so it is not necessary to be fully compatible
>   * with the kernel CPU and NUMA binding rules.
>   */
>
> So that's the design I've been implicitly reviewing these changes
> against.

I still agree fully with the above. But possibly I meant less by what I 
said than you heard. Human language, eh?

The only things that *need* hardcoding really are:
- What (family of) platform(s) is this? (sbsa-ref)
- Start of Secure memory.
- Start of Non-secure memory.
- Mechanism by which to receive platform configuration data that might
  differ at runtime.

Admittedly, using QEMU gives us more flexibility than would be likely in 
a real platform - like specifying any supported CPU that can access the 
whole address map. A real platform would be very unlikely to have 
runtime detection for everything from Cortex-A57 to Neoverse-N2 - but 
it's genuinely useful for our (non-platform-specific) firmware 
development to be able to do that.

> It is pretty surprising to me to hear that in real-world systems
> the firmware is not built to know exactly where its UART, USB
> controller, etc are and that it is instead asking some board
> management controller chip for all this information and being
> fully-flexible in the firmware that runs on the application CPU,
> but I have zero experience in that area so that's just my
> lack of knowledge speaking.

As an example, in said previous design we were prototyping having the 
ability to hold PCIe space inside 48 bits (to work with non-LPA2-aware
operating systems utilising 4k pages) and having a software-configurable 
option to expand into 52 bits (enabling more space for both DRAM and 
PCIe), where an LPA2-aware (or 64k-paged) OS was used.

The addresses (from the application processor's perspective) of certain 
other i/o blocks were also ultimately decided by a microcontroller 
programming the CMN. So even if we "decided" on locations for them and 
stuck to those for simplicity, they weren't actually hard-wired.

> If there's a standard/common protocol for how the BMC communicates
> that info to the application-CPU firmware then it might be
> less confusing to use it, I guess. But I'm not inherently
> opposed to putting this stuff in a dtb-format blob.

Not really. If anything I'm hoping to inspire standardisation along 
those axes by having libraries available to just plug in.

> (Side note: is the commit message line "Trusted Firmware will
> read it and provide to next firmware level." intended to
> mean "TF will take this dtb node and pass it on", or merely
> "TF will take the information in this dtb node and use
> it to construct or modify the ACPI tables it passes to the
> next level"?)

The latter.
We're kind of working our way backwards towards the design we ultimately 
want, so the DT was originally placed in NS DRAM since that let us reuse 
more of the mach-virt code in edk2 rather than needing to rewrite 
everything to get anything working.

Now we're getting versioning in place, we will eventually deprecate that 
and move the DT to Secure DRAM, dropping access to it for Non-secure 
firmware. But we never exposed the DT to the OS as a configuration 
table, it was always used to generate the bits of ACPI that weren't just 
hardcoded.

/
    Leif



/
    Leif