Re: [Qemu-devel] [PATCH v2 3/3] target/ppc: generalize check on radix when in HV mode

[David Gibson]

On Mon, Mar 12, 2018 at 07:36:05PM +0100, Cédric Le Goater wrote:
> On 02/19/2018 04:29 AM, David Gibson wrote:
> > On Fri, Feb 16, 2018 at 09:45:04AM +0100, Cédric Le Goater wrote:
> >> 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. Use that bit to check
> >> for radix mode on pseries and powernv QEMU machines.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> >>  Changes since v1:
> >>
> >>  - fixed commit log
> >>  - introduced ppc64_v3_get_patbe0()
> >>  - renamed ppc64_radix() in ppc64_v3_radix()
> >>  
> >>  target/ppc/mmu-book3s-v3.c  | 16 +++++++++++++++-
> >>  target/ppc/mmu-book3s-v3.h  | 11 +++--------
> >>  target/ppc/mmu_helper.c     |  4 ++--
> >>  target/ppc/translate_init.c |  2 +-
> >>  4 files changed, 21 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/target/ppc/mmu-book3s-v3.c b/target/ppc/mmu-book3s-v3.c
> >> index b60df4408f3b..9d05e07ef6bd 100644
> >> --- a/target/ppc/mmu-book3s-v3.c
> >> +++ b/target/ppc/mmu-book3s-v3.c
> >> @@ -23,10 +23,24 @@
> >>  #include "mmu-book3s-v3.h"
> >>  #include "mmu-radix64.h"
> >>  
> >> +bool ppc64_v3_radix(PowerPCCPU *cpu)
> >> +{
> >> +    CPUPPCState *env = &cpu->env;
> >> +
> >> +    if (msr_hv) {
> >> +        return ppc64_v3_get_patbe0(cpu) & PATBE0_HR;
> >> +    } else  {
> >> +        PPCVirtualHypervisorClass *vhc =
> >> +            PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
> >> +
> >> +        return !!(vhc->get_patbe(cpu->vhyp) & PATBE1_GR);
> >> +    }
> > 
> > I think this is backwards.  If cpu->vhyp is set, you should always the
> > get_patbe() hook, before you go looking at anything else.
> 
> OK. So, we should probably change the ppc64_radix_guest() name to 
> reflect its relation to spapr. How about ppc64_v3_radix_spapr() ?

Um.. why?  The existing name is accurate AFAICT.  It already says
"guest", and even in the unlikely event of a non PAPR guest, the
vhyp->get_patbe method can abstract that correctly.

> > This is also wrong if you have a powernv platform but msr_hv is not
> > set - which is what you'll have once you get to the point of trying to
> > run guests within an emulated powernv machine.
> 
> That is a good goal to reach ! I will add an error for the !msr_hv
> case.
> 
> Thanks,
> 
> C.
> 
> 
> >> +}
> >> +
> >>  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_v3_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 a7ab580c3140..a12bb1e28b45 100644
> >> --- a/target/ppc/mmu-book3s-v3.h
> >> +++ b/target/ppc/mmu-book3s-v3.h
> >> @@ -29,7 +29,8 @@
> >>  #define PTCR_PATS               0x000000000000001FULL /* Partition Table 
> >> Size */
> >>  
> >>  /* Partition Table Entry Fields */
> >> -#define PATBE1_GR 0x8000000000000000
> >> +#define PATBE0_HR               PPC_BIT(0)            /* 1:Host Radix 
> >> 0:HPT   */
> >> +#define PATBE1_GR               PPC_BIT(0)            /* 1:Guest Radix 
> >> 0:HPT  */
> >>  
> >>  /* Process Table Entry */
> >>  struct prtb_entry {
> >> @@ -43,13 +44,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_v3_radix(PowerPCCPU *cpu);
> >>  
> >>  int ppc64_v3_handle_mmu_fault(PowerPCCPU *cpu, vaddr eaddr, int rwx,
> >>                                int mmu_idx);
> >> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> >> index 82e63552f617..81a43982e421 100644
> >> --- a/target/ppc/mmu_helper.c
> >> +++ b/target/ppc/mmu_helper.c
> >> @@ -1285,7 +1285,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_v3_radix(ppc_env_get_cpu(env))) {
> >>              /* TODO - Unsupported */
> >>          } else {
> >>              dump_slb(f, cpu_fprintf, ppc_env_get_cpu(env));
> >> @@ -1431,7 +1431,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_v3_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 c998ac2ee405..21d5dcd15386 100644
> >> --- a/target/ppc/translate_init.c
> >> +++ b/target/ppc/translate_init.c
> >> @@ -8967,7 +8967,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 when radix is enabled.
> >>           */
> >> -        if (ppc64_radix_guest(cpu)) {
> >> +        if (ppc64_v3_radix(cpu)) {
> >>              lpcr->default_value |= LPCR_UPRT | LPCR_GTSE;
> >>          } else {
> >>              lpcr->default_value &= ~(LPCR_UPRT | LPCR_GTSE);
> > 
> 

-- 
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