Add support for RME extension to SBSA

[mathieupoirier]

Hi maintainers,

This pull request is adding support for the RME extension to the SBSA machine. When RME is enabled, the RMM is occupying the first 1GB of system RAM, followed by the DT. This reality needs to be reflected in the base RAM address and more space in flash need to be reserved. Otherwise it is the same configuration as the non-RME aware SBSA implementation.

Thanks,
Mathieu

[hrw]

I wonder can we move PcdSystemMemoryBase by those 1072MB always.

Then check "is there RME present" and if not then add that memory as another entry in memory map. That may require SMC call to TF-A to ask for RME (or other way as I am not familiar with RME yet).

This way one firmware image would work for the platform despite which firmware options are enabled/present.

[mathieupoirier]

@hrw ok, that's a valuable point. I am completely new to EDK2, can you provide pointers to here the SMC call should be made and the memory map entry added?

[hrw]

All SMC are in Silicon/Qemu/SbsaQemu/Library/SbsaQemuHardwareInfoLib/SbsaQemuHardwareInfoLib.c on EDK2 side and in plat/qemu/qemu_sbsa/sbsa_sip_svc.c on TF-A side.

When it comes to memory map - check functions which call GetMemInfo() in Silicon/Qemu/* - on NUMA systems we use SMC to get info about all memory nodes and add them into memory map.

So maybe instead of creating copy of dsc/fdf etc:

• move memorybase to get space for RME (so you can check does it work with/without RME using hardcoded "is RME present or not")
• add GetRmeInfo() function to SbsaHardwareInfoLib (it may return hardcoded value at start)
• add SMC call to TF-A so it will report state of RME
• use above SMC call in GetRmeInfo() function (handle situation when TF-A respond with "sorry, unknown SMC" cause people will get such combos)
• if no RME then add memory reserved for RME into memory map

NOTE: TF-A 2.12 and edk202411 are coming. RME code for both of them will rather be merged after releases to make sure that we have combo which works for users.

[mathieupoirier]

When an RMM is not present, should I add the extra memory to VirtualMemoryTable[3] in ArmPlatformGetVirtualMemoryMap() and zero-out VirtualMemoryTable[4]? To do so I would obviously change MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS to 5 instead of 4.

[leiflindholm]

Apologies for late reply, caught a cold on a trip last week, just getting back into it.

While there is nothing wrong (aside from some minor naming conventions, this is CamelCaseCountry, not dash-country) with this approach, is there a particular rationale/philosophy behind creating separate .dsc/.fdf files?

As in, the syntax supports conditionals - it would have been possible to implement the same functionality to be disabled by default, but enabled (and image output names changed) within the existing files given a command line parameter.
https://github.com/tianocore/edk2-platforms/blob/master/Platform/Marvell/Cn913xDb/Cn913xDbA.dsc provides an example for the alternative solution I'm thinking about.

But clearly with/without RMM are fundamentally different platforms, just with an awful lot of overlap, so it's a valid thing to split them out to highlight. Just wondering if that was the thinking behind it :)

[leiflindholm]

This pull request is adding support for the RME extension to the SBSA machine. When RME is enabled, the RMM is occupying the first 1GB of system RAM, followed by the DT. This reality needs to be reflected in the base RAM address and more space in flash need to be reserved. Otherwise it is the same configuration as the non-RME aware SBSA implementation.

Is there any documentation on how this functions in qemu? What options affect the sbsa-ref machine memory map? (Only thing I can find on a quick grep of the qemu docs is a comment that FEAT_RME is supported but experimental.)

[hrw]

OK, we have 3 components here:

• QEMU (CPU with/without RME)
• TF-A (with/without RMM)
• EDK2

I wonder how exactly it is supposed to work and will guess a bit below.

If we boot QEMU (-m 8G) with RME enabled then TF-A notices it, starts RMM, 1GB ram is reserved for it and EDK2 starts, does own stuff and OS loads with RMM enabled and we can use RME functionality. Memory map says that 7GB ram is present.

If we boot QEMU without RME then TF-A loads, do not start RMM, EDK2 boots, notice 'no RMM' and adds that 1GB ram to memory map. OS loads. Memory map says that 8GB ram is present.

Note that I assume that Physical memory stays the same in both cases. For normal boot OS sees whole memory, for RME boot it sees whole memory - 1GB reserved for RMM.

In other words: the only "physical" part which changes is CPU.

[mathieupoirier]

To complement @hrw's description, RME support is enabled in TF-A with a compilation flag, i.e "ENABLE_RME=1". This can't be a runtime configuration as we are doing with EDK2 because the Granule Protection Table is installed at the top of the secure RAM and other components of the boot firmware, i.e BL2 and BL31, are installed below that.

[mathieupoirier]

When an RMM is not present, should I add the extra memory to VirtualMemoryTable[3] in ArmPlatformGetVirtualMemoryMap() and zero-out VirtualMemoryTable[4]? To do so I would obviously change MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS to 5 instead of 4.

Any input on this?

[leiflindholm]

To complement @hrw's description, RME support is enabled in TF-A with a compilation flag, i.e "ENABLE_RME=1". This can't be a runtime configuration as we are doing with EDK2 because the Granule Protection Table is installed at the top of the secure RAM and other components of the boot firmware, i.e BL2 and BL31, are installed below that.

Right, so this answers a question I didn't ask yet - whether we will need separate TF-A binaries in edk2-non-osi for this.

[leiflindholm]

When an RMM is not present, should I add the extra memory to VirtualMemoryTable[3] in ArmPlatformGetVirtualMemoryMap() and zero-out VirtualMemoryTable[4]? To do so I would obviously change MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS to 5 instead of 4.

Any input on this?

The MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS is kind of an ugly idiom anyway. The #define is not used for anything outside of the file, and I don't see any serious drawback in having two terminating null entries instead of one :)

But I'd ideally want to refrain from hardcoding the 1GB thing. TF-A will per definition know this, right? So the same SMC call that tells us RMM is enabled could also tell us size of reservation from start of DRAM (which we depend on hardcoding anyway)? And then we could set that dynamically and keep only one VirtualMemoryTable entry for DRAM?

[mathieupoirier]

When an RMM is not present, should I add the extra memory to VirtualMemoryTable[3] in ArmPlatformGetVirtualMemoryMap() and zero-out VirtualMemoryTable[4]? To do so I would obviously change MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS to 5 instead of 4.

Any input on this?

The MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS is kind of an ugly idiom anyway. The #define is not used for anything outside of the file, and I don't see any serious drawback in having two terminating null entries instead of one :)

But I'd ideally want to refrain from hardcoding the 1GB thing. TF-A will per definition know this, right? So the same SMC call that tells us RMM is enabled could also tell us size of reservation from start of DRAM (which we depend on hardcoding anyway)? And then we could set that dynamically and keep only one VirtualMemoryTable entry for DRAM?

The SMC call that tells us the RMM is enabled is already part of the RMM specification by way of RMI_VERSION. That part is already implemented in the TF-A and I'm simply re-using it. As such, if we get a valid ABI version from that call, the RMM is present. If not, or the call is not supported, we don't have an RMM.

But looking at the code further, we may not need to do an SMC call. The call to GetMemInfo() in SbsaQemuLibConstructor() will return the base address and size of the memory with the RMM taken into account. The only problem is the ASSERT() in SbsaQemuLibConstructor(). If we get rid of that check we should be able to have a single, dynamic, entry for DRAM in VirtualMemoryTable[]. If I am correct, are you in agreement with dropping the ASSERT()?

[leiflindholm]

The SMC call that tells us the RMM is enabled is already part of the RMM specification by way of RMI_VERSION. That part is already implemented in the TF-A and I'm simply re-using it. As such, if we get a valid ABI version from that call, the RMM is present. If not, or the call is not supported, we don't have an RMM.

Ah, right. Makes sense.

But looking at the code further, we may not need to do an SMC call. The call to GetMemInfo() in SbsaQemuLibConstructor() will return the base address and size of the memory with the RMM taken into account. The only problem is the ASSERT() in SbsaQemuLibConstructor(). If we get rid of that check we should be able to have a single, dynamic, entry for DRAM in VirtualMemoryTable[]. If I am correct, are you in agreement with dropping the ASSERT()?

I mean ... we may want to change it to a <= instead of a ==, just to sanity check that the offset from the hardwired base is positive?

[mathieupoirier]

The SMC call that tells us the RMM is enabled is already part of the RMM specification by way of RMI_VERSION. That part is already implemented in the TF-A and I'm simply re-using it. As such, if we get a valid ABI version from that call, the RMM is present. If not, or the call is not supported, we don't have an RMM.

Ah, right. Makes sense.

But looking at the code further, we may not need to do an SMC call. The call to GetMemInfo() in SbsaQemuLibConstructor() will return the base address and size of the memory with the RMM taken into account. The only problem is the ASSERT() in SbsaQemuLibConstructor(). If we get rid of that check we should be able to have a single, dynamic, entry for DRAM in VirtualMemoryTable[]. If I am correct, are you in agreement with dropping the ASSERT()?

I mean ... we may want to change it to a <= instead of a ==, just to sanity check that the offset from the hardwired base is positive?

I'll send out a new patchset based on the above conversation - we can continue from there.

[hrw]

Can you in meantime share info how to build tf-a for sbsa-ref/x-rme?

cd trusted-firmware-a && rm -rf build/qemu* && CROSS_COMPILE=aarch64-linux-gnu- ARCH=arm make -j12 PLAT=qemu_sbsa DEBUG=1  all fip ENABLE_LTO=1  ENABLE_RME=1
make[1]: Entering directory '/home/marcin/devel/linaro/sbsa-qemu/code/trusted-firmware-a'
services/std_svc/rmmd/trp/trp.mk:24: *** TRP is not supported on platform qemu_sbsa.  Stop.

[mathieupoirier]

RME support for SBSA is not yet upstream, something I will start working on after EDK2/EDK2-platforms have been dealt with. In the meantime the code is hosted here, branch "cca/v3". Note that you will also need to compile an RMM image. How to get the RMM code base and compile both RMM and TF-A, please refer to instructions here.

[hrw]

Thanks. "./boot-sbsa-ref.py --cpu max,x-rme=on" boots:

NOTICE:  Booting Trusted Firmware
NOTICE:  BL1: v2.11.0(debug):hrw-2024-08-04-2212-143-ge5fe6f949
NOTICE:  BL1: Built : 22:15:36, Oct 29 2024
INFO:    BL1: RAM 0x37fbe000 - 0x37fce000
INFO:    BL1: Loading BL2
INFO:    Loading image id=1 at address 0x37ba1000
INFO:    Image id=1 loaded: 0x37ba1000 - 0x37ba92c0
NOTICE:  BL1: Booting BL2
INFO:    Entry point address = 0x37ba1000
INFO:    SPSR = 0x3cd
NOTICE:  BL2: v2.11.0(debug):hrw-2024-08-04-2212-143-ge5fe6f949
NOTICE:  BL2: Built : 22:15:37, Oct 29 2024
INFO:    BL2: Doing platform setup
INFO:    Reserved RMM memory [0x10000000000, 0x10042ffffff] in Device tree
INFO:    BL2: Loading image id 3
INFO:    Loading image id=3 at address 0x37bbe000
INFO:    Image id=3 loaded: 0x37bbe000 - 0x37bd1168
INFO:    BL2: Loading image id 35
INFO:    Loading image id=35 at address 0x10000000000
INFO:    Image id=35 loaded: 0x10000000000 - 0x10000219820
INFO:    BL2: Skip loading image id 5
NOTICE:  BL2: Booting BL31
INFO:    Entry point address = 0x37bbe000
INFO:    SPSR = 0x3cd
INFO:    Platform version: 0.4
INFO:    GICD base = 0x40060000
INFO:    GICR base = 0x40080000
INFO:    GICI base = 0x44081000
INFO:    CPU 0: node-id: 0, mpidr: 0
INFO:    CPU 1: node-id: 0, mpidr: 1
INFO:    CPU 2: node-id: 0, mpidr: 2
INFO:    CPU 3: node-id: 0, mpidr: 3
INFO:    Found 4 cpus
INFO:    Cpu topology: sockets: 1, clusters: 1, cores: 4, threads: 1
INFO:    RAM 0: node-id: 0, address: 0x10043000000 - 0x101ffffffff
INFO:    GPT: Boot Configuration
INFO:      PPS/T:     0x3/42
INFO:      PGS/P:     0x0/12
INFO:      L0GPTSZ/S: 0x0/30
INFO:      PAS count: 5
INFO:      L0 base:   0x37fd0000
INFO:    Enabling Granule Protection Checks
NOTICE:  BL31: v2.11.0(debug):hrw-2024-08-04-2212-143-ge5fe6f949
NOTICE:  BL31: Built : 22:15:38, Oct 29 2024
INFO:    GICv3 without legacy support detected.
INFO:    ARM GICv3 driver initialized in EL3
INFO:    Maximum SPI INTID supported: 287
INFO:    BL31: Initializing runtime services
INFO:    RMM setup done.
INFO:    BL31: Initializing RMM
INFO:    RMM init start.
Booting RMM v.0.5.0(debug) 85b4a1a Built with GCC 14.2.1
RMM-EL3 Interface v.0.2
Boot Manifest Interface v.0.3
RMI/RSI ABI v.1.0/1.0 built: Oct 29 2024 22:11:54
INFO:    RMM init end.
INFO:    BL31: Preparing for EL3 exit to normal world
INFO:    Entry point address = 0x10000000
INFO:    SPSR = 0x3c9
UEFI firmware (version 1.0 built at 22:15:44 on Oct 29 2024)
INFO:    Platform version requested

[mathieupoirier]

When an RMM is not present, should I add the extra memory to VirtualMemoryTable[3] in ArmPlatformGetVirtualMemoryMap() and zero-out VirtualMemoryTable[4]? To do so I would obviously change MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS to 5 instead of 4.

Any input on this?

The MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS is kind of an ugly idiom anyway. The #define is not used for anything outside of the file, and I don't see any serious drawback in having two terminating null entries instead of one :)
But I'd ideally want to refrain from hardcoding the 1GB thing. TF-A will per definition know this, right? So the same SMC call that tells us RMM is enabled could also tell us size of reservation from start of DRAM (which we depend on hardcoding anyway)? And then we could set that dynamically and keep only one VirtualMemoryTable entry for DRAM?

The SMC call that tells us the RMM is enabled is already part of the RMM specification by way of RMI_VERSION. That part is already implemented in the TF-A and I'm simply re-using it. As such, if we get a valid ABI version from that call, the RMM is present. If not, or the call is not supported, we don't have an RMM.

But looking at the code further, we may not need to do an SMC call. The call to GetMemInfo() in SbsaQemuLibConstructor() will return the base address and size of the memory with the RMM taken into account. The only problem is the ASSERT() in SbsaQemuLibConstructor(). If we get rid of that check we should be able to have a single, dynamic, entry for DRAM in VirtualMemoryTable[]. If I am correct, are you in agreement with dropping the ASSERT()?

One unforeseen problem with the above scheme is variable PcdCPUCoresStackBase - it is used very early on in the boot process, well before any kind of information about the platform can be requested from EL3. Its value is statically set to (PcdSystemMemoryBase + 0x7c000) but that obviously won't work if PcdSystemMemoryBase becomes dynamic. I have reflected long and hard on how to address this and came up empty. I'm hoping that people more familiar with the project will be able to provide guidance. Otherwise we are back to my initial solution of using different binaries to support RME.

[leiflindholm]

I'm hoping that people more familiar with the project will be able to provide guidance.

Hmm, there are some complications here. I'll try to spend some cycles to consider possibilities on Monday.

[leiflindholm]

I'm hoping that people more familiar with the project will be able to provide guidance.

Hmm, there are some complications here. I'll try to spend some cycles to consider possibilities on Monday.

OK, none of my clever ideas seemed to work.
Can we do separate images for now, but ideally using single .dsc/.fdf with conditionals?

[mathieupoirier]

I'm hoping that people more familiar with the project will be able to provide guidance.

Hmm, there are some complications here. I'll try to spend some cycles to consider possibilities on Monday.

OK, none of my clever ideas seemed to work. Can we do separate images for now, but ideally using single .dsc/.fdf with conditionals?

Very well, we can settle on that for the time being. I will send another revision.