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

[Cédric Le Goater]

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