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>
---
hw/ppc/spapr.c | 8 ++++++++
target/ppc/cpu.h | 1 +
target/ppc/mmu-hash64.h | 5 +++++
3 files changed, 14 insertions(+)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f1798457bc4d..2329664e0c2c 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1327,6 +1327,13 @@ static hwaddr spapr_hpt_mask(PPCVirtualHypervisor *vhyp)
return HTAB_SIZE(spapr) / HASH_PTEG_SIZE_64 - 1;
}
+static hwaddr spapr_hpt_base(PPCVirtualHypervisor *vhyp)
+{
+ sPAPRMachineState *spapr = SPAPR_MACHINE(vhyp);
+
+ return (hwaddr) spapr->htab;
+}
+
static target_ulong spapr_encode_hpt_for_kvm_pr(PPCVirtualHypervisor *vhyp)
{
sPAPRMachineState *spapr = SPAPR_MACHINE(vhyp);
@@ -4073,6 +4080,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
smc->phb_placement = spapr_phb_placement;
vhc->hypercall = emulate_spapr_hypercall;
vhc->hpt_mask = spapr_hpt_mask;
+ vhc->hpt_base = spapr_hpt_base;
vhc->map_hptes = spapr_map_hptes;
vhc->unmap_hptes = spapr_unmap_hptes;
vhc->store_hpte = spapr_store_hpte;
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 7bde1884a142..4de0653a3984 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1258,6 +1258,7 @@ struct PPCVirtualHypervisorClass {
InterfaceClass parent;
void (*hypercall)(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu);
hwaddr (*hpt_mask)(PPCVirtualHypervisor *vhyp);
+ hwaddr (*hpt_base)(PPCVirtualHypervisor *vhyp);
const ppc_hash_pte64_t *(*map_hptes)(PPCVirtualHypervisor *vhyp,
hwaddr ptex, int n);
void (*unmap_hptes)(PPCVirtualHypervisor *vhyp,
diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
index d297b97d3773..0ade8d15d9e4 100644
--- a/target/ppc/mmu-hash64.h
+++ b/target/ppc/mmu-hash64.h
@@ -100,6 +100,11 @@ void ppc_hash64_update_rmls(CPUPPCState *env);
static inline hwaddr ppc_hash64_hpt_base(PowerPCCPU *cpu)
{
+ if (cpu->vhyp) {
+ PPCVirtualHypervisorClass *vhc =
+ PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
+ return vhc->hpt_base(cpu->vhyp);
+ }
return cpu->env.spr[SPR_SDR1] & SDR_64_HTABORG;
}
--
2.13.6
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.
> ---
> hw/ppc/spapr.c | 8 ++++++++
> target/ppc/cpu.h | 1 +
> target/ppc/mmu-hash64.h | 5 +++++
> 3 files changed, 14 insertions(+)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f1798457bc4d..2329664e0c2c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1327,6 +1327,13 @@ static hwaddr spapr_hpt_mask(PPCVirtualHypervisor *vhyp)
> return HTAB_SIZE(spapr) / HASH_PTEG_SIZE_64 - 1;
> }
>
> +static hwaddr spapr_hpt_base(PPCVirtualHypervisor *vhyp)
> +{
> + sPAPRMachineState *spapr = SPAPR_MACHINE(vhyp);
> +
> + return (hwaddr) spapr->htab;
> +}
> +
> static target_ulong spapr_encode_hpt_for_kvm_pr(PPCVirtualHypervisor *vhyp)
> {
> sPAPRMachineState *spapr = SPAPR_MACHINE(vhyp);
> @@ -4073,6 +4080,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
> smc->phb_placement = spapr_phb_placement;
> vhc->hypercall = emulate_spapr_hypercall;
> vhc->hpt_mask = spapr_hpt_mask;
> + vhc->hpt_base = spapr_hpt_base;
> vhc->map_hptes = spapr_map_hptes;
> vhc->unmap_hptes = spapr_unmap_hptes;
> vhc->store_hpte = spapr_store_hpte;
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 7bde1884a142..4de0653a3984 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1258,6 +1258,7 @@ struct PPCVirtualHypervisorClass {
> InterfaceClass parent;
> void (*hypercall)(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu);
> hwaddr (*hpt_mask)(PPCVirtualHypervisor *vhyp);
> + hwaddr (*hpt_base)(PPCVirtualHypervisor *vhyp);
> const ppc_hash_pte64_t *(*map_hptes)(PPCVirtualHypervisor *vhyp,
> hwaddr ptex, int n);
> void (*unmap_hptes)(PPCVirtualHypervisor *vhyp,
> diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
> index d297b97d3773..0ade8d15d9e4 100644
> --- a/target/ppc/mmu-hash64.h
> +++ b/target/ppc/mmu-hash64.h
> @@ -100,6 +100,11 @@ void ppc_hash64_update_rmls(CPUPPCState *env);
>
> static inline hwaddr ppc_hash64_hpt_base(PowerPCCPU *cpu)
> {
> + if (cpu->vhyp) {
> + PPCVirtualHypervisorClass *vhc =
> + PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
> + return vhc->hpt_base(cpu->vhyp);
> + }
> return cpu->env.spr[SPR_SDR1] & SDR_64_HTABORG;
> }
>
--
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
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;
}
....
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.
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
© 2016 - 2025 Red Hat, Inc.