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) {
+ 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;
--
2.24.1
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.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
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;
> >
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
© 2016 - 2025 Red Hat, Inc.