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 |
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); > } >
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); > > } > > >
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.
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]; }
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 --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); }