diff mbox series

[05/26] ppc/pnv: Distribute RAM among the chips

Message ID 20210809134547.689560-6-clg@kaod.org (mailing list archive)
State New, archived
Headers show
Series ppc/pnv: Extend the powernv10 machine | expand

Commit Message

Cédric Le Goater Aug. 9, 2021, 1:45 p.m. UTC 
But always give the first 1GB to chip 0 as skiboot requires it.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/pnv.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

Comments

Greg Kurz Aug. 20, 2021, 2:08 p.m. UTC | #1 
On Mon, 9 Aug 2021 15:45:26 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> But always give the first 1GB to chip 0 as skiboot requires it.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/ppc/pnv.c | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 025f01c55744..2f5358b70c95 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -710,6 +710,23 @@ static void pnv_chip_power10_pic_print_info(PnvChip *chip, Monitor *mon)
>      pnv_psi_pic_print_info(&chip10->psi, mon);
>  }
>  
> +/* Always give the first 1GB to chip 0 else we won't boot */
> +static uint64_t pnv_chip_get_ram_size(PnvMachineState *pnv, int chip_id)
> +{
> +    MachineState *machine = MACHINE(pnv);
> +    uint64_t ram_per_chip;
> +
> +    assert(machine->ram_size >= 1 * GiB);
> +
> +    ram_per_chip = machine->ram_size / pnv->num_chips;
> +    if (ram_per_chip >= 1 * GiB) {
> +        return QEMU_ALIGN_DOWN(ram_per_chip, 1 * MiB);
> +    }
> +

So this is only reached if pnv->num_chips is >= 2, since
a single chip would have ram_per_chip == machine->ram_size
and thus take the return branch above.

Maybe worth making it clear with an assert() ?

> +    ram_per_chip = (machine->ram_size - 1 * GiB) / (pnv->num_chips - 1);

Suggesting that because I was looking for a potential divide by zero ^^

> +    return chip_id == 0 ? 1 * GiB : QEMU_ALIGN_DOWN(ram_per_chip, 1 * MiB);
> +}
> +
>  static void pnv_init(MachineState *machine)
>  {
>      const char *bios_name = machine->firmware ?: FW_FILE_NAME;
> @@ -717,6 +734,7 @@ static void pnv_init(MachineState *machine)
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
>      char *fw_filename;
>      long fw_size;
> +    uint64_t chip_ram_start = 0;
>      int i;
>      char *chip_typename;
>      DriveInfo *pnor = drive_get(IF_MTD, 0, 0);
> @@ -821,17 +839,16 @@ static void pnv_init(MachineState *machine)
>          char chip_name[32];
>          Object *chip = OBJECT(qdev_new(chip_typename));
>          int chip_id = i;
> +        uint64_t chip_ram_size =  pnv_chip_get_ram_size(pnv, chip_id);
>  
>          pnv->chips[i] = PNV_CHIP(chip);
>  
> -        /*
> -         * TODO: put all the memory in one node on chip 0 until we find a
> -         * way to specify different ranges for each chip
> -         */
> -        if (i == 0) {
> -            object_property_set_int(chip, "ram-size", machine->ram_size,
> -                                    &error_fatal);
> -        }
> +        /* Distribute RAM among the chips  */
> +        object_property_set_int(chip, "ram-start", chip_ram_start,
> +                                &error_fatal);
> +        object_property_set_int(chip, "ram-size", chip_ram_size,
> +                                &error_fatal);

Not really related but failing to set either of these looks
like it should never happen so I'd rather pass &error_abort
for debugging purpose.

Anyway,

Reviewed-by: Greg Kurz <groug@kaod.org>

> +        chip_ram_start += chip_ram_size;
>  
>          snprintf(chip_name, sizeof(chip_name), "chip[%d]", chip_id);
>          object_property_add_child(OBJECT(pnv), chip_name, chip);
Cédric Le Goater Aug. 30, 2021, 7:14 a.m. UTC | #2 
On 8/20/21 4:08 PM, Greg Kurz wrote:
> On Mon, 9 Aug 2021 15:45:26 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> But always give the first 1GB to chip 0 as skiboot requires it.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/ppc/pnv.c | 33 +++++++++++++++++++++++++--------
>>  1 file changed, 25 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 025f01c55744..2f5358b70c95 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -710,6 +710,23 @@ static void pnv_chip_power10_pic_print_info(PnvChip *chip, Monitor *mon)
>>      pnv_psi_pic_print_info(&chip10->psi, mon);
>>  }
>>  
>> +/* Always give the first 1GB to chip 0 else we won't boot */
>> +static uint64_t pnv_chip_get_ram_size(PnvMachineState *pnv, int chip_id)
>> +{
>> +    MachineState *machine = MACHINE(pnv);
>> +    uint64_t ram_per_chip;
>> +
>> +    assert(machine->ram_size >= 1 * GiB);
>> +
>> +    ram_per_chip = machine->ram_size / pnv->num_chips;
>> +    if (ram_per_chip >= 1 * GiB) {
>> +        return QEMU_ALIGN_DOWN(ram_per_chip, 1 * MiB);
>> +    }
>> +
> 
> So this is only reached if pnv->num_chips is >= 2, since
> a single chip would have ram_per_chip == machine->ram_size
> and thus take the return branch above.
> 
> Maybe worth making it clear with an assert() ?
> 
>> +    ram_per_chip = (machine->ram_size - 1 * GiB) / (pnv->num_chips - 1);
> 
> Suggesting that because I was looking for a potential divide by zero ^^

yes.

> 
>> +    return chip_id == 0 ? 1 * GiB : QEMU_ALIGN_DOWN(ram_per_chip, 1 * MiB);
>> +}
>> +
>>  static void pnv_init(MachineState *machine)
>>  {
>>      const char *bios_name = machine->firmware ?: FW_FILE_NAME;
>> @@ -717,6 +734,7 @@ static void pnv_init(MachineState *machine)
>>      MachineClass *mc = MACHINE_GET_CLASS(machine);
>>      char *fw_filename;
>>      long fw_size;
>> +    uint64_t chip_ram_start = 0;
>>      int i;
>>      char *chip_typename;
>>      DriveInfo *pnor = drive_get(IF_MTD, 0, 0);
>> @@ -821,17 +839,16 @@ static void pnv_init(MachineState *machine)
>>          char chip_name[32];
>>          Object *chip = OBJECT(qdev_new(chip_typename));
>>          int chip_id = i;
>> +        uint64_t chip_ram_size =  pnv_chip_get_ram_size(pnv, chip_id);
>>  
>>          pnv->chips[i] = PNV_CHIP(chip);
>>  
>> -        /*
>> -         * TODO: put all the memory in one node on chip 0 until we find a
>> -         * way to specify different ranges for each chip
>> -         */
>> -        if (i == 0) {
>> -            object_property_set_int(chip, "ram-size", machine->ram_size,
>> -                                    &error_fatal);
>> -        }
>> +        /* Distribute RAM among the chips  */
>> +        object_property_set_int(chip, "ram-start", chip_ram_start,
>> +                                &error_fatal);
>> +        object_property_set_int(chip, "ram-size", chip_ram_size,
>> +                                &error_fatal);
> 
> Not really related but failing to set either of these looks
> like it should never happen so I'd rather pass &error_abort
> for debugging purpose.
All the other object_property_set* calls are using &error_fatal in this
routine (not the link ones). I rather be consistent and use the same error. 
But we can change all of it one day it necessary.

Thanks,


C.

> 
> Anyway,
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
>> +        chip_ram_start += chip_ram_size;
>>  
>>          snprintf(chip_name, sizeof(chip_name), "chip[%d]", chip_id);
>>          object_property_add_child(OBJECT(pnv), chip_name, chip);
>
diff mbox series

Patch

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 025f01c55744..2f5358b70c95 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -710,6 +710,23 @@  static void pnv_chip_power10_pic_print_info(PnvChip *chip, Monitor *mon)
     pnv_psi_pic_print_info(&chip10->psi, mon);
 }
 
+/* Always give the first 1GB to chip 0 else we won't boot */
+static uint64_t pnv_chip_get_ram_size(PnvMachineState *pnv, int chip_id)
+{
+    MachineState *machine = MACHINE(pnv);
+    uint64_t ram_per_chip;
+
+    assert(machine->ram_size >= 1 * GiB);
+
+    ram_per_chip = machine->ram_size / pnv->num_chips;
+    if (ram_per_chip >= 1 * GiB) {
+        return QEMU_ALIGN_DOWN(ram_per_chip, 1 * MiB);
+    }
+
+    ram_per_chip = (machine->ram_size - 1 * GiB) / (pnv->num_chips - 1);
+    return chip_id == 0 ? 1 * GiB : QEMU_ALIGN_DOWN(ram_per_chip, 1 * MiB);
+}
+
 static void pnv_init(MachineState *machine)
 {
     const char *bios_name = machine->firmware ?: FW_FILE_NAME;
@@ -717,6 +734,7 @@  static void pnv_init(MachineState *machine)
     MachineClass *mc = MACHINE_GET_CLASS(machine);
     char *fw_filename;
     long fw_size;
+    uint64_t chip_ram_start = 0;
     int i;
     char *chip_typename;
     DriveInfo *pnor = drive_get(IF_MTD, 0, 0);
@@ -821,17 +839,16 @@  static void pnv_init(MachineState *machine)
         char chip_name[32];
         Object *chip = OBJECT(qdev_new(chip_typename));
         int chip_id = i;
+        uint64_t chip_ram_size =  pnv_chip_get_ram_size(pnv, chip_id);
 
         pnv->chips[i] = PNV_CHIP(chip);
 
-        /*
-         * TODO: put all the memory in one node on chip 0 until we find a
-         * way to specify different ranges for each chip
-         */
-        if (i == 0) {
-            object_property_set_int(chip, "ram-size", machine->ram_size,
-                                    &error_fatal);
-        }
+        /* Distribute RAM among the chips  */
+        object_property_set_int(chip, "ram-start", chip_ram_start,
+                                &error_fatal);
+        object_property_set_int(chip, "ram-size", chip_ram_size,
+                                &error_fatal);
+        chip_ram_start += chip_ram_size;
 
         snprintf(chip_name, sizeof(chip_name), "chip[%d]", chip_id);
         object_property_add_child(OBJECT(pnv), chip_name, chip);