| Message ID | 20200224233724.46415-18-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6,01/18] pseries: Update SLOF firmware image | expand |
On 2/25/20 12:37 AM, David Gibson wrote: > Move the calculation of the Real Mode Area (RMA) size into a helper > function. While we're there clean it up and correct it in a few ways: > * Add comments making it clearer where the various constraints come from > * Remove a pointless check that the RMA fits within Node 0 (we've just > clamped it so that it does) > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr.c | 59 ++++++++++++++++++++++++++++++-------------------- > 1 file changed, 35 insertions(+), 24 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 6e9f15f64d..f0354b699d 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2648,6 +2648,40 @@ static PCIHostState *spapr_create_default_phb(void) > return PCI_HOST_BRIDGE(dev); > } > > +static hwaddr spapr_rma_size(SpaprMachineState *spapr, Error **errp) > +{ > + MachineState *machine = MACHINE(spapr); > + hwaddr rma_size = machine->ram_size; > + hwaddr node0_size = spapr_node0_size(machine); > + > + /* RMA has to fit in the first NUMA node */ > + rma_size = MIN(rma_size, node0_size); > + > + /* > + * VRMA access is via a special 1TiB SLB mapping, so the RMA can > + * never exceed that > + */ > + rma_size = MIN(rma_size, TiB); Can you use '1 * TiB'? It makes review obvious. > + > + /* > + * Clamp the RMA size based on machine type. This is for > + * migration compatibility with older qemu versions, which limited > + * the RMA size for complicated and mostly bad reasons. > + */ > + if (smc->rma_limit) { > + spapr->rma_size = MIN(spapr->rma_size, smc->rma_limit); > + } > + > + if (rma_size < (MIN_RMA_SLOF * MiB)) { This looks old copy/paste before the change "spapr: Don't use weird units for MIN_RMA_SLOF". > + error_setg(errp, > +"pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area)", > + MIN_RMA_SLOF); Similarly, "MIN_RMA_SLOF / MiB"? > + return -1; Maybe return 0 in case this function is called with errp != &error_fatal. > + } > + > + return rma_size; > +} > + > /* pSeries LPAR / sPAPR hardware init */ > static void spapr_machine_init(MachineState *machine) > { > @@ -2660,7 +2694,6 @@ static void spapr_machine_init(MachineState *machine) > int i; > MemoryRegion *sysmem = get_system_memory(); > MemoryRegion *ram = g_new(MemoryRegion, 1); > - hwaddr node0_size = spapr_node0_size(machine); > long load_limit, fw_size; > char *filename; > Error *resize_hpt_err = NULL; > @@ -2700,22 +2733,7 @@ static void spapr_machine_init(MachineState *machine) > exit(1); > } > > - spapr->rma_size = node0_size; > - > - /* > - * Clamp the RMA size based on machine type. This is for > - * migration compatibility with older qemu versions, which limited > - * the RMA size for complicated and mostly bad reasons. > - */ > - if (smc->rma_limit) { > - spapr->rma_size = MIN(spapr->rma_size, smc->rma_limit); > - } > - > - if (spapr->rma_size > node0_size) { > - error_report("Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")", > - spapr->rma_size); > - exit(1); > - } > + spapr->rma_size = spapr_rma_size(spapr, &error_fatal); > > /* Setup a load limit for the ramdisk leaving room for SLOF and FDT */ > load_limit = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FW_OVERHEAD; > @@ -2954,13 +2972,6 @@ static void spapr_machine_init(MachineState *machine) > } > } > > - if (spapr->rma_size < MIN_RMA_SLOF) { > - error_report( > - "pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area memory)", > - MIN_RMA_SLOF / MiB); > - exit(1); > - } > - > if (kernel_filename) { > uint64_t lowaddr = 0; > >
On Tue, Feb 25, 2020 at 12:07:29PM +0100, Philippe Mathieu-Daudé wrote: > On 2/25/20 12:37 AM, David Gibson wrote: > > Move the calculation of the Real Mode Area (RMA) size into a helper > > function. While we're there clean it up and correct it in a few ways: > > * Add comments making it clearer where the various constraints come from > > * Remove a pointless check that the RMA fits within Node 0 (we've just > > clamped it so that it does) > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > hw/ppc/spapr.c | 59 ++++++++++++++++++++++++++++++-------------------- > > 1 file changed, 35 insertions(+), 24 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 6e9f15f64d..f0354b699d 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -2648,6 +2648,40 @@ static PCIHostState *spapr_create_default_phb(void) > > return PCI_HOST_BRIDGE(dev); > > } > > +static hwaddr spapr_rma_size(SpaprMachineState *spapr, Error **errp) > > +{ > > + MachineState *machine = MACHINE(spapr); > > + hwaddr rma_size = machine->ram_size; > > + hwaddr node0_size = spapr_node0_size(machine); > > + > > + /* RMA has to fit in the first NUMA node */ > > + rma_size = MIN(rma_size, node0_size); > > + > > + /* > > + * VRMA access is via a special 1TiB SLB mapping, so the RMA can > > + * never exceed that > > + */ > > + rma_size = MIN(rma_size, TiB); > > Can you use '1 * TiB'? It makes review obvious. Done. > > + > > + /* > > + * Clamp the RMA size based on machine type. This is for > > + * migration compatibility with older qemu versions, which limited > > + * the RMA size for complicated and mostly bad reasons. > > + */ > > + if (smc->rma_limit) { > > + spapr->rma_size = MIN(spapr->rma_size, smc->rma_limit); > > + } > > + > > + if (rma_size < (MIN_RMA_SLOF * MiB)) { > > This looks old copy/paste before the change "spapr: Don't use weird units > for MIN_RMA_SLOF". > > > + error_setg(errp, > > +"pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area)", > > + MIN_RMA_SLOF); > > Similarly, "MIN_RMA_SLOF / MiB"? Ah, good catch. I re-ordered the series at some point and forgot to fix this up. > > + return -1; > > Maybe return 0 in case this function is called with errp != > &error_fatal. Good idea.
On Tue, 25 Feb 2020 10:37:23 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > Move the calculation of the Real Mode Area (RMA) size into a helper > function. While we're there clean it up and correct it in a few ways: > * Add comments making it clearer where the various constraints come from > * Remove a pointless check that the RMA fits within Node 0 (we've just > clamped it so that it does) > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr.c | 59 ++++++++++++++++++++++++++++++-------------------- > 1 file changed, 35 insertions(+), 24 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 6e9f15f64d..f0354b699d 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2648,6 +2648,40 @@ static PCIHostState *spapr_create_default_phb(void) > return PCI_HOST_BRIDGE(dev); > } > > +static hwaddr spapr_rma_size(SpaprMachineState *spapr, Error **errp) > +{ > + MachineState *machine = MACHINE(spapr); > + hwaddr rma_size = machine->ram_size; > + hwaddr node0_size = spapr_node0_size(machine); > + > + /* RMA has to fit in the first NUMA node */ > + rma_size = MIN(rma_size, node0_size); > + > + /* > + * VRMA access is via a special 1TiB SLB mapping, so the RMA can > + * never exceed that > + */ > + rma_size = MIN(rma_size, TiB); > + > + /* > + * Clamp the RMA size based on machine type. This is for > + * migration compatibility with older qemu versions, which limited > + * the RMA size for complicated and mostly bad reasons. > + */ > + if (smc->rma_limit) { /home/greg/Work/qemu/qemu-ppc/hw/ppc/spapr.c: In function ‘spapr_rma_size’: /home/greg/Work/qemu/qemu-ppc/hw/ppc/spapr.c:2671:9: error: ‘smc’ undeclared (first use in this function) if (smc->rma_limit) { > + spapr->rma_size = MIN(spapr->rma_size, smc->rma_limit); > + } > + > + if (rma_size < (MIN_RMA_SLOF * MiB)) { > + error_setg(errp, > +"pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area)", > + MIN_RMA_SLOF); > + return -1; > + } > + > + return rma_size; > +} > + > /* pSeries LPAR / sPAPR hardware init */ > static void spapr_machine_init(MachineState *machine) > { > @@ -2660,7 +2694,6 @@ static void spapr_machine_init(MachineState *machine) > int i; > MemoryRegion *sysmem = get_system_memory(); > MemoryRegion *ram = g_new(MemoryRegion, 1); > - hwaddr node0_size = spapr_node0_size(machine); > long load_limit, fw_size; > char *filename; > Error *resize_hpt_err = NULL; > @@ -2700,22 +2733,7 @@ static void spapr_machine_init(MachineState *machine) > exit(1); > } > > - spapr->rma_size = node0_size; > - > - /* > - * Clamp the RMA size based on machine type. This is for > - * migration compatibility with older qemu versions, which limited > - * the RMA size for complicated and mostly bad reasons. > - */ > - if (smc->rma_limit) { > - spapr->rma_size = MIN(spapr->rma_size, smc->rma_limit); > - } > - > - if (spapr->rma_size > node0_size) { > - error_report("Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")", > - spapr->rma_size); > - exit(1); > - } > + spapr->rma_size = spapr_rma_size(spapr, &error_fatal); > > /* Setup a load limit for the ramdisk leaving room for SLOF and FDT */ > load_limit = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FW_OVERHEAD; > @@ -2954,13 +2972,6 @@ static void spapr_machine_init(MachineState *machine) > } > } > > - if (spapr->rma_size < MIN_RMA_SLOF) { > - error_report( > - "pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area memory)", > - MIN_RMA_SLOF / MiB); > - exit(1); > - } > - > if (kernel_filename) { > uint64_t lowaddr = 0; >
On Wed, Feb 26, 2020 at 02:37:51PM +0100, Greg Kurz wrote: > On Tue, 25 Feb 2020 10:37:23 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > Move the calculation of the Real Mode Area (RMA) size into a helper > > function. While we're there clean it up and correct it in a few ways: > > * Add comments making it clearer where the various constraints come from > > * Remove a pointless check that the RMA fits within Node 0 (we've just > > clamped it so that it does) > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > hw/ppc/spapr.c | 59 ++++++++++++++++++++++++++++++-------------------- > > 1 file changed, 35 insertions(+), 24 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 6e9f15f64d..f0354b699d 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -2648,6 +2648,40 @@ static PCIHostState *spapr_create_default_phb(void) > > return PCI_HOST_BRIDGE(dev); > > } > > > > +static hwaddr spapr_rma_size(SpaprMachineState *spapr, Error **errp) > > +{ > > + MachineState *machine = MACHINE(spapr); > > + hwaddr rma_size = machine->ram_size; > > + hwaddr node0_size = spapr_node0_size(machine); > > + > > + /* RMA has to fit in the first NUMA node */ > > + rma_size = MIN(rma_size, node0_size); > > + > > + /* > > + * VRMA access is via a special 1TiB SLB mapping, so the RMA can > > + * never exceed that > > + */ > > + rma_size = MIN(rma_size, TiB); > > + > > + /* > > + * Clamp the RMA size based on machine type. This is for > > + * migration compatibility with older qemu versions, which limited > > + * the RMA size for complicated and mostly bad reasons. > > + */ > > + if (smc->rma_limit) { > > /home/greg/Work/qemu/qemu-ppc/hw/ppc/spapr.c: In function ‘spapr_rma_size’: > /home/greg/Work/qemu/qemu-ppc/hw/ppc/spapr.c:2671:9: error: ‘smc’ undeclared (first use in this function) > if (smc->rma_limit) { Oops. Fixed. > > > + spapr->rma_size = MIN(spapr->rma_size, smc->rma_limit); > > + } > > + > > + if (rma_size < (MIN_RMA_SLOF * MiB)) { > > + error_setg(errp, > > +"pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area)", > > + MIN_RMA_SLOF); > > + return -1; > > + } > > + > > + return rma_size; > > +} > > + > > /* pSeries LPAR / sPAPR hardware init */ > > static void spapr_machine_init(MachineState *machine) > > { > > @@ -2660,7 +2694,6 @@ static void spapr_machine_init(MachineState *machine) > > int i; > > MemoryRegion *sysmem = get_system_memory(); > > MemoryRegion *ram = g_new(MemoryRegion, 1); > > - hwaddr node0_size = spapr_node0_size(machine); > > long load_limit, fw_size; > > char *filename; > > Error *resize_hpt_err = NULL; > > @@ -2700,22 +2733,7 @@ static void spapr_machine_init(MachineState *machine) > > exit(1); > > } > > > > - spapr->rma_size = node0_size; > > - > > - /* > > - * Clamp the RMA size based on machine type. This is for > > - * migration compatibility with older qemu versions, which limited > > - * the RMA size for complicated and mostly bad reasons. > > - */ > > - if (smc->rma_limit) { > > - spapr->rma_size = MIN(spapr->rma_size, smc->rma_limit); > > - } > > - > > - if (spapr->rma_size > node0_size) { > > - error_report("Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")", > > - spapr->rma_size); > > - exit(1); > > - } > > + spapr->rma_size = spapr_rma_size(spapr, &error_fatal); > > > > /* Setup a load limit for the ramdisk leaving room for SLOF and FDT */ > > load_limit = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FW_OVERHEAD; > > @@ -2954,13 +2972,6 @@ static void spapr_machine_init(MachineState *machine) > > } > > } > > > > - if (spapr->rma_size < MIN_RMA_SLOF) { > > - error_report( > > - "pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area memory)", > > - MIN_RMA_SLOF / MiB); > > - exit(1); > > - } > > - > > if (kernel_filename) { > > uint64_t lowaddr = 0; > > >
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 6e9f15f64d..f0354b699d 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2648,6 +2648,40 @@ static PCIHostState *spapr_create_default_phb(void) return PCI_HOST_BRIDGE(dev); } +static hwaddr spapr_rma_size(SpaprMachineState *spapr, Error **errp) +{ + MachineState *machine = MACHINE(spapr); + hwaddr rma_size = machine->ram_size; + hwaddr node0_size = spapr_node0_size(machine); + + /* RMA has to fit in the first NUMA node */ + rma_size = MIN(rma_size, node0_size); + + /* + * VRMA access is via a special 1TiB SLB mapping, so the RMA can + * never exceed that + */ + rma_size = MIN(rma_size, TiB); + + /* + * Clamp the RMA size based on machine type. This is for + * migration compatibility with older qemu versions, which limited + * the RMA size for complicated and mostly bad reasons. + */ + if (smc->rma_limit) { + spapr->rma_size = MIN(spapr->rma_size, smc->rma_limit); + } + + if (rma_size < (MIN_RMA_SLOF * MiB)) { + error_setg(errp, +"pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area)", + MIN_RMA_SLOF); + return -1; + } + + return rma_size; +} + /* pSeries LPAR / sPAPR hardware init */ static void spapr_machine_init(MachineState *machine) { @@ -2660,7 +2694,6 @@ static void spapr_machine_init(MachineState *machine) int i; MemoryRegion *sysmem = get_system_memory(); MemoryRegion *ram = g_new(MemoryRegion, 1); - hwaddr node0_size = spapr_node0_size(machine); long load_limit, fw_size; char *filename; Error *resize_hpt_err = NULL; @@ -2700,22 +2733,7 @@ static void spapr_machine_init(MachineState *machine) exit(1); } - spapr->rma_size = node0_size; - - /* - * Clamp the RMA size based on machine type. This is for - * migration compatibility with older qemu versions, which limited - * the RMA size for complicated and mostly bad reasons. - */ - if (smc->rma_limit) { - spapr->rma_size = MIN(spapr->rma_size, smc->rma_limit); - } - - if (spapr->rma_size > node0_size) { - error_report("Numa node 0 has to span the RMA (%#08"HWADDR_PRIx")", - spapr->rma_size); - exit(1); - } + spapr->rma_size = spapr_rma_size(spapr, &error_fatal); /* Setup a load limit for the ramdisk leaving room for SLOF and FDT */ load_limit = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FW_OVERHEAD; @@ -2954,13 +2972,6 @@ static void spapr_machine_init(MachineState *machine) } } - if (spapr->rma_size < MIN_RMA_SLOF) { - error_report( - "pSeries SLOF firmware requires >= %ldMiB guest RMA (Real Mode Area memory)", - MIN_RMA_SLOF / MiB); - exit(1); - } - if (kernel_filename) { uint64_t lowaddr = 0;
Move the calculation of the Real Mode Area (RMA) size into a helper function. While we're there clean it up and correct it in a few ways: * Add comments making it clearer where the various constraints come from * Remove a pointless check that the RMA fits within Node 0 (we've just clamped it so that it does) Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/spapr.c | 59 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 24 deletions(-)