qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] target/ppc: Restore [H]DEXCR to 64-bits
@ 2024-03-20  1:50 Benjamin Gray
  2024-03-20  1:50 ` [PATCH 2/2] target/ppc: Fix GDB register indexing on secondary CPUs Benjamin Gray
  2024-03-20  4:31 ` [PATCH 1/2] target/ppc: Restore [H]DEXCR to 64-bits Nicholas Piggin
  0 siblings, 2 replies; 5+ messages in thread
From: Benjamin Gray @ 2024-03-20  1:50 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc; +Cc: npiggin, Benjamin Gray

The DEXCR emulation was recently changed to a 32-bit register, possibly
because it does have a 32-bit read-only view. It is a full 64-bit
SPR though, so use the corresponding 64-bit write functions.

Fixes: c9de140c2171 ("target/ppc: Fix width of some 32-bit SPRs")
Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
 target/ppc/cpu_init.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 7e65f08147..22fdea093b 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -5820,7 +5820,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
 {
     spr_register(env, SPR_DEXCR, "DEXCR",
             SPR_NOACCESS, SPR_NOACCESS,
-            &spr_read_generic, &spr_write_generic32,
+            &spr_read_generic, &spr_write_generic,
             0);
 
     spr_register(env, SPR_UDEXCR, "UDEXCR",
@@ -5831,7 +5831,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
     spr_register_hv(env, SPR_HDEXCR, "HDEXCR",
             SPR_NOACCESS, SPR_NOACCESS,
             SPR_NOACCESS, SPR_NOACCESS,
-            &spr_read_generic, &spr_write_generic32,
+            &spr_read_generic, &spr_write_generic,
             0);
 
     spr_register(env, SPR_UHDEXCR, "UHDEXCR",
-- 
2.44.0



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 2/2] target/ppc: Fix GDB register indexing on secondary CPUs
  2024-03-20  1:50 [PATCH 1/2] target/ppc: Restore [H]DEXCR to 64-bits Benjamin Gray
@ 2024-03-20  1:50 ` Benjamin Gray
  2024-03-20  4:32   ` Nicholas Piggin
  2024-03-20  4:31 ` [PATCH 1/2] target/ppc: Restore [H]DEXCR to 64-bits Nicholas Piggin
  1 sibling, 1 reply; 5+ messages in thread
From: Benjamin Gray @ 2024-03-20  1:50 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc; +Cc: npiggin, Benjamin Gray

The GDB server protocol assigns an arbitrary numbering of the SPRs.
We track this correspondence on each SPR with gdb_id, using it to
resolve any SPR requests GDB makes.

Early on we generate an XML representation of the SPRs to give GDB,
including this numbering. However the XML is cached globally, and we
skip setting the SPR gdb_id values on subsequent threads if we detect
it is cached. This causes QEMU to fail to resolve SPR requests against
secondary CPUs because it cannot find the matching gdb_id value on that
thread's SPRs.

This is a minimal fix to first assign the gdb_id values, then return
early if the XML is cached. Otherwise we generate the XML using the
now already initialised gdb_id values.

Fixes: 1b53948ff8f7 ("target/ppc: Use GDBFeature for dynamic XML")
Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
---
 target/ppc/gdbstub.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index 3f1e61bdb7..3b28d4e21c 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -305,14 +305,6 @@ static void gdb_gen_spr_feature(CPUState *cs)
     unsigned int num_regs = 0;
     int i;
 
-    if (pcc->gdb_spr.xml) {
-        return;
-    }
-
-    gdb_feature_builder_init(&builder, &pcc->gdb_spr,
-                             "org.qemu.power.spr", "power-spr.xml",
-                             cs->gdb_num_regs);
-
     for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
         ppc_spr_t *spr = &env->spr_cb[i];
 
@@ -320,9 +312,6 @@ static void gdb_gen_spr_feature(CPUState *cs)
             continue;
         }
 
-        gdb_feature_builder_append_reg(&builder, g_ascii_strdown(spr->name, -1),
-                                       TARGET_LONG_BITS, num_regs,
-                                       "int", "spr");
         /*
          * GDB identifies registers based on the order they are
          * presented in the XML. These ids will not match QEMU's
@@ -335,6 +324,26 @@ static void gdb_gen_spr_feature(CPUState *cs)
         num_regs++;
     }
 
+    if (pcc->gdb_spr.xml) {
+        return;
+    }
+
+    gdb_feature_builder_init(&builder, &pcc->gdb_spr,
+                             "org.qemu.power.spr", "power-spr.xml",
+                             cs->gdb_num_regs);
+
+    for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
+        ppc_spr_t *spr = &env->spr_cb[i];
+
+        if (!spr->name) {
+            continue;
+        }
+
+        gdb_feature_builder_append_reg(&builder, g_ascii_strdown(spr->name, -1),
+                                       TARGET_LONG_BITS, spr->gdb_id,
+                                       "int", "spr");
+    }
+
     gdb_feature_builder_end(&builder);
 }
 #endif
-- 
2.44.0



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] target/ppc: Restore [H]DEXCR to 64-bits
  2024-03-20  1:50 [PATCH 1/2] target/ppc: Restore [H]DEXCR to 64-bits Benjamin Gray
  2024-03-20  1:50 ` [PATCH 2/2] target/ppc: Fix GDB register indexing on secondary CPUs Benjamin Gray
@ 2024-03-20  4:31 ` Nicholas Piggin
  2024-03-20  4:33   ` Benjamin Gray
  1 sibling, 1 reply; 5+ messages in thread
From: Nicholas Piggin @ 2024-03-20  4:31 UTC (permalink / raw)
  To: Benjamin Gray, qemu-devel, qemu-ppc

On Wed Mar 20, 2024 at 11:50 AM AEST, Benjamin Gray wrote:
> The DEXCR emulation was recently changed to a 32-bit register, possibly
> because it does have a 32-bit read-only view. It is a full 64-bit
> SPR though, so use the corresponding 64-bit write functions.
>

Thanks, paper bag for me.

> Fixes: c9de140c2171 ("target/ppc: Fix width of some 32-bit SPRs")

Should that hash be fbda88f7abdee?

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> ---
>  target/ppc/cpu_init.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 7e65f08147..22fdea093b 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -5820,7 +5820,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
>  {
>      spr_register(env, SPR_DEXCR, "DEXCR",
>              SPR_NOACCESS, SPR_NOACCESS,
> -            &spr_read_generic, &spr_write_generic32,
> +            &spr_read_generic, &spr_write_generic,
>              0);
>  
>      spr_register(env, SPR_UDEXCR, "UDEXCR",
> @@ -5831,7 +5831,7 @@ static void register_power10_dexcr_sprs(CPUPPCState *env)
>      spr_register_hv(env, SPR_HDEXCR, "HDEXCR",
>              SPR_NOACCESS, SPR_NOACCESS,
>              SPR_NOACCESS, SPR_NOACCESS,
> -            &spr_read_generic, &spr_write_generic32,
> +            &spr_read_generic, &spr_write_generic,
>              0);
>  
>      spr_register(env, SPR_UHDEXCR, "UHDEXCR",



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] target/ppc: Fix GDB register indexing on secondary CPUs
  2024-03-20  1:50 ` [PATCH 2/2] target/ppc: Fix GDB register indexing on secondary CPUs Benjamin Gray
@ 2024-03-20  4:32   ` Nicholas Piggin
  0 siblings, 0 replies; 5+ messages in thread
From: Nicholas Piggin @ 2024-03-20  4:32 UTC (permalink / raw)
  To: Benjamin Gray, qemu-devel, qemu-ppc

On Wed Mar 20, 2024 at 11:50 AM AEST, Benjamin Gray wrote:
> The GDB server protocol assigns an arbitrary numbering of the SPRs.
> We track this correspondence on each SPR with gdb_id, using it to
> resolve any SPR requests GDB makes.
>
> Early on we generate an XML representation of the SPRs to give GDB,
> including this numbering. However the XML is cached globally, and we
> skip setting the SPR gdb_id values on subsequent threads if we detect
> it is cached. This causes QEMU to fail to resolve SPR requests against
> secondary CPUs because it cannot find the matching gdb_id value on that
> thread's SPRs.
>
> This is a minimal fix to first assign the gdb_id values, then return
> early if the XML is cached. Otherwise we generate the XML using the
> now already initialised gdb_id values.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>
> Fixes: 1b53948ff8f7 ("target/ppc: Use GDBFeature for dynamic XML")
> Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> ---
>  target/ppc/gdbstub.c | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
> index 3f1e61bdb7..3b28d4e21c 100644
> --- a/target/ppc/gdbstub.c
> +++ b/target/ppc/gdbstub.c
> @@ -305,14 +305,6 @@ static void gdb_gen_spr_feature(CPUState *cs)
>      unsigned int num_regs = 0;
>      int i;
>  
> -    if (pcc->gdb_spr.xml) {
> -        return;
> -    }
> -
> -    gdb_feature_builder_init(&builder, &pcc->gdb_spr,
> -                             "org.qemu.power.spr", "power-spr.xml",
> -                             cs->gdb_num_regs);
> -
>      for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
>          ppc_spr_t *spr = &env->spr_cb[i];
>  
> @@ -320,9 +312,6 @@ static void gdb_gen_spr_feature(CPUState *cs)
>              continue;
>          }
>  
> -        gdb_feature_builder_append_reg(&builder, g_ascii_strdown(spr->name, -1),
> -                                       TARGET_LONG_BITS, num_regs,
> -                                       "int", "spr");
>          /*
>           * GDB identifies registers based on the order they are
>           * presented in the XML. These ids will not match QEMU's
> @@ -335,6 +324,26 @@ static void gdb_gen_spr_feature(CPUState *cs)
>          num_regs++;
>      }
>  
> +    if (pcc->gdb_spr.xml) {
> +        return;
> +    }
> +
> +    gdb_feature_builder_init(&builder, &pcc->gdb_spr,
> +                             "org.qemu.power.spr", "power-spr.xml",
> +                             cs->gdb_num_regs);
> +
> +    for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
> +        ppc_spr_t *spr = &env->spr_cb[i];
> +
> +        if (!spr->name) {
> +            continue;
> +        }
> +
> +        gdb_feature_builder_append_reg(&builder, g_ascii_strdown(spr->name, -1),
> +                                       TARGET_LONG_BITS, spr->gdb_id,
> +                                       "int", "spr");
> +    }
> +
>      gdb_feature_builder_end(&builder);
>  }
>  #endif



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] target/ppc: Restore [H]DEXCR to 64-bits
  2024-03-20  4:31 ` [PATCH 1/2] target/ppc: Restore [H]DEXCR to 64-bits Nicholas Piggin
@ 2024-03-20  4:33   ` Benjamin Gray
  0 siblings, 0 replies; 5+ messages in thread
From: Benjamin Gray @ 2024-03-20  4:33 UTC (permalink / raw)
  To: Nicholas Piggin, qemu-devel, qemu-ppc

On Wed, 2024-03-20 at 14:31 +1000, Nicholas Piggin wrote:
> On Wed Mar 20, 2024 at 11:50 AM AEST, Benjamin Gray wrote:
> > The DEXCR emulation was recently changed to a 32-bit register,
> > possibly
> > because it does have a 32-bit read-only view. It is a full 64-bit
> > SPR though, so use the corresponding 64-bit write functions.
> > 
> 
> Thanks, paper bag for me.
> 
> > Fixes: c9de140c2171 ("target/ppc: Fix width of some 32-bit SPRs")
> 
> Should that hash be fbda88f7abdee?
> 

Oops, yeah, somehow pasted the local commit hash for this patch itself

> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> 
> > Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
> > ---
> >  target/ppc/cpu_init.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> > index 7e65f08147..22fdea093b 100644
> > --- a/target/ppc/cpu_init.c
> > +++ b/target/ppc/cpu_init.c
> > @@ -5820,7 +5820,7 @@ static void
> > register_power10_dexcr_sprs(CPUPPCState *env)
> >  {
> >      spr_register(env, SPR_DEXCR, "DEXCR",
> >              SPR_NOACCESS, SPR_NOACCESS,
> > -            &spr_read_generic, &spr_write_generic32,
> > +            &spr_read_generic, &spr_write_generic,
> >              0);
> >  
> >      spr_register(env, SPR_UDEXCR, "UDEXCR",
> > @@ -5831,7 +5831,7 @@ static void
> > register_power10_dexcr_sprs(CPUPPCState *env)
> >      spr_register_hv(env, SPR_HDEXCR, "HDEXCR",
> >              SPR_NOACCESS, SPR_NOACCESS,
> >              SPR_NOACCESS, SPR_NOACCESS,
> > -            &spr_read_generic, &spr_write_generic32,
> > +            &spr_read_generic, &spr_write_generic,
> >              0);
> >  
> >      spr_register(env, SPR_UHDEXCR, "UHDEXCR",
> 



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-03-20  4:33 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-20  1:50 [PATCH 1/2] target/ppc: Restore [H]DEXCR to 64-bits Benjamin Gray
2024-03-20  1:50 ` [PATCH 2/2] target/ppc: Fix GDB register indexing on secondary CPUs Benjamin Gray
2024-03-20  4:32   ` Nicholas Piggin
2024-03-20  4:31 ` [PATCH 1/2] target/ppc: Restore [H]DEXCR to 64-bits Nicholas Piggin
2024-03-20  4:33   ` Benjamin Gray

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).