| Message ID | 1360236500-5613-1-git-send-email-erlon.cruz@fit-tecnologia.org.br |
|---|---|
| State | New |
| Headers | show |
On Thu, Feb 07, 2013 at 09:28:20AM -0200, Erlon Cruz wrote: > From: Erlon Cruz <erlon.cruz@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.cruz@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. > + 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.
On 07.02.2013, at 12:28, Erlon Cruz wrote: > From: Erlon Cruz <erlon.cruz@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. > > Signed-off-by: Erlon Cruz <erlon.cruz@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)) { > + target_ulong v, r; > + target_ulong *pteh = &args[0]; > + target_ulong *ptel = &args[1]; > + > + hpte = env->external_htab + (pte_index * HASH_PTE_SIZE_64); You are not guaranteed that there is an external htab. In fact, looking at the external_htab users, we should probably introduce a few helper read functions for the htab that abstract the glorious external_htab/htab_base details away from you. Alex
On Tue, Feb 12, 2013 at 11:07:10PM +0100, Alexander Graf wrote: > > On 07.02.2013, at 12:28, Erlon Cruz wrote: > > > From: Erlon Cruz <erlon.cruz@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. > > > > Signed-off-by: Erlon Cruz <erlon.cruz@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)) { > > + target_ulong v, r; > > + target_ulong *pteh = &args[0]; > > + target_ulong *ptel = &args[1]; > > + > > + hpte = env->external_htab + (pte_index * HASH_PTE_SIZE_64); > > You are not guaranteed that there is an external htab. Actually in the case of spapr, you are - the existing hash table management calls all assume the existence of an external htab. > In fact, looking at the external_htab users, we should probably > introduce a few helper read functions for the htab that abstract the > glorious external_htab/htab_base details away from you. That said, I actually wrote such helpers about 15 minutes ago as part of my MMU cleanup series.
On Sun, Feb 10, 2013 at 10:10 PM, David Gibson <david@gibson.dropbear.id.au> wrote: > > On Thu, Feb 07, 2013 at 09:28:20AM -0200, Erlon Cruz wrote: > > From: Erlon Cruz <erlon.cruz@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.cruz@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
On Thu, Feb 14, 2013 at 10:44:34AM -0200, Erlon Cruz wrote: > On Sun, Feb 10, 2013 at 10:10 PM, David Gibson > <david@gibson.dropbear.id.au> wrote: > > > > On Thu, Feb 07, 2013 at 09:28:20AM -0200, Erlon Cruz wrote: > > > From: Erlon Cruz <erlon.cruz@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.cruz@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. Yes. > > 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? Probably not, my series will be a little while because it will probably need to wait for the big cpu qomification series.
On 13.02.2013, at 06:21, David Gibson wrote: > On Tue, Feb 12, 2013 at 11:07:10PM +0100, Alexander Graf wrote: >> >> On 07.02.2013, at 12:28, Erlon Cruz wrote: >> >>> From: Erlon Cruz <erlon.cruz@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. >>> >>> Signed-off-by: Erlon Cruz <erlon.cruz@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)) { >>> + target_ulong v, r; >>> + target_ulong *pteh = &args[0]; >>> + target_ulong *ptel = &args[1]; >>> + >>> + hpte = env->external_htab + (pte_index * HASH_PTE_SIZE_64); >> >> You are not guaranteed that there is an external htab. > > Actually in the case of spapr, you are - the existing hash table > management calls all assume the existence of an external htab. Ok, just leave the code using external_htab and we'll make it use the helpers once they're there. Alex > >> In fact, looking at the external_htab users, we should probably >> introduce a few helper read functions for the htab that abstract the >> glorious external_htab/htab_base details away from you. > > That said, I actually wrote such helpers about 15 minutes ago as part > of my MMU cleanup series. > > -- > 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
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)) { + 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 */ + ; + } + + *pteh = v; + *ptel = r; + + } else { + int i, ridx = 0; + target_ulong v[4], r[4]; + + /* Clear the two low order bits */ + pte_index &= ~(3ULL); + hpte = env->external_htab + (pte_index * HASH_PTE_SIZE_64); + + for (i = 0; i < 4; i++) { + v[i] = ldq_p(hpte); + r[i] = ldq_p(hpte + (HASH_PTE_SIZE_64/2)); + + if (flags & H_R_XLATE) { + /* FIXME: include a valid logical page num in the pte */ + ; + } + + args[ridx++] = v[i]; + args[ridx++] = r[i]; + + hpte += HASH_PTE_SIZE_64; + } + } + + return H_SUCCESS; +} + static target_ulong h_set_dabr(PowerPCCPU *cpu, sPAPREnvironment *spapr, target_ulong opcode, target_ulong *args) { @@ -714,6 +771,7 @@ static void hypercall_register_types(void) spapr_register_hypercall(H_ENTER, h_enter); spapr_register_hypercall(H_REMOVE, h_remove); spapr_register_hypercall(H_PROTECT, h_protect); + spapr_register_hypercall(H_READ, h_read); /* hcall-bulk */ spapr_register_hypercall(H_BULK_REMOVE, h_bulk_remove);