[Qemu-devel] [PATCH v3] spapr: make sure RMA is in first mode of first memory node

[Qemu-devel] [PATCH v3] spapr: make sure RMA is in first mode of first memory node

[Alexey Kardashevskiy]

The SPAPR specification says that the RMA starts at the LPAR's logical
address 0 and is the first logical memory block reported in
the LPAR’s device tree.

So SLOF only maps the first block and that block needs to span
the full RMA.

This makes sure that the RMA area is where SLOF expects it.

Cc: Thomas Huth <thuth@linux.vnet.ibm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v3:
* removed unnecessary RMA fixup from spapr_populate_memory()

v2:
* changed as recommended by Alex Graf
---
 hw/ppc/spapr.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 7e53a5f..1f320f6 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -532,9 +532,6 @@ static int spapr_populate_memory(sPAPREnvironment *spapr, void *fdt)
 
     /* memory node(s) */
     node0_size = (nb_numa_nodes > 1) ? node_mem[0] : ram_size;
-    if (spapr->rma_size > node0_size) {
-        spapr->rma_size = node0_size;
-    }
 
     /* RMA */
     mem_reg_property[0] = 0;
@@ -1113,6 +1110,7 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     hwaddr rma_alloc_size;
+    hwaddr node0_size = (nb_numa_nodes > 1) ? node_mem[0] : ram_size;
     uint32_t initrd_base = 0;
     long kernel_size = 0, initrd_size = 0;
     long load_limit, rtas_limit, fw_size;
@@ -1137,7 +1135,7 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
     if (rma_alloc_size && (rma_alloc_size < ram_size)) {
         spapr->rma_size = rma_alloc_size;
     } else {
-        spapr->rma_size = ram_size;
+        spapr->rma_size = node0_size;
 
         /* With KVM, we don't actually know whether KVM supports an
          * unbounded RMA (PR KVM) or is limited by the hash table size
@@ -1154,6 +1152,12 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
         }
     }
 
+    if (spapr->rma_size > node0_size) {
+        fprintf(stderr, "Error: Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")\n",
+                spapr->rma_size);
+        exit(1);
+    }
+
     /* We place the device tree and RTAS just below either the top of the RMA,
      * or just below 2GB, whichever is lowere, so that it can be
      * processed with 32-bit real mode code if necessary */
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH v3] spapr: make sure RMA is in first mode of first memory node

[Thomas Huth]

On Thu,  7 Nov 2013 14:50:00 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> The SPAPR specification says that the RMA starts at the LPAR's logical
> address 0 and is the first logical memory block reported in
> the LPAR’s device tree.
> 
> So SLOF only maps the first block and that block needs to span
> the full RMA.
> 
> This makes sure that the RMA area is where SLOF expects it.
> 
> Cc: Thomas Huth <thuth@linux.vnet.ibm.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v3:
> * removed unnecessary RMA fixup from spapr_populate_memory()
> 
> v2:
> * changed as recommended by Alex Graf
> ---
>  hw/ppc/spapr.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7e53a5f..1f320f6 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -532,9 +532,6 @@ static int spapr_populate_memory(sPAPREnvironment *spapr, void *fdt)
> 
>      /* memory node(s) */
>      node0_size = (nb_numa_nodes > 1) ? node_mem[0] : ram_size;
> -    if (spapr->rma_size > node0_size) {
> -        spapr->rma_size = node0_size;
> -    }
> 
>      /* RMA */
>      mem_reg_property[0] = 0;
> @@ -1113,6 +1110,7 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>      MemoryRegion *sysmem = get_system_memory();
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>      hwaddr rma_alloc_size;
> +    hwaddr node0_size = (nb_numa_nodes > 1) ? node_mem[0] : ram_size;
>      uint32_t initrd_base = 0;
>      long kernel_size = 0, initrd_size = 0;
>      long load_limit, rtas_limit, fw_size;
> @@ -1137,7 +1135,7 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>      if (rma_alloc_size && (rma_alloc_size < ram_size)) {

Uh, sorry for not seeing this earlier, ... but should that be
"rma_alloc_size < node0_size" instead of "rma_alloc_size < ram_size"?
For example, if rma_alloc_size = 256 MB, ram_size = 512 MB but
node0_size = 128 MB, this will set rma_size to 256 MB ...

>          spapr->rma_size = rma_alloc_size;
>      } else {
> -        spapr->rma_size = ram_size;
> +        spapr->rma_size = node0_size;
> 
>          /* With KVM, we don't actually know whether KVM supports an
>           * unbounded RMA (PR KVM) or is limited by the hash table size
> @@ -1154,6 +1152,12 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>          }
>      }
> 
> +    if (spapr->rma_size > node0_size) {
> +        fprintf(stderr, "Error: Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")\n",
> +                spapr->rma_size);
> +        exit(1);
> +    }

... so in the above example you would get the error + exit here.
Or did I miss something?

 Thomas

Re: [Qemu-devel] [PATCH v3] spapr: make sure RMA is in first mode of first memory node

[Alexander Graf]

On 07.11.2013, at 09:33, Thomas Huth <thuth@linux.vnet.ibm.com> wrote:

> On Thu,  7 Nov 2013 14:50:00 +1100
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
>> The SPAPR specification says that the RMA starts at the LPAR's logical
>> address 0 and is the first logical memory block reported in
>> the LPAR’s device tree.
>> 
>> So SLOF only maps the first block and that block needs to span
>> the full RMA.
>> 
>> This makes sure that the RMA area is where SLOF expects it.
>> 
>> Cc: Thomas Huth <thuth@linux.vnet.ibm.com>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v3:
>> * removed unnecessary RMA fixup from spapr_populate_memory()
>> 
>> v2:
>> * changed as recommended by Alex Graf
>> ---
>> hw/ppc/spapr.c | 12 ++++++++----
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 7e53a5f..1f320f6 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -532,9 +532,6 @@ static int spapr_populate_memory(sPAPREnvironment *spapr, void *fdt)
>> 
>>     /* memory node(s) */
>>     node0_size = (nb_numa_nodes > 1) ? node_mem[0] : ram_size;
>> -    if (spapr->rma_size > node0_size) {
>> -        spapr->rma_size = node0_size;
>> -    }
>> 
>>     /* RMA */
>>     mem_reg_property[0] = 0;
>> @@ -1113,6 +1110,7 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>>     MemoryRegion *sysmem = get_system_memory();
>>     MemoryRegion *ram = g_new(MemoryRegion, 1);
>>     hwaddr rma_alloc_size;
>> +    hwaddr node0_size = (nb_numa_nodes > 1) ? node_mem[0] : ram_size;
>>     uint32_t initrd_base = 0;
>>     long kernel_size = 0, initrd_size = 0;
>>     long load_limit, rtas_limit, fw_size;
>> @@ -1137,7 +1135,7 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>>     if (rma_alloc_size && (rma_alloc_size < ram_size)) {
> 
> Uh, sorry for not seeing this earlier, ... but should that be
> "rma_alloc_size < node0_size" instead of "rma_alloc_size < ram_size"?
> For example, if rma_alloc_size = 256 MB, ram_size = 512 MB but
> node0_size = 128 MB, this will set rma_size to 256 MB ...
> 
>>         spapr->rma_size = rma_alloc_size;
>>     } else {
>> -        spapr->rma_size = ram_size;
>> +        spapr->rma_size = node0_size;
>> 
>>         /* With KVM, we don't actually know whether KVM supports an
>>          * unbounded RMA (PR KVM) or is limited by the hash table size
>> @@ -1154,6 +1152,12 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>>         }
>>     }
>> 
>> +    if (spapr->rma_size > node0_size) {
>> +        fprintf(stderr, "Error: Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")\n",
>> +                spapr->rma_size);
>> +        exit(1);
>> +    }
> 
> ... so in the above example you would get the error + exit here.
> Or did I miss something?

No, nice catch :). I'm really eagerly awaiting wide public availability of HV capable POWER hardware so that we can finally get rid of all the 970 code...


Alex