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 <da...@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
signature.asc
Description: PGP signature
