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

[Cédric Le Goater]

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() ? 
 
> 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);
>