diff mbox series

[v6,09/18] target/ppc: Streamline calculation of RMA limit from LPCR[RMLS]

Message ID 20200224233724.46415-10-david@gibson.dropbear.id.au (mailing list archive)
State New, archived
Headers show
Series [v6,01/18] pseries: Update SLOF firmware image | expand

Commit Message

David Gibson Feb. 24, 2020, 11:37 p.m. UTC 
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 <david@gibson.dropbear.id.au>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
---
 target/ppc/mmu-hash64.c | 71 ++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 36 deletions(-)

Comments

Greg Kurz Feb. 25, 2020, 5:05 p.m. UTC | #1 
On Tue, 25 Feb 2020 10:37:15 +1100
David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
>  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... 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
> +         */
> +        return 0;
> +    }
> +}
> +
>  int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr,
>                                  int rwx, int mmu_idx)
>  {
> @@ -1006,41 +1040,6 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu, target_ulong ptex,
>      cpu->env.tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH;
>  }
>  
> -static void ppc_hash64_update_rmls(PowerPCCPU *cpu)
> -{
> -    CPUPPCState *env = &cpu->env;
> -    uint64_t lpcr = env->spr[SPR_LPCR];
> -
> -    /*
> -     * 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
> -     */
> -    switch ((lpcr & LPCR_RMLS) >> LPCR_RMLS_SHIFT) {
> -    case 0x8: /* 32MB */
> -        env->rmls = 0x2000000ull;
> -        break;
> -    case 0x3: /* 64MB */
> -        env->rmls = 0x4000000ull;
> -        break;
> -    case 0x7: /* 128MB */
> -        env->rmls = 0x8000000ull;
> -        break;
> -    case 0x4: /* 256MB */
> -        env->rmls = 0x10000000ull;
> -        break;
> -    case 0x2: /* 1GB */
> -        env->rmls = 0x40000000ull;
> -        break;
> -    case 0x1: /* 16GB */
> -        env->rmls = 0x400000000ull;
> -        break;
> -    default:
> -        /* What to do here ??? */
> -        env->rmls = 0;
> -    }
> -}
> -
>  static void ppc_hash64_update_vrma(PowerPCCPU *cpu)
>  {
>      CPUPPCState *env = &cpu->env;
> @@ -1099,7 +1098,7 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
>      CPUPPCState *env = &cpu->env;
>  
>      env->spr[SPR_LPCR] = val & pcc->lpcr_mask;
> -    ppc_hash64_update_rmls(cpu);
> +    env->rmls = rmls_limit(cpu);
>      ppc_hash64_update_vrma(cpu);
>  }
>
Greg Kurz Feb. 25, 2020, 10:47 p.m. UTC | #2 
On Tue, 25 Feb 2020 18:05:31 +0100
Greg Kurz <groug@kaod.org> wrote:

> On Tue, 25 Feb 2020 10:37:15 +1100
> David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> > Reviewed-by: Cédric Le Goater <clg@kaod.org>
> > ---
> >  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...

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,
    };

?

> > +        return 0;
> > +    }
> > +}
> > +
> >  int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr,
> >                                  int rwx, int mmu_idx)
> >  {
> > @@ -1006,41 +1040,6 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu, target_ulong ptex,
> >      cpu->env.tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH;
> >  }
> >  
> > -static void ppc_hash64_update_rmls(PowerPCCPU *cpu)
> > -{
> > -    CPUPPCState *env = &cpu->env;
> > -    uint64_t lpcr = env->spr[SPR_LPCR];
> > -
> > -    /*
> > -     * 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
> > -     */
> > -    switch ((lpcr & LPCR_RMLS) >> LPCR_RMLS_SHIFT) {
> > -    case 0x8: /* 32MB */
> > -        env->rmls = 0x2000000ull;
> > -        break;
> > -    case 0x3: /* 64MB */
> > -        env->rmls = 0x4000000ull;
> > -        break;
> > -    case 0x7: /* 128MB */
> > -        env->rmls = 0x8000000ull;
> > -        break;
> > -    case 0x4: /* 256MB */
> > -        env->rmls = 0x10000000ull;
> > -        break;
> > -    case 0x2: /* 1GB */
> > -        env->rmls = 0x40000000ull;
> > -        break;
> > -    case 0x1: /* 16GB */
> > -        env->rmls = 0x400000000ull;
> > -        break;
> > -    default:
> > -        /* What to do here ??? */
> > -        env->rmls = 0;
> > -    }
> > -}
> > -
> >  static void ppc_hash64_update_vrma(PowerPCCPU *cpu)
> >  {
> >      CPUPPCState *env = &cpu->env;
> > @@ -1099,7 +1098,7 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
> >      CPUPPCState *env = &cpu->env;
> >  
> >      env->spr[SPR_LPCR] = val & pcc->lpcr_mask;
> > -    ppc_hash64_update_rmls(cpu);
> > +    env->rmls = rmls_limit(cpu);
> >      ppc_hash64_update_vrma(cpu);
> >  }
> >  
>
David Gibson Feb. 26, 2020, 1:04 a.m. UTC | #3 
On Tue, Feb 25, 2020 at 11:47:25PM +0100, Greg Kurz wrote:
> On Tue, 25 Feb 2020 18:05:31 +0100
> Greg Kurz <groug@kaod.org> wrote:
> 
> > On Tue, 25 Feb 2020 10:37:15 +1100
> > David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> > > Reviewed-by: Cédric Le Goater <clg@kaod.org>
> > > ---
> > >  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.
Greg Kurz Feb. 26, 2020, 7:56 a.m. UTC | #4 
On Wed, 26 Feb 2020 12:04:13 +1100
David Gibson <david@gibson.dropbear.id.au> 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 <groug@kaod.org> wrote:
> > 
> > > On Tue, 25 Feb 2020 10:37:15 +1100
> > > David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> > > > Reviewed-by: Cédric Le Goater <clg@kaod.org>
> > > > ---
> > > >  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];
}
David Gibson Feb. 27, 2020, 4:25 a.m. UTC | #5 
On Wed, Feb 26, 2020 at 08:56:40AM +0100, Greg Kurz wrote:
> On Wed, 26 Feb 2020 12:04:13 +1100
> David Gibson <david@gibson.dropbear.id.au> 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 <groug@kaod.org> wrote:
> > > 
> > > > On Tue, 25 Feb 2020 10:37:15 +1100
> > > > David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> > > > > Reviewed-by: Cédric Le Goater <clg@kaod.org>
> > > > > ---
> > > > >  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.
diff mbox series

Patch

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"
@@ -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)) {
+        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
+         */
+        return 0;
+    }
+}
+
 int ppc_hash64_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr,
                                 int rwx, int mmu_idx)
 {
@@ -1006,41 +1040,6 @@  void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu, target_ulong ptex,
     cpu->env.tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH;
 }
 
-static void ppc_hash64_update_rmls(PowerPCCPU *cpu)
-{
-    CPUPPCState *env = &cpu->env;
-    uint64_t lpcr = env->spr[SPR_LPCR];
-
-    /*
-     * 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
-     */
-    switch ((lpcr & LPCR_RMLS) >> LPCR_RMLS_SHIFT) {
-    case 0x8: /* 32MB */
-        env->rmls = 0x2000000ull;
-        break;
-    case 0x3: /* 64MB */
-        env->rmls = 0x4000000ull;
-        break;
-    case 0x7: /* 128MB */
-        env->rmls = 0x8000000ull;
-        break;
-    case 0x4: /* 256MB */
-        env->rmls = 0x10000000ull;
-        break;
-    case 0x2: /* 1GB */
-        env->rmls = 0x40000000ull;
-        break;
-    case 0x1: /* 16GB */
-        env->rmls = 0x400000000ull;
-        break;
-    default:
-        /* What to do here ??? */
-        env->rmls = 0;
-    }
-}
-
 static void ppc_hash64_update_vrma(PowerPCCPU *cpu)
 {
     CPUPPCState *env = &cpu->env;
@@ -1099,7 +1098,7 @@  void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
     CPUPPCState *env = &cpu->env;
 
     env->spr[SPR_LPCR] = val & pcc->lpcr_mask;
-    ppc_hash64_update_rmls(cpu);
+    env->rmls = rmls_limit(cpu);
     ppc_hash64_update_vrma(cpu);
 }