On Sun, Feb 10, 2013 at 10:10 PM, David Gibson <da...@gibson.dropbear.id.au> wrote: > > On Thu, Feb 07, 2013 at 09:28:20AM -0200, Erlon Cruz wrote: > > From: Erlon Cruz <erlon.c...@br.flextronics.com> > > > > This h_call is useful for DLPAR in future amongst other things. Given an > > index > > it fetches the corresponding PTE stored in the htab. > > Nice. It would be good to add in this little bit of PAPR compliance. > > Couple of small nits in the implementation: > > > > > Signed-off-by: Erlon Cruz <erlon.c...@br.flextronics.com> > > --- > > hw/spapr_hcall.c | 58 > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 58 insertions(+) > > > > diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c > > index 2889742..5ba07e5 100644 > > --- a/hw/spapr_hcall.c > > +++ b/hw/spapr_hcall.c > > @@ -323,6 +323,63 @@ static target_ulong h_protect(PowerPCCPU *cpu, > > sPAPREnvironment *spapr, > > return H_SUCCESS; > > } > > > > +static target_ulong h_read(PowerPCCPU *cpu, sPAPREnvironment *spapr, > > + target_ulong opcode, target_ulong *args) > > +{ > > + CPUPPCState *env = &cpu->env; > > + target_ulong flags = args[0]; > > + target_ulong pte_index = args[1]; > > + uint8_t *hpte; > > + > > + if ((pte_index * HASH_PTE_SIZE_64) & ~env->htab_mask) { > > + return H_PARAMETER; > > + } > > + > > + if (!(flags & H_READ_4)) { > > It would be nice to combine the H_READ_4 and !H_READ_4 paths together, > since except for the masking and stopping sooner the !H_READ_4 path is > just like the H_READ_4 path.
Ok. > > > > + target_ulong v, r; > > + target_ulong *pteh = &args[0]; > > + target_ulong *ptel = &args[1]; > > + > > + hpte = env->external_htab + (pte_index * HASH_PTE_SIZE_64); > > + > > + v = ldq_p(hpte); > > + r = ldq_p(hpte + (HASH_PTE_SIZE_64/2)); > > + > > + if (flags & H_R_XLATE) { > > + /* FIXME: include a valid logical page num in the pte */ > > This comment is misleading. Since you do copy out both words of the > hpte, and qemu stores the external_htab in terms of guest physical (== > logical) addresses, in fact you're *always* supplying a valid logical > page num. So you've already correctly implemented the flag as a > no-op. > > I believe that flag is included for the benefit of a true hypervisor, > where the native htab would be stored as true physical addresses, and > it might be expensive for the hypervisor to recompute the logical > address. > Ok, then I think we can just skip this checking. > That said, I actually wrote such helpers about 15 minutes ago as part > of my MMU cleanup series. Should I wait for the helpers to send It again? > > -- > 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