On 8/20/21 4:08 PM, Greg Kurz wrote:
> On Mon, 9 Aug 2021 15:45:26 +0200
> Cédric Le Goater <c...@kaod.org> wrote:
>
>> But always give the first 1GB to chip 0 as skiboot requires it.
>>
>> Signed-off-by: Cédric Le Goater <c...@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 <gr...@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);
>
