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 <c...@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
signature.asc
Description: PGP signature
