Re: [Qemu-ppc] [PATCH v3 1/4] target/ppc: export external HPT via virtual hypervisor

[Cédric Le Goater]

On 03/17/2018 05:15 AM, David Gibson wrote:
> On Thu, Mar 15, 2018 at 01:33:59PM +0000, Cédric Le Goater wrote:
>> commit e57ca75ce3b2 ("target/ppc: Manage external HPT via virtual
>> hypervisor") exported a set of methods to manipulate the HPT from the
>> core hash MMU but the base address of the HPT was not part of them and
>> SPR_SDR1 is still used under some circumstances, which is incorrect
>> for the sPAPR machines.
>>
>> This is not a major change as only the logging should be impacted but
>> nevertheless, it will help to introduce support for the hash MMU on
>> POWER9 PowerNV machines.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
> 
> This doesn't make sense.  The whole point of the "virtual hypervisor"
> is that the hash table doesn't live within the guest address space,
> and therefore it *has* no meaningful base address.  Basically
> ppc_hash64_hpt_base() should never be called if vhyp is set.  If it
> is, that's a bug.


ppc_hash64_hpt_base() is being called in a couple of places but the
returned value is only used if the machines is not a pseries :

  static inline hwaddr ppc_hash64_hpt_mask(PowerPCCPU *cpu)
  {
      if (cpu->vhyp) {
          PPCVirtualHypervisorClass *vhc =
              PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
          return vhc->hpt_mask(cpu->vhyp);
      }
      ....

  const ppc_hash_pte64_t *ppc_hash64_map_hptes(PowerPCCPU *cpu,
                                               hwaddr ptex, int n)
  {
      hwaddr pte_offset = ptex * HASH_PTE_SIZE_64;
      hwaddr base = ppc_hash64_hpt_base(cpu);
      hwaddr plen = n * HASH_PTE_SIZE_64;
      const ppc_hash_pte64_t *hptes;
  
      if (cpu->vhyp) {
          PPCVirtualHypervisorClass *vhc =
              PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
          return vhc->map_hptes(cpu->vhyp, ptex, n);
      }  
      ....
  
and also :
  
  void ppc_hash64_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
                             uint64_t pte0, uint64_t pte1)
  {
      hwaddr base = ppc_hash64_hpt_base(cpu);
      hwaddr offset = ptex * HASH_PTE_SIZE_64;
  
      if (cpu->vhyp) {
          PPCVirtualHypervisorClass *vhc =
              PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
          vhc->store_hpte(cpu->vhyp, ptex, pte0, pte1);
          return;
      }
      ....

And, in ppc_hash64_htab_lookup(), the HPT base is logged so we need
some value returned (today, this is SPR_SDR1 which equals zero but 
that's confusing I think).


If you don't agree with the hpt_base() op, we can change it to
something like :

  static inline hwaddr ppc_hash64_hpt_base(PowerPCCPU *cpu)
  {
      if (cpu->vhyp) {
          /* Unused on sPAPR machines */
          return 0;
      }
      return ppc_hash64_hpt_reg(cpu) & SDR_64_HTABORG;
  }

to be consistent with the other routines. I would like to make sure we
don't reach ppc_hash64_hpt_reg() on pseries machines. see patch 3/4.


Thanks,

C.