| Message ID | 20180315133402.13470-2-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > } >
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.
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; }
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(+)