Re: [PATCH 1/1] hw/arm/sbsa-ref: bump default memory size to 2GB

2024-12-05 Thread Peter Maydell
On Thu, 5 Dec 2024 at 15:10, Marcin Juszkiewicz
 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



Re: [PATCH 1/1] hw/arm/sbsa-ref: bump default memory size to 2GB

2024-12-05 Thread Marcin Juszkiewicz

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.




Re: [PATCH 1/1] hw/arm/sbsa-ref: bump default memory size to 2GB

2024-12-05 Thread Peter Maydell
On Thu, 5 Dec 2024 at 10:14, Marcin Juszkiewicz
 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
>   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



Re: [PATCH 1/1] hw/arm/sbsa-ref: bump default memory size to 2GB

2024-12-05 Thread Marcin Juszkiewicz

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
 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.



Re: [PATCH 1/1] hw/arm/sbsa-ref: bump default memory size to 2GB

2024-12-04 Thread Philippe Mathieu-Daudé

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
 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



Re: [PATCH 1/1] hw/arm/sbsa-ref: bump default memory size to 2GB

2024-12-04 Thread Leif Lindholm

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
 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 




Re: [PATCH 1/1] hw/arm/sbsa-ref: bump default memory size to 2GB

2024-12-02 Thread Marcin Juszkiewicz

W dniu 26.11.2024 o 14:14, Peter Maydell pisze:

On Tue, 26 Nov 2024 at 08:49, Marcin Juszkiewicz
 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.



Re: [PATCH 1/1] hw/arm/sbsa-ref: bump default memory size to 2GB

2024-11-26 Thread Peter Maydell
On Tue, 26 Nov 2024 at 08:49, Marcin Juszkiewicz
 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