[PATCH] target/ppc: Fix lxv/stxv MSR facility check

qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed

* [PATCH] target/ppc: Fix lxv/stxv MSR facility check
@ 2024-02-13  8:39 Nicholas Piggin
  2024-02-13  9:18 ` Harsh Prateek Bora
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Nicholas Piggin @ 2024-02-13  8:39 UTC (permalink / raw)
  To: qemu-ppc
  Cc: Nicholas Piggin, Daniel Henrique Barboza, Cédric Le Goater,
	Lucas Mateus Castro, qemu-devel, qemu-stable, Joel Stanley,
	Harsh Prateek Bora

The move to decodetree flipped the inequality test for the VEC / VSX
MSR facility check.

This caused application crashes under Linux, where these facility
unavailable interrupts are used for lazy-switching of VEC/VSX register
sets. Getting the incorrect interrupt would result in wrong registers
being loaded, potentially overwriting live values and/or exposing
stale ones.

Cc: qemu-stable@nongnu.org
Reported-by: Joel Stanley <joel@jms.id.au>
Fixes: 70426b5bb738 ("target/ppc: moved stxvx and lxvx from legacy to decodtree")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1769
Tested-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/translate/vsx-impl.c.inc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/translate/vsx-impl.c.inc b/target/ppc/translate/vsx-impl.c.inc
index 6db87ab336..0266f09119 100644
--- a/target/ppc/translate/vsx-impl.c.inc
+++ b/target/ppc/translate/vsx-impl.c.inc
@@ -2268,7 +2268,7 @@ static bool do_lstxv(DisasContext *ctx, int ra, TCGv displ,
 
 static bool do_lstxv_D(DisasContext *ctx, arg_D *a, bool store, bool paired)
 {
-    if (paired || a->rt >= 32) {
+    if (paired || a->rt < 32) {
         REQUIRE_VSX(ctx);
     } else {
         REQUIRE_VECTOR(ctx);
-- 
2.42.0



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

* Re: [PATCH] target/ppc: Fix lxv/stxv MSR facility check
  2024-02-13  8:39 [PATCH] target/ppc: Fix lxv/stxv MSR facility check Nicholas Piggin
@ 2024-02-13  9:18 ` Harsh Prateek Bora
  2024-02-14  7:35 ` Cédric Le Goater
  2024-09-09 23:06 ` Fabiano Rosas
  2 siblings, 0 replies; 5+ messages in thread
From: Harsh Prateek Bora @ 2024-02-13  9:18 UTC (permalink / raw)
  To: Nicholas Piggin, qemu-ppc
  Cc: Daniel Henrique Barboza, Cédric Le Goater,
	Lucas Mateus Castro, qemu-devel, qemu-stable, Joel Stanley



On 2/13/24 14:09, Nicholas Piggin wrote:
> The move to decodetree flipped the inequality test for the VEC / VSX
> MSR facility check.
> 
> This caused application crashes under Linux, where these facility
> unavailable interrupts are used for lazy-switching of VEC/VSX register
> sets. Getting the incorrect interrupt would result in wrong registers
> being loaded, potentially overwriting live values and/or exposing
> stale ones.
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Joel Stanley <joel@jms.id.au>
> Fixes: 70426b5bb738 ("target/ppc: moved stxvx and lxvx from legacy to decodtree")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1769
> Tested-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   target/ppc/translate/vsx-impl.c.inc | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/translate/vsx-impl.c.inc b/target/ppc/translate/vsx-impl.c.inc
> index 6db87ab336..0266f09119 100644
> --- a/target/ppc/translate/vsx-impl.c.inc
> +++ b/target/ppc/translate/vsx-impl.c.inc
> @@ -2268,7 +2268,7 @@ static bool do_lstxv(DisasContext *ctx, int ra, TCGv displ,
>   
>   static bool do_lstxv_D(DisasContext *ctx, arg_D *a, bool store, bool paired)
>   {
> -    if (paired || a->rt >= 32) {
> +    if (paired || a->rt < 32) {

Thanks for catching this,

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

>           REQUIRE_VSX(ctx);
>       } else {
>           REQUIRE_VECTOR(ctx);


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

* Re: [PATCH] target/ppc: Fix lxv/stxv MSR facility check
  2024-02-13  8:39 [PATCH] target/ppc: Fix lxv/stxv MSR facility check Nicholas Piggin
  2024-02-13  9:18 ` Harsh Prateek Bora
@ 2024-02-14  7:35 ` Cédric Le Goater
  2024-09-09 23:06 ` Fabiano Rosas
  2 siblings, 0 replies; 5+ messages in thread
From: Cédric Le Goater @ 2024-02-14  7:35 UTC (permalink / raw)
  To: Nicholas Piggin, qemu-ppc
  Cc: Daniel Henrique Barboza, Lucas Mateus Castro, qemu-devel,
	qemu-stable, Joel Stanley, Harsh Prateek Bora

On 2/13/24 09:39, Nicholas Piggin wrote:
> The move to decodetree flipped the inequality test for the VEC / VSX
> MSR facility check.
> 
> This caused application crashes under Linux, where these facility
> unavailable interrupts are used for lazy-switching of VEC/VSX register
> sets. Getting the incorrect interrupt would result in wrong registers
> being loaded, potentially overwriting live values and/or exposing
> stale ones.
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Joel Stanley <joel@jms.id.au>
> Fixes: 70426b5bb738 ("target/ppc: moved stxvx and lxvx from legacy to decodtree")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1769
> Tested-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Tested-by: Cédric Le Goater <clg@kaod.org>

with a RHEL9 image.

Thanks,

C.


> ---
>   target/ppc/translate/vsx-impl.c.inc | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/translate/vsx-impl.c.inc b/target/ppc/translate/vsx-impl.c.inc
> index 6db87ab336..0266f09119 100644
> --- a/target/ppc/translate/vsx-impl.c.inc
> +++ b/target/ppc/translate/vsx-impl.c.inc
> @@ -2268,7 +2268,7 @@ static bool do_lstxv(DisasContext *ctx, int ra, TCGv displ,
>   
>   static bool do_lstxv_D(DisasContext *ctx, arg_D *a, bool store, bool paired)
>   {
> -    if (paired || a->rt >= 32) {
> +    if (paired || a->rt < 32) {
>           REQUIRE_VSX(ctx);
>       } else {
>           REQUIRE_VECTOR(ctx);



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

* Re: [PATCH] target/ppc: Fix lxv/stxv MSR facility check
  2024-02-13  8:39 [PATCH] target/ppc: Fix lxv/stxv MSR facility check Nicholas Piggin
  2024-02-13  9:18 ` Harsh Prateek Bora
  2024-02-14  7:35 ` Cédric Le Goater
@ 2024-09-09 23:06 ` Fabiano Rosas
  2024-09-13  4:40   ` Harsh Prateek Bora
  2 siblings, 1 reply; 5+ messages in thread
From: Fabiano Rosas @ 2024-09-09 23:06 UTC (permalink / raw)
  To: Nicholas Piggin, qemu-ppc
  Cc: Nicholas Piggin, Daniel Henrique Barboza, Cédric Le Goater,
	Lucas Mateus Castro, qemu-devel, qemu-stable, Joel Stanley,
	Harsh Prateek Bora

Nicholas Piggin <npiggin@gmail.com> writes:

> The move to decodetree flipped the inequality test for the VEC / VSX
> MSR facility check.
>
> This caused application crashes under Linux, where these facility
> unavailable interrupts are used for lazy-switching of VEC/VSX register
> sets. Getting the incorrect interrupt would result in wrong registers
> being loaded, potentially overwriting live values and/or exposing
> stale ones.
>
> Cc: qemu-stable@nongnu.org
> Reported-by: Joel Stanley <joel@jms.id.au>
> Fixes: 70426b5bb738 ("target/ppc: moved stxvx and lxvx from legacy to decodtree")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1769
> Tested-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  target/ppc/translate/vsx-impl.c.inc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/ppc/translate/vsx-impl.c.inc b/target/ppc/translate/vsx-impl.c.inc
> index 6db87ab336..0266f09119 100644
> --- a/target/ppc/translate/vsx-impl.c.inc
> +++ b/target/ppc/translate/vsx-impl.c.inc
> @@ -2268,7 +2268,7 @@ static bool do_lstxv(DisasContext *ctx, int ra, TCGv displ,
>  
>  static bool do_lstxv_D(DisasContext *ctx, arg_D *a, bool store, bool paired)
>  {
> -    if (paired || a->rt >= 32) {
> +    if (paired || a->rt < 32) {
>          REQUIRE_VSX(ctx);
>      } else {
>          REQUIRE_VECTOR(ctx);

What about the X-form down below?

static bool do_lstxv_X(DisasContext *ctx, arg_X *a, bool store, bool paired)
{
    if (paired || a->rt >= 32) {
        REQUIRE_VSX(ctx);
    } else {
        REQUIRE_VECTOR(ctx);
    }

    return do_lstxv(ctx, a->ra, cpu_gpr[a->rb], a->rt, store, paired);
}


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

* Re: [PATCH] target/ppc: Fix lxv/stxv MSR facility check
  2024-09-09 23:06 ` Fabiano Rosas
@ 2024-09-13  4:40   ` Harsh Prateek Bora
  0 siblings, 0 replies; 5+ messages in thread
From: Harsh Prateek Bora @ 2024-09-13  4:40 UTC (permalink / raw)
  To: Fabiano Rosas, Nicholas Piggin, qemu-ppc
  Cc: Daniel Henrique Barboza, Cédric Le Goater,
	Lucas Mateus Castro, qemu-devel, qemu-stable, Joel Stanley

Hi Fabiano,

On 9/10/24 04:36, Fabiano Rosas wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
> 
>> The move to decodetree flipped the inequality test for the VEC / VSX
>> MSR facility check.
>>
>> This caused application crashes under Linux, where these facility
>> unavailable interrupts are used for lazy-switching of VEC/VSX register
>> sets. Getting the incorrect interrupt would result in wrong registers
>> being loaded, potentially overwriting live values and/or exposing
>> stale ones.
>>
>> Cc: qemu-stable@nongnu.org
>> Reported-by: Joel Stanley <joel@jms.id.au>
>> Fixes: 70426b5bb738 ("target/ppc: moved stxvx and lxvx from legacy to decodtree")
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1769
>> Tested-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   target/ppc/translate/vsx-impl.c.inc | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/ppc/translate/vsx-impl.c.inc b/target/ppc/translate/vsx-impl.c.inc
>> index 6db87ab336..0266f09119 100644
>> --- a/target/ppc/translate/vsx-impl.c.inc
>> +++ b/target/ppc/translate/vsx-impl.c.inc
>> @@ -2268,7 +2268,7 @@ static bool do_lstxv(DisasContext *ctx, int ra, TCGv displ,
>>   
>>   static bool do_lstxv_D(DisasContext *ctx, arg_D *a, bool store, bool paired)
>>   {
>> -    if (paired || a->rt >= 32) {
>> +    if (paired || a->rt < 32) {
>>           REQUIRE_VSX(ctx);
>>       } else {
>>           REQUIRE_VECTOR(ctx);
> 
> What about the X-form down below?
> 
> static bool do_lstxv_X(DisasContext *ctx, arg_X *a, bool store, bool paired)
> {
>      if (paired || a->rt >= 32) {
>          REQUIRE_VSX(ctx);
>      } else {
>          REQUIRE_VECTOR(ctx);
>      }
> 
>      return do_lstxv(ctx, a->ra, cpu_gpr[a->rb], a->rt, store, paired);
> }

Thanks for catching this. I have posted the fix here:
https://lore.kernel.org/qemu-devel/20240913043827.914457-1-harshpb@linux.ibm.com/T/#u

regards,
Harsh



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

end of thread, other threads:[~2024-09-13  4:41 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-13  8:39 [PATCH] target/ppc: Fix lxv/stxv MSR facility check Nicholas Piggin
2024-02-13  9:18 ` Harsh Prateek Bora
2024-02-14  7:35 ` Cédric Le Goater
2024-09-09 23:06 ` Fabiano Rosas
2024-09-13  4:40   ` Harsh Prateek Bora

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