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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4029 bytes --]

On Sat, Mar 17, 2018 at 09:55:28AM +0100, Cédric Le Goater wrote:
> 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 <clg@kaod.org>
> > 
> > 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;
>       }
>       ....

Right.. so called, but not really used.  A little ugly, but we get
away with it for now.

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

I certainly don't agree with the definition proposed - that can return
either a guest address or a host userspace address depending on
various factors.  That's definitely not a sensible interface.

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

We could do that, since this routine is basically only used for
logging / debugging, the 0 acts as just a placeholder.

The more strictly correct (but a bit more work) option would be to put
an assert(!cpu->vhyp) in there, and fix the code so that we
really don't call ppc_hash64_hpt_base() in the vhyp cases.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]