[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 09/18] target/ppc: Streamline calculation of RMA limit fro
From: |
David Gibson |
Subject: |
Re: [PATCH v6 09/18] target/ppc: Streamline calculation of RMA limit from LPCR[RMLS] |
Date: |
Thu, 27 Feb 2020 15:25:16 +1100 |
On Wed, Feb 26, 2020 at 08:56:40AM +0100, Greg Kurz wrote:
> On Wed, 26 Feb 2020 12:04:13 +1100
> David Gibson <address@hidden> wrote:
>
> > On Tue, Feb 25, 2020 at 11:47:25PM +0100, Greg Kurz wrote:
> > > On Tue, 25 Feb 2020 18:05:31 +0100
> > > Greg Kurz <address@hidden> wrote:
> > >
> > > > On Tue, 25 Feb 2020 10:37:15 +1100
> > > > David Gibson <address@hidden> wrote:
> > > >
> > > > > Currently we use a big switch statement in ppc_hash64_update_rmls()
> > > > > to work
> > > > > out what the right RMA limit is based on the LPCR[RMLS] field.
> > > > > There's no
> > > > > formula for this - it's just an arbitrary mapping defined by the
> > > > > existing
> > > > > CPU implementations - but we can make it a bit more readable by using
> > > > > a
> > > > > lookup table rather than a switch. In addition we can use the MiB/GiB
> > > > > symbols to make it a bit clearer.
> > > > >
> > > > > While there we add a bit of clarity and rationale to the comment about
> > > > > what happens if the LPCR[RMLS] doesn't contain a valid value.
> > > > >
> > > > > Signed-off-by: David Gibson <address@hidden>
> > > > > Reviewed-by: Cédric Le Goater <address@hidden>
> > > > > ---
> > > > > target/ppc/mmu-hash64.c | 71
> > > > > ++++++++++++++++++++---------------------
> > > > > 1 file changed, 35 insertions(+), 36 deletions(-)
> > > > >
> > > > > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> > > > > index 0ef330a614..4f082d775d 100644
> > > > > --- a/target/ppc/mmu-hash64.c
> > > > > +++ b/target/ppc/mmu-hash64.c
> > > > > @@ -18,6 +18,7 @@
> > > > > * License along with this library; if not, see
> > > > > <http://www.gnu.org/licenses/>.
> > > > > */
> > > > > #include "qemu/osdep.h"
> > > > > +#include "qemu/units.h"
> > > > > #include "cpu.h"
> > > > > #include "exec/exec-all.h"
> > > > > #include "exec/helper-proto.h"This tool was originally developed to
> > > > > fix Linux CPU throttling issues affecting Lenovo T480 / T480s / X1C6
> > > > > as described here.
> > > > > @@ -757,6 +758,39 @@ static void ppc_hash64_set_c(PowerPCCPU *cpu,
> > > > > hwaddr ptex, uint64_t pte1)
> > > > > stb_phys(CPU(cpu)->as, base + offset, (pte1 & 0xff) | 0x80);
> > > > > }
> > > > >
> > > > > +static target_ulong rmls_limit(PowerPCCPU *cpu)
> > > > > +{
> > > > > + CPUPPCState *env = &cpu->env;
> > > > > + /*
> > > > > + * This is the full 4 bits encoding of POWER8. Previous
> > > > > + * CPUs only support a subset of these but the filtering
> > > > > + * is done when writing LPCR
> > > > > + */
> > > > > + const target_ulong rma_sizes[] = {
> > > > > + [0] = 0,
> > > > > + [1] = 16 * GiB,
> > > > > + [2] = 1 * GiB,
> > > > > + [3] = 64 * MiB,
> > > > > + [4] = 256 * MiB,
> > > > > + [5] = 0,
> > > > > + [6] = 0,
> > > > > + [7] = 128 * MiB,
> > > > > + [8] = 32 * MiB,
> > > > > + };
> > > > > + target_ulong rmls = (env->spr[SPR_LPCR] & LPCR_RMLS) >>
> > > > > LPCR_RMLS_SHIFT;
> > > > > +
> > > > > + if (rmls < ARRAY_SIZE(rma_sizes)) {
> > > >
> > > > This condition is always true since the RMLS field is 4-bit long...
> > >
> > > Oops my mistake, I was already thinking about the suggestion I have
> > > for something that was puzzling me. See below.
> > >
> > > > I guess you want to check that RMLS encodes a valid RMA size instead.
> > > >
> > > > if (rma_sizes[rmls]) {
> > > >
> > > > > + return rma_sizes[rmls];
> > > > > + } else {
> > > > > + /*
> > > > > + * Bad value, so the OS has shot itself in the foot. Return
> > > > > a
> > > > > + * 0-sized RMA which we expect to trigger an immediate DSI or
> > > > > + * ISI
> > > > > + */
> > >
> > > It seems a bit weird to differentiate the case where the value is bad
> > > because it happens to be bigger than the highest supported one, compared
> > > to values that are declared bad in rma_sizes[], like 0, 5 or 6. They're
> > > all basically the same case of values not used to encode a valid
> > > size...
> >
> > Right, but the result is the same either way - the function returns
> > 0. This is basically just a small space optimization.
> >
> > >
> > > What about :
> > >
> > > static const target_ulong rma_sizes[16] = {
> > > [1] = 16 * GiB,
> > > [2] = 1 * GiB,
> > > [3] = 64 * MiB,
> > > [4] = 256 * MiB,
> > > [7] = 128 * MiB,
> > > [8] = 32 * MiB,
> > > };
> >
> > Eh, I guess? I don't see much to pick between them.
> >
>
> This is what I had in mind actually.
>
> static target_ulong rmls_limit(PowerPCCPU *cpu)
> {
> CPUPPCState *env = &cpu->env;
> /*
> * This is the full 4 bits encoding of POWER8. Previous
> * CPUs only support a subset of these but the filtering
> * is done when writing LPCR.
> *
> * Unsupported values mean the OS has shot itself in the
> * foot. Return a 0-sized RMA in this case, which we expect
> * to trigger an immediate DSI or ISI
> */
> static const target_ulong rma_sizes[16] = {
> [1] = 16 * GiB,
> [2] = 1 * GiB,
> [3] = 64 * MiB,
> [4] = 256 * MiB,
> [7] = 128 * MiB,
> [8] = 32 * MiB,
> };
> target_ulong rmls = (env->spr[SPR_LPCR] & LPCR_RMLS) >> LPCR_RMLS_SHIFT;
>
> return rma_sizes[rmls];
> }
Yeah, I guess that is a little neater. I've made the change.
--
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
- Re: [PATCH v6 17/18] spapr: Clean up RMA size calculation, (continued)
[PATCH v6 15/18] spapr: Don't attempt to clamp RMA to VRMA constraint, David Gibson, 2020/02/24
[PATCH v6 16/18] spapr: Don't clamp RMA to 16GiB on new machine types, David Gibson, 2020/02/24
[PATCH v6 01/18] pseries: Update SLOF firmware image, David Gibson, 2020/02/24
[PATCH v6 18/18] spapr: Fold spapr_node0_size() into its only caller, David Gibson, 2020/02/24