[RFC PATCH] target/ppc: removed usage of ppc_store_sdr1 in kvm.c

[RFC PATCH] target/ppc: removed usage of ppc_store_sdr1 in kvm.c

[Bruno Larsen (billionai)]

The only use of this function in kvm.c is right after using the KVM
ioctl to get the registers themselves, so there is no need to do the
error checks done by ppc_store_sdr1.

The probable reason this was here before is because of the hack where
KVM PR stores the hash table size along with the SDR1 information, but
since ppc_store_sdr1 would also store that information, there should be
no need to do any extra processing here.

Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---

This change means we won't have to compile ppc_store_sdr1 when we get
disable-tcg working, but I'm not working on that code motion just yet
since Lucas is dealing with the same file.

I'm sending this as an RFC because I'm pretty sure I'm missing
something, but from what I can see, this is all we'd need

 target/ppc/kvm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 104a308abb..3f52a7189d 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -1159,7 +1159,11 @@ static int kvmppc_get_books_sregs(PowerPCCPU *cpu)
     }
 
     if (!cpu->vhyp) {
-        ppc_store_sdr1(env, sregs.u.s.sdr1);
+        /*
+         * We have just gotten the SDR1, there should be no
+         * reason to do error checking.... right?
+         */
+        env->spr[SPR_SDR1] = sregs.u.s.sdr1;
     }
 
     /* Sync SLB */
-- 
2.17.1

[RFC PATCH] target/ppc: removed usage of ppc_store_sdr1 in kvm.c

[Bruno Larsen (billionai)]

The only use of this function in kvm.c is right after using the KVM
ioctl to get the registers themselves, so there is no need to do the
error checks done by ppc_store_sdr1.

The probable reason this was here before is because of the hack where
KVM PR stores the hash table size along with the SDR1 information, but
since ppc_store_sdr1 would also store that information, there should be
no need to do any extra processing here.

Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---

This change means we won't have to compile ppc_store_sdr1 when we get
disable-tcg working, but I'm not working on that code motion just yet
since Lucas is dealing with the same file.

I'm sending this as an RFC because I'm pretty sure I'm missing
something, but from what I can see, this is all we'd need

 target/ppc/kvm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 104a308abb..3f52a7189d 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -1159,7 +1159,11 @@ static int kvmppc_get_books_sregs(PowerPCCPU *cpu)
     }
 
     if (!cpu->vhyp) {
-        ppc_store_sdr1(env, sregs.u.s.sdr1);
+        /*
+         * We have just gotten the SDR1, there should be no
+         * reason to do error checking.... right?
+         */
+        env->spr[SPR_SDR1] = sregs.u.s.sdr1;
     }
 
     /* Sync SLB */
-- 
2.17.1

Re: [RFC PATCH] target/ppc: removed usage of ppc_store_sdr1 in kvm.c

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2064 bytes --]

On Tue, Jun 01, 2021 at 03:42:42PM -0300, Bruno Larsen (billionai) wrote:
> The only use of this function in kvm.c is right after using the KVM
> ioctl to get the registers themselves, so there is no need to do the
> error checks done by ppc_store_sdr1.
> 
> The probable reason this was here before is because of the hack where
> KVM PR stores the hash table size along with the SDR1 information, but
> since ppc_store_sdr1 would also store that information, there should be
> no need to do any extra processing here.
> 
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> ---
> 
> This change means we won't have to compile ppc_store_sdr1 when we get
> disable-tcg working, but I'm not working on that code motion just yet
> since Lucas is dealing with the same file.
> 
> I'm sending this as an RFC because I'm pretty sure I'm missing
> something, but from what I can see, this is all we'd need

I don't think this is a good idea.  Even though it's not strictly
necessary for KVM, I'd prefer to have a common entry point for SDR1
updates to reduce confusion.  Plus this won't be sufficient to fix
things for !TCG builds, since we still have the common calls on the
loadvm path.

> 
>  target/ppc/kvm.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 104a308abb..3f52a7189d 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1159,7 +1159,11 @@ static int kvmppc_get_books_sregs(PowerPCCPU *cpu)
>      }
>  
>      if (!cpu->vhyp) {
> -        ppc_store_sdr1(env, sregs.u.s.sdr1);
> +        /*
> +         * We have just gotten the SDR1, there should be no
> +         * reason to do error checking.... right?
> +         */
> +        env->spr[SPR_SDR1] = sregs.u.s.sdr1;
>      }
>  
>      /* Sync SLB */

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] target/ppc: removed usage of ppc_store_sdr1 in kvm.c

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2064 bytes --]

On Tue, Jun 01, 2021 at 03:42:42PM -0300, Bruno Larsen (billionai) wrote:
> The only use of this function in kvm.c is right after using the KVM
> ioctl to get the registers themselves, so there is no need to do the
> error checks done by ppc_store_sdr1.
> 
> The probable reason this was here before is because of the hack where
> KVM PR stores the hash table size along with the SDR1 information, but
> since ppc_store_sdr1 would also store that information, there should be
> no need to do any extra processing here.
> 
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> ---
> 
> This change means we won't have to compile ppc_store_sdr1 when we get
> disable-tcg working, but I'm not working on that code motion just yet
> since Lucas is dealing with the same file.
> 
> I'm sending this as an RFC because I'm pretty sure I'm missing
> something, but from what I can see, this is all we'd need

I don't think this is a good idea.  Even though it's not strictly
necessary for KVM, I'd prefer to have a common entry point for SDR1
updates to reduce confusion.  Plus this won't be sufficient to fix
things for !TCG builds, since we still have the common calls on the
loadvm path.

> 
>  target/ppc/kvm.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 104a308abb..3f52a7189d 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1159,7 +1159,11 @@ static int kvmppc_get_books_sregs(PowerPCCPU *cpu)
>      }
>  
>      if (!cpu->vhyp) {
> -        ppc_store_sdr1(env, sregs.u.s.sdr1);
> +        /*
> +         * We have just gotten the SDR1, there should be no
> +         * reason to do error checking.... right?
> +         */
> +        env->spr[SPR_SDR1] = sregs.u.s.sdr1;
>      }
>  
>      /* Sync SLB */

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]