From: Michael Ellerman <mpe@ellerman•id.au>
To: Sukadev Bhattiprolu <sukadev@linux•vnet.ibm.com>,
Paul Clarke <pc@us•ibm.com>
Cc: linuxppc-dev@lists•ozlabs.org, linux-kernel@vger•kernel.org
Subject: Re: [PATCH 2/2] powerpc/pseries: Dynamically increase RMA size
Date: Wed, 01 Feb 2017 16:37:58 +1100 [thread overview]
Message-ID: <87wpdatqrt.fsf@concordia.ellerman.id.au> (raw)
In-Reply-To: <20160809171131.GA2329@us.ibm.com>
Sukadev Bhattiprolu <sukadev@linux•vnet.ibm.com> writes:
> Paul Clarke [pc@us•ibm.com] wrote:
> ---
>
> From f9e9e8460206bc3fa7eaa741b9a2bde22870b9e0 Mon Sep 17 00:00:00 2001
I know it's been a while but I think it would still be good to get this
in a shape that we can merge it.
Comments inline ...
> From: root <sukadev@linux•vnet.ibm.com>
> Date: Thu, 4 Aug 2016 23:13:37 -0400
> Subject: [PATCH 2/2] powerpc/pseries: Dynamically grow RMA size
>
> When booting a very large system with a large initrd we run out of space
> for the flattened device tree (FDT). To fix this we must increase the
> space allocated for the RMA region.
>
> The RMA size is hard-coded in the 'ibm_architecture_vec[]' and increasing
> the size there will apply to all systems, large and small, so we want to
> increase the RMA region only when necessary.
>
> When we run out of room for the FDT, set a new OF property, 'ibm,new-rma-size'
> to the new RMA size (512MB) and issue a client-architecture-support (CAS)
> call to the firmware. This will initiate a system reboot. Upon reboot we
> notice the new property and update the RMA size accordingly.
>
> Fix suggested by Michael Ellerman.
>
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux•vnet.ibm.com>
>
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index f612a99..d1aaeda 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -87,6 +87,9 @@
> int of_workarounds;
> #endif
>
> +#define IBM_NEW_RMA_SIZE_PROP "ibm,new-rma-size"
> +#define IBM_NEW_RMA_SIZE_STR "512"
The property name should really start with "linux,", as it's a Linux
property, not used by firmware at all.
And does it need to contain a value? Just its existence is a flag that
we want to increase the RMA size.
So it could just be called "linux,increase-rma-size".
And we don't need a #define for the name, it's not going to change once
the code is in, and a #define just obscures the actual name.
> @@ -898,6 +910,42 @@ static void fixup_nr_cores(void)
> ptcores[1] = (cores >> 16) & 0xff;
> ptcores[2] = (cores >> 8) & 0xff;
> ptcores[3] = cores & 0xff;
> + fixup_nr_cores_done = true;
That code has changed upstream, so that won't apply. But that's OK, I
don't think we need to do it anyway.
> +static void __init fixup_rma_size(void)
> +{
> + int rc;
> + u64 size;
> + unsigned char *min_rmap;
> + phandle optnode;
> + char str[64];
> +
> + optnode = call_prom("finddevice", 1, 1, ADDR("/options"));
> + if (!PHANDLE_VALID(optnode))
> + prom_panic("Cannot find /options");
> +
> + /*
> + * If a prior boot specified a new RMA size, use that size in
> + * ibm_architecture_vec[]. See also increase_rma_size().
> + */
> + size = 0ULL;
> + memset(str, 0, sizeof(str));
> + rc = prom_getprop(optnode, IBM_NEW_RMA_SIZE_PROP, &str, sizeof(str));
> + if (rc <= 0)
> + return;
So this can just become something like:
rc = prom_getprop(optnode, "linux,increase-rma-size", NULL, 0)
if (rc == PROM_ERROR)
return;
val = be32_to_cpu(ibm_architecture_vec.vec2.min_rma);
ibm_architecture_vec.vec2.min_rma = cpu_to_be32(val * 2);
> @@ -946,6 +996,49 @@ static void __init prom_send_capabilities(void)
> }
> #endif /* __BIG_ENDIAN__ */
> }
> +
> +static void __init increase_rma_size(void)
> +{
> + int rc, len;
> + char str[64];
> + phandle optnode;
> +
> + optnode = call_prom("finddevice", 1, 1, ADDR("/options"));
> + if (!PHANDLE_VALID(optnode))
> + prom_panic("Cannot find /options");
> +
> + /*
> + * If we already increased the RMA size, return.
> + */
> + memset(str, 0, sizeof(str));
> + rc = prom_getprop(optnode, IBM_NEW_RMA_SIZE_PROP, &str, sizeof(str));
> +
> + if (!strcmp(str, IBM_NEW_RMA_SIZE_STR)) {
> + prom_printf("RMA size already at %.3s.\n", str);
> + return;
> + }
> + /*
> + * Otherwise, set the ibm,new-rma-size property and initiate a CAS
> + * reboot so the RMA size can take effect. See also init_rma_size().
> + */
> + len = strlen(IBM_NEW_RMA_SIZE_STR) + 1;
> + memcpy(str, IBM_NEW_RMA_SIZE_STR, len);
> +
> + prom_printf("Setting %s property to %s\n", IBM_NEW_RMA_SIZE_PROP, str);
> + rc = prom_setprop(optnode, "/options", IBM_NEW_RMA_SIZE_PROP, str, len);
We should check rc there shouldn't we?
Again that code can be simpler if the property is just a flag.
> + /* Force a reboot. Will work only if ibm,fw-override-cas==false */
> + prom_send_capabilities();
> +
> + prom_printf("No CAS initiated reboot? Try setting ibm,fw-override-cas to 'false' in Open Firmware\n");
I'm not sure if we want to be referring to ibm,fw-override-case. I don't
thing it's a documented property (not in PAPR anyway), and it's
certainly IBM PFW specific even if it is.
I know for a fact that on KVM you won't get rebooted here, so I think if
the CAS returns we should just reboot directly.
cheers
next prev parent reply other threads:[~2017-02-01 5:38 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-05 6:10 [PATCH 1/2] powerpc/pseries: Use a helper to fixup nr_cores Sukadev Bhattiprolu
2016-08-05 6:14 ` [PATCH 2/2] powerpc/pseries: Dynamically increase RMA size Sukadev Bhattiprolu
2016-08-05 13:28 ` kbuild test robot
2016-08-05 18:30 ` Sukadev Bhattiprolu
2016-08-05 19:04 ` Paul Clarke
2016-08-09 17:11 ` Sukadev Bhattiprolu
2017-02-01 5:37 ` Michael Ellerman [this message]
2017-02-01 17:49 ` Thiago Jung Bauermann
2017-02-01 18:11 ` Sukadev Bhattiprolu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87wpdatqrt.fsf@concordia.ellerman.id.au \
--to=mpe@ellerman$(echo .)id.au \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=pc@us$(echo .)ibm.com \
--cc=sukadev@linux$(echo .)vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox