pseries: Implements h_read hcall

Submitted by Erlon Cruz on Feb. 7, 2013, 11:28 a.m.

Details

Message ID 1360236500-5613-1-git-send-email-erlon.cruz@fit-tecnologia.org.br
State New
Headers show

Commit Message

Erlon Cruz Feb. 7, 2013, 11:28 a.m.
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(+)

Comments

David Gibson Feb. 11, 2013, 12:10 a.m.
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.
Alexander Graf Feb. 12, 2013, 10:07 p.m.
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
David Gibson Feb. 13, 2013, 5:21 a.m.
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.
Erlon Cruz Feb. 14, 2013, 12:44 p.m.
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
David Gibson Feb. 14, 2013, 11:34 p.m.
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.
Alexander Graf Feb. 15, 2013, 12:09 a.m.
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

Patch hide | download patch | download mbox

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);