Re: [Qemu-devel] [PATCH 3/3] target/ppc: generalize check on radix when in HV mode
On 02/02/2018 03:43 AM, Suraj Jitindar Singh wrote: > On Wed, 2018-01-31 at 09:27 +0100, Cédric Le Goater wrote: >> On a POWER9 processor, the first doubleword of the PTCR indicates >> whether the partition uses HPT or Radix Trees translation. Use that >> bit to check for radix mode on powernv QEMU machines. > > The above isn't quite right. > > On a POWER9 processor, the first doubleword of the partition table > entry (as pointed to by the PTCR) indicates whether the host uses HPT > or Radix Tree translation for that partition. yes. This is better. >> >> Signed-off-by: Cédric Le Goater >> --- >> target/ppc/mmu-book3s-v3.c | 17 - >> target/ppc/mmu-book3s-v3.h | 8 +--- >> target/ppc/mmu-hash64.h | 1 + >> target/ppc/mmu_helper.c | 4 ++-- >> target/ppc/translate_init.c | 2 +- >> 5 files changed, 21 insertions(+), 11 deletions(-) >> >> diff --git a/target/ppc/mmu-book3s-v3.c b/target/ppc/mmu-book3s-v3.c >> index e7798b3582b0..50b60fca3445 100644 >> --- a/target/ppc/mmu-book3s-v3.c >> +++ b/target/ppc/mmu-book3s-v3.c >> @@ -24,10 +24,25 @@ >> #include "mmu-book3s-v3.h" >> #include "mmu-radix64.h" >> >> +bool ppc64_radix(PowerPCCPU *cpu) >> +{ >> +CPUPPCState *env = &cpu->env; >> + >> +if (msr_hv) { > > I would prefer something like: > > uint64_t prtbe0 = ldq_phys(...); > return prtbe0 & HR; I will add a helper to retrieve the first partition table entry, as we need it in other places in patch 2. >> +return ldq_phys(CPU(cpu)->as, cpu->env.spr[SPR_PTCR] & >> +PTCR_PTAB) & PTCR_PTAB_HR; >> +} else { >> +PPCVirtualHypervisorClass *vhc = >> +PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp); >> + >> +return !!(vhc->get_patbe(cpu->vhyp) & PATBE1_GR); >> +} >> +} >> + >> int ppc64_v3_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr, int rwx, >>int mmu_idx) >> { >> -if (ppc64_radix_guest(cpu)) { /* Guest uses radix */ >> +if (ppc64_radix(cpu)) { /* radix mode */ >> return ppc_radix64_handle_mmu_fault(cpu, eaddr, rwx, >> mmu_idx); >> } else { /* Guest uses hash */ >> return ppc_hash64_handle_mmu_fault(cpu, eaddr, rwx, >> mmu_idx); >> diff --git a/target/ppc/mmu-book3s-v3.h b/target/ppc/mmu-book3s-v3.h >> index 56095dab522c..3876cb51b35c 100644 >> --- a/target/ppc/mmu-book3s-v3.h >> +++ b/target/ppc/mmu-book3s-v3.h >> @@ -37,13 +37,7 @@ static inline bool ppc64_use_proc_tbl(PowerPCCPU >> *cpu) >> return !!(cpu->env.spr[SPR_LPCR] & LPCR_UPRT); >> } >> >> -static inline bool ppc64_radix_guest(PowerPCCPU *cpu) >> -{ >> -PPCVirtualHypervisorClass *vhc = >> -PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp); >> - >> -return !!(vhc->get_patbe(cpu->vhyp) & PATBE1_GR); >> -} >> +bool ppc64_radix(PowerPCCPU *cpu); >> >> int ppc64_v3_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr, int rwx, >>int mmu_idx); >> diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h >> index 4dc6b3968ec0..7e2ac64b6eeb 100644 >> --- a/target/ppc/mmu-hash64.h >> +++ b/target/ppc/mmu-hash64.h >> @@ -106,6 +106,7 @@ void ppc_hash64_update_rmls(CPUPPCState *env); >> /* >> * Partition table definitions >> */ >> +#define PTCR_PTAB_HRPPC_BIT(0)/* 1:Host > > This isn't a bit in the partition table register, it is a bit in the > partition table entry. It should be defined in target/ppc/mmu-book3s- > v3.h as part of "/* Partition Table Entry Fields */" > > Also to follow the naming, please call it: > #define PATBE0_HR PPC_BIT(0) > > :) yeah sure. Thanks, C. >> Radix 0:HPT */ >> #define PTCR_PTAB 0x0000ULL /* Partition >> Table Base */ >> #define PTCR_PTAS 0x001FULL /* Partition >> Table Size */ >> >> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c >> index b1e660a4d16a..059863b99b2e 100644 >> --- a/target/ppc/mmu_helper.c >> +++ b/target/ppc/mmu_helper.c >> @@ -1286,7 +1286,7 @@ void dump_mmu(FILE *f, fprintf_function >> cpu_fprintf, CPUPPCState *env) >> dump_slb(f, cpu_fprintf, ppc_env_get_cpu(env)); >> break; >> case POWERPC_MMU_VER_3_00: >> -if (ppc64_radix_guest(ppc_env_get_cpu(env))) { >> +if (ppc64_radix(ppc_env_get_cpu(env))) { >> /* TODO - Unsupported */ >> } else { >> dump_slb(f, cpu_fprintf, ppc_env_get_cpu(env)); >> @@ -1432,7 +1432,7 @@ hwaddr ppc_cpu_get_phys_page_debug(CPUState >> *cs, vaddr addr) >> case POWERPC_MMU_VER_2_07: >> return ppc_hash64_get_phys_page_debug(cpu, addr); >> case POWERPC_MMU_VER_3_00: >> -if (ppc64_radix_guest(ppc_env_get_cpu(env))) { >> +if (ppc64_radix(ppc_env_get_cpu(env))) { >> return ppc_radix64_get_phys_page_debug(cpu, addr); >> } else { >> return ppc_hash64_get_phys_page_debug(cpu, addr); >> diff --git a/target/ppc/transla
Re: [Qemu-devel] [PATCH 3/3] target/ppc: generalize check on radix when in HV mode
On Wed, 2018-01-31 at 09:27 +0100, Cédric Le Goater wrote: > On a POWER9 processor, the first doubleword of the PTCR indicates > whether the partition uses HPT or Radix Trees translation. Use that > bit to check for radix mode on powernv QEMU machines. The above isn't quite right. On a POWER9 processor, the first doubleword of the partition table entry (as pointed to by the PTCR) indicates whether the host uses HPT or Radix Tree translation for that partition. > > Signed-off-by: Cédric Le Goater > --- > target/ppc/mmu-book3s-v3.c | 17 - > target/ppc/mmu-book3s-v3.h | 8 +--- > target/ppc/mmu-hash64.h | 1 + > target/ppc/mmu_helper.c | 4 ++-- > target/ppc/translate_init.c | 2 +- > 5 files changed, 21 insertions(+), 11 deletions(-) > > diff --git a/target/ppc/mmu-book3s-v3.c b/target/ppc/mmu-book3s-v3.c > index e7798b3582b0..50b60fca3445 100644 > --- a/target/ppc/mmu-book3s-v3.c > +++ b/target/ppc/mmu-book3s-v3.c > @@ -24,10 +24,25 @@ > #include "mmu-book3s-v3.h" > #include "mmu-radix64.h" > > +bool ppc64_radix(PowerPCCPU *cpu) > +{ > +CPUPPCState *env = &cpu->env; > + > +if (msr_hv) { I would prefer something like: uint64_t prtbe0 = ldq_phys(...); return prtbe0 & HR; > +return ldq_phys(CPU(cpu)->as, cpu->env.spr[SPR_PTCR] & > +PTCR_PTAB) & PTCR_PTAB_HR; > +} else { > +PPCVirtualHypervisorClass *vhc = > +PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp); > + > +return !!(vhc->get_patbe(cpu->vhyp) & PATBE1_GR); > +} > +} > + > int ppc64_v3_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr, int rwx, >int mmu_idx) > { > -if (ppc64_radix_guest(cpu)) { /* Guest uses radix */ > +if (ppc64_radix(cpu)) { /* radix mode */ > return ppc_radix64_handle_mmu_fault(cpu, eaddr, rwx, > mmu_idx); > } else { /* Guest uses hash */ > return ppc_hash64_handle_mmu_fault(cpu, eaddr, rwx, > mmu_idx); > diff --git a/target/ppc/mmu-book3s-v3.h b/target/ppc/mmu-book3s-v3.h > index 56095dab522c..3876cb51b35c 100644 > --- a/target/ppc/mmu-book3s-v3.h > +++ b/target/ppc/mmu-book3s-v3.h > @@ -37,13 +37,7 @@ static inline bool ppc64_use_proc_tbl(PowerPCCPU > *cpu) > return !!(cpu->env.spr[SPR_LPCR] & LPCR_UPRT); > } > > -static inline bool ppc64_radix_guest(PowerPCCPU *cpu) > -{ > -PPCVirtualHypervisorClass *vhc = > -PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp); > - > -return !!(vhc->get_patbe(cpu->vhyp) & PATBE1_GR); > -} > +bool ppc64_radix(PowerPCCPU *cpu); > > int ppc64_v3_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr, int rwx, >int mmu_idx); > diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h > index 4dc6b3968ec0..7e2ac64b6eeb 100644 > --- a/target/ppc/mmu-hash64.h > +++ b/target/ppc/mmu-hash64.h > @@ -106,6 +106,7 @@ void ppc_hash64_update_rmls(CPUPPCState *env); > /* > * Partition table definitions > */ > +#define PTCR_PTAB_HRPPC_BIT(0)/* 1:Host This isn't a bit in the partition table register, it is a bit in the partition table entry. It should be defined in target/ppc/mmu-book3s- v3.h as part of "/* Partition Table Entry Fields */" Also to follow the naming, please call it: #define PATBE0_HR PPC_BIT(0) :) > Radix 0:HPT */ > #define PTCR_PTAB 0x0000ULL /* Partition > Table Base */ > #define PTCR_PTAS 0x001FULL /* Partition > Table Size */ > > diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c > index b1e660a4d16a..059863b99b2e 100644 > --- a/target/ppc/mmu_helper.c > +++ b/target/ppc/mmu_helper.c > @@ -1286,7 +1286,7 @@ void dump_mmu(FILE *f, fprintf_function > cpu_fprintf, CPUPPCState *env) > dump_slb(f, cpu_fprintf, ppc_env_get_cpu(env)); > break; > case POWERPC_MMU_VER_3_00: > -if (ppc64_radix_guest(ppc_env_get_cpu(env))) { > +if (ppc64_radix(ppc_env_get_cpu(env))) { > /* TODO - Unsupported */ > } else { > dump_slb(f, cpu_fprintf, ppc_env_get_cpu(env)); > @@ -1432,7 +1432,7 @@ hwaddr ppc_cpu_get_phys_page_debug(CPUState > *cs, vaddr addr) > case POWERPC_MMU_VER_2_07: > return ppc_hash64_get_phys_page_debug(cpu, addr); > case POWERPC_MMU_VER_3_00: > -if (ppc64_radix_guest(ppc_env_get_cpu(env))) { > +if (ppc64_radix(ppc_env_get_cpu(env))) { > return ppc_radix64_get_phys_page_debug(cpu, addr); > } else { > return ppc_hash64_get_phys_page_debug(cpu, addr); > diff --git a/target/ppc/translate_init.c > b/target/ppc/translate_init.c > index a6eaa74244ca..07012ee75e81 100644 > --- a/target/ppc/translate_init.c > +++ b/target/ppc/translate_init.c > @@ -8965,7 +8965,7 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu, > PPCVirtualHypervisor *vhyp) > * KVM but not under TCG. Update the default LPCR to keep > new > * CPUs in sync wh