| Message ID | 20241126084928.252067-1-marcin.juszkiewicz@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] hw/arm/sbsa-ref: bump default memory size to 2GB | expand |
On Tue, 26 Nov 2024 at 08:49, Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org> wrote: > > We are working on adding RME support to SBSA Reference Platform. > When RME is enabled then RMM (Realm Managment Monitor) takes 1072MB of > memory for own use. Which ends with firmware panic on 1GB machine. Reasonable change, but isn't it also a bug in the RMM that it grabs 1GB of RAM regardless of how much RAM the machine actually has? thanks -- PMM
W dniu 26.11.2024 o 14:14, Peter Maydell pisze: > On Tue, 26 Nov 2024 at 08:49, Marcin Juszkiewicz > <marcin.juszkiewicz@linaro.org> wrote: >> >> We are working on adding RME support to SBSA Reference Platform. >> When RME is enabled then RMM (Realm Managment Monitor) takes 1072MB of >> memory for own use. Which ends with firmware panic on 1GB machine. > > Reasonable change, but isn't it also a bug in the RMM that it > grabs 1GB of RAM regardless of how much RAM the machine > actually has? I think that the goal is "get it working" first and then optimize.
On 2024-12-02 10:53, Marcin Juszkiewicz wrote: > W dniu 26.11.2024 o 14:14, Peter Maydell pisze: >> On Tue, 26 Nov 2024 at 08:49, Marcin Juszkiewicz >> <marcin.juszkiewicz@linaro.org> wrote: >>> >>> We are working on adding RME support to SBSA Reference Platform. >>> When RME is enabled then RMM (Realm Managment Monitor) takes 1072MB of >>> memory for own use. Which ends with firmware panic on 1GB machine. >> >> Reasonable change, but isn't it also a bug in the RMM that it >> grabs 1GB of RAM regardless of how much RAM the machine >> actually has? > > I think that the goal is "get it working" first and then optimize. I agree on a different platform this could feel quite hacky, but in reality even 2GB falls within "ridiculously low for an SBSA platform". If we're worried about overhead for CI jobs that do not require the feature, we could always conditionalize it on RME being enabled. But I'd be happy to wait and see. Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>
On 4/12/24 13:17, Leif Lindholm wrote: > On 2024-12-02 10:53, Marcin Juszkiewicz wrote: >> W dniu 26.11.2024 o 14:14, Peter Maydell pisze: >>> On Tue, 26 Nov 2024 at 08:49, Marcin Juszkiewicz >>> <marcin.juszkiewicz@linaro.org> wrote: >>>> >>>> We are working on adding RME support to SBSA Reference Platform. >>>> When RME is enabled then RMM (Realm Managment Monitor) takes 1072MB of >>>> memory for own use. Which ends with firmware panic on 1GB machine. >>> >>> Reasonable change, but isn't it also a bug in the RMM that it >>> grabs 1GB of RAM regardless of how much RAM the machine >>> actually has? >> >> I think that the goal is "get it working" first and then optimize. > > I agree on a different platform this could feel quite hacky, but in > reality even 2GB falls within "ridiculously low for an SBSA platform". > > If we're worried about overhead for CI jobs that do not require the > feature, we could always conditionalize it on RME being enabled. But I'd > be happy to wait and see. I'd rather do that, since it is as simple as: -- >8 -- diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c index e3195d54497..66751d0806c 100644 --- a/hw/arm/sbsa-ref.c +++ b/hw/arm/sbsa-ref.c @@ -51,6 +51,7 @@ #include "qapi/qmp/qlist.h" #include "qom/object.h" #include "target/arm/cpu-qom.h" +#include "target/arm/cpu-features.h" #include "target/arm/gtimer.h" #define RAMLIMIT_GB 8192 @@ -795,6 +796,12 @@ static void sbsa_ref_init(MachineState *machine) object_unref(cpuobj); } + if (cpu_isar_feature(aa64_rme, ARM_CPU(qemu_get_cpu(0))) + && machine->ram_size < 2 * GiB) { + error_report("sbsa-ref: RME feature requires at least 2GB of RAM"); + exit(1); + } + memory_region_add_subregion(sysmem, sbsa_ref_memmap[SBSA_MEM].base, machine->ram); --- $ ./qemu-system-aarch64 -M sbsa-ref -m 1G -cpu max,x-rme=on qemu-system-aarch64: sbsa-ref: RME feature requires at least 2GB of RAM $ ./qemu-system-aarch64 -M sbsa-ref -m 2G -cpu max,x-rme=on // OK
W dniu 4.12.2024 o 21:50, Philippe Mathieu-Daudé pisze: > On 4/12/24 13:17, Leif Lindholm wrote: >> On 2024-12-02 10:53, Marcin Juszkiewicz wrote: >>> W dniu 26.11.2024 o 14:14, Peter Maydell pisze: >>>> On Tue, 26 Nov 2024 at 08:49, Marcin Juszkiewicz >>>> <marcin.juszkiewicz@linaro.org> wrote: >>>>> >>>>> We are working on adding RME support to SBSA Reference Platform. >>>>> When RME is enabled then RMM (Realm Managment Monitor) takes 1072MB of >>>>> memory for own use. Which ends with firmware panic on 1GB machine. >>>> >>>> Reasonable change, but isn't it also a bug in the RMM that it >>>> grabs 1GB of RAM regardless of how much RAM the machine >>>> actually has? >>> >>> I think that the goal is "get it working" first and then optimize. >> >> I agree on a different platform this could feel quite hacky, but in >> reality even 2GB falls within "ridiculously low for an SBSA platform". >> >> If we're worried about overhead for CI jobs that do not require the >> feature, we could always conditionalize it on RME being enabled. But >> I'd be happy to wait and see. > > I'd rather do that, since it is as simple as: This is sbsa-ref not sbc-ref. Let it just have that 2GB of ram. None of existing SBSA systems comes with such low amount. If there is a use which is memory limited then it can set other values with "-m" argument as always. That's my opinion. I am fine with whatever option gets merged.
On Thu, 5 Dec 2024 at 10:14, Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org> wrote: > > W dniu 4.12.2024 o 21:50, Philippe Mathieu-Daudé pisze: > > On 4/12/24 13:17, Leif Lindholm wrote: > >> On 2024-12-02 10:53, Marcin Juszkiewicz wrote: > >>> W dniu 26.11.2024 o 14:14, Peter Maydell pisze: > >>>> On Tue, 26 Nov 2024 at 08:49, Marcin Juszkiewicz > >>>> <marcin.juszkiewicz@linaro.org> wrote: > >>>>> > >>>>> We are working on adding RME support to SBSA Reference Platform. > >>>>> When RME is enabled then RMM (Realm Managment Monitor) takes 1072MB of > >>>>> memory for own use. Which ends with firmware panic on 1GB machine. > >>>> > >>>> Reasonable change, but isn't it also a bug in the RMM that it > >>>> grabs 1GB of RAM regardless of how much RAM the machine > >>>> actually has? > >>> > >>> I think that the goal is "get it working" first and then optimize. > >> > >> I agree on a different platform this could feel quite hacky, but in > >> reality even 2GB falls within "ridiculously low for an SBSA platform". > >> > >> If we're worried about overhead for CI jobs that do not require the > >> feature, we could always conditionalize it on RME being enabled. But > >> I'd be happy to wait and see. > > > > I'd rather do that, since it is as simple as: > > This is sbsa-ref not sbc-ref. Let it just have that 2GB of ram. None of > existing SBSA systems comes with such low amount. Yes, I think I agree here. Changing the default RAM size based on whether the CPU does or does not have a particular feature is unusual and not something we do on other board types, and it definitely doesn't match the general intention that sbsa-ref is a "looks like real hardware" machine. The problem with using 2GB, though, is that it doesn't work on 32-bit hosts, which have a max of 2047MB (and if you try larger then hw/core/machine.c will error out with "at most 2047 MB RAM can be simulated"). As a result we have several board types which have an ifdef to say "limit the RAM size to 1GB if HOST_LONG_BITS is 32". (I think this limit primarily derives from ramptr_t being defined as a uintptr_t, but also trying to malloc 2GB on a 32-bit host is unlikely to work very well.) thanks -- PMM
W dniu 5.12.2024 o 15:53, Peter Maydell pisze: >> This is sbsa-ref not sbc-ref. Let it just have that 2GB of ram. >> None of existing SBSA systems comes with such low amount. > Yes, I think I agree here. Changing the default RAM size based on > whether the CPU does or does not have a particular feature is > unusual and not something we do on other board types, and it > definitely doesn't match the general intention that sbsa-ref is a > "looks like real hardware" machine. > > The problem with using 2GB, though, is that it doesn't work on 32- > bit hosts, which have a max of 2047MB (and Do we do CI on 32-bit hosts? RME tests (queued to testing/next by Alex) already use 2GB so in such environment we would have failure anyway.
On Thu, 5 Dec 2024 at 15:10, Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org> wrote: > > W dniu 5.12.2024 o 15:53, Peter Maydell pisze: > >> This is sbsa-ref not sbc-ref. Let it just have that 2GB of ram. > >> None of existing SBSA systems comes with such low amount. > > > Yes, I think I agree here. Changing the default RAM size based on > > whether the CPU does or does not have a particular feature is > > unusual and not something we do on other board types, and it > > definitely doesn't match the general intention that sbsa-ref is a > > "looks like real hardware" machine. > > > > The problem with using 2GB, though, is that it doesn't work on 32- > > bit hosts, which have a max of 2047MB (and > > Do we do CI on 32-bit hosts? We do at least some. In particular the qom-test tests run by 'make check' include "start every machine in its default configuration and check it doesn't fall over". This will fail for a machine that defaults to 2GB RAM on a 32-bit host. > RME tests (queued to testing/next by Alex) already use 2GB so in such > environment we would have failure anyway. I think we probably don't run the 'check-functional' tests on 32-bit hosts. -- PMM
diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c index e3195d5449..2195465202 100644 --- a/hw/arm/sbsa-ref.c +++ b/hw/arm/sbsa-ref.c @@ -901,7 +901,7 @@ static void sbsa_ref_class_init(ObjectClass *oc, void *data) mc->block_default_type = IF_IDE; mc->no_cdrom = 1; mc->default_nic = "e1000e"; - mc->default_ram_size = 1 * GiB; + mc->default_ram_size = 2 * GiB; mc->default_ram_id = "sbsa-ref.ram"; mc->default_cpus = 4; mc->smp_props.clusters_supported = true;
We are working on adding RME support to SBSA Reference Platform. When RME is enabled then RMM (Realm Managment Monitor) takes 1072MB of memory for own use. Which ends with firmware panic on 1GB machine. Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org> --- hw/arm/sbsa-ref.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)