target/ppc: Fix sc instruction handling of LEV field

[PATCH] target/ppc: Fix sc instruction handling of LEV field

Posted by Nicholas Piggin 10 months, 2 weeks ago

The top bits of the LEV field of the sc instruction are to be treated as
as a reserved field rather than a reserved value, meaning LEV is
effectively the bottom bit. LEV=0xF should be treated as LEV=1 and be
a hypercall, for example.

This changes the instruction execution to just set lev from the low bit
of the field. Processors which don't support the LEV field will continue
to ignore it.

ISA v3.1 defines LEV to be 2 bits, in order to add the 'sc 2' ultracall
instruction. TCG does not support Ultravisor, so don't worry about
that bit.

Suggested-by: "Harsh Prateek Bora" <harshpb@linux.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
This should probably go ahead of the ISA 3.1 LEV in SRR1 patch. I
don't think they need to be backported to stable though, have not
caused any real problems.

Thanks to Harsh for spotting it.

Thanks,
Nick

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

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 15a00bd4fa..3c62f9188a 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -4424,7 +4424,12 @@ static void gen_sc(DisasContext *ctx)
 {
     uint32_t lev;
 
-    lev = (ctx->opcode >> 5) & 0x7F;
+    /*
+     * LEV is a 7-bit field, but the top 6 bits are treated as a reserved
+     * field (i.e., ignored). ISA v3.1 changes that to 5 bits, but that is
+     * for Ultravisor which TCG does not support, so just ignore the top 6.
+     */
+    lev = (ctx->opcode >> 5) & 0x1;
     gen_exception_err(ctx, POWERPC_SYSCALL, lev);
 }
 
-- 
2.40.1

Re: [PATCH] target/ppc: Fix sc instruction handling of LEV field

Posted by Cédric Le Goater 10 months, 2 weeks ago

On 6/21/23 13:09, Nicholas Piggin wrote:
> The top bits of the LEV field of the sc instruction are to be treated as
> as a reserved field rather than a reserved value, meaning LEV is
> effectively the bottom bit. LEV=0xF should be treated as LEV=1 and be
> a hypercall, for example.
> 
> This changes the instruction execution to just set lev from the low bit
> of the field. Processors which don't support the LEV field will continue
> to ignore it.
> 
> ISA v3.1 defines LEV to be 2 bits, in order to add the 'sc 2' ultracall
> instruction. TCG does not support Ultravisor, so don't worry about
> that bit.
> 
> Suggested-by: "Harsh Prateek Bora" <harshpb@linux.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> This should probably go ahead of the ISA 3.1 LEV in SRR1 patch. I
> don't think they need to be backported to stable though, have not
> caused any real problems.
> 
> Thanks to Harsh for spotting it.
> 
> Thanks,
> Nick
> 
>   target/ppc/translate.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 15a00bd4fa..3c62f9188a 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -4424,7 +4424,12 @@ static void gen_sc(DisasContext *ctx)
>   {
>       uint32_t lev;
>   
> -    lev = (ctx->opcode >> 5) & 0x7F;
> +    /*
> +     * LEV is a 7-bit field, but the top 6 bits are treated as a reserved
> +     * field (i.e., ignored). ISA v3.1 changes that to 5 bits, but that is
> +     * for Ultravisor which TCG does not support, so just ignore the top 6.
> +     */
> +    lev = (ctx->opcode >> 5) & 0x1;
>       gen_exception_err(ctx, POWERPC_SYSCALL, lev);
>   }
>   

Applied to ppc-next.

Thanks,

C.

Re: [PATCH] target/ppc: Fix sc instruction handling of LEV field

Posted by Harsh Prateek Bora 10 months, 2 weeks ago


On 6/21/23 16:39, Nicholas Piggin wrote:
> The top bits of the LEV field of the sc instruction are to be treated as
> as a reserved field rather than a reserved value, meaning LEV is
> effectively the bottom bit. LEV=0xF should be treated as LEV=1 and be
> a hypercall, for example.
> 
> This changes the instruction execution to just set lev from the low bit
> of the field. Processors which don't support the LEV field will continue
> to ignore it.
> 
> ISA v3.1 defines LEV to be 2 bits, in order to add the 'sc 2' ultracall
> instruction. TCG does not support Ultravisor, so don't worry about
> that bit.
> 
> Suggested-by: "Harsh Prateek Bora" <harshpb@linux.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> This should probably go ahead of the ISA 3.1 LEV in SRR1 patch. I
> don't think they need to be backported to stable though, have not
> caused any real problems.
> 
> Thanks to Harsh for spotting it.
> 
> Thanks,
> Nick
> 
>   target/ppc/translate.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 15a00bd4fa..3c62f9188a 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -4424,7 +4424,12 @@ static void gen_sc(DisasContext *ctx)
>   {
>       uint32_t lev;
>   
> -    lev = (ctx->opcode >> 5) & 0x7F;
> +    /*
> +     * LEV is a 7-bit field, but the top 6 bits are treated as a reserved
> +     * field (i.e., ignored). ISA v3.1 changes that to 5 bits, but that is
> +     * for Ultravisor which TCG does not support, so just ignore the top 6.
> +     */
> +    lev = (ctx->opcode >> 5) & 0x1;

should this change be applied to gen_scv() defined next to it as well ?

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

>       gen_exception_err(ctx, POWERPC_SYSCALL, lev);
>   }
>

Re: [PATCH] target/ppc: Fix sc instruction handling of LEV field

Posted by Nicholas Piggin 10 months, 2 weeks ago

On Thu Jun 22, 2023 at 6:00 PM AEST, Harsh Prateek Bora wrote:
>
>
> On 6/21/23 16:39, Nicholas Piggin wrote:
> > The top bits of the LEV field of the sc instruction are to be treated as
> > as a reserved field rather than a reserved value, meaning LEV is
> > effectively the bottom bit. LEV=0xF should be treated as LEV=1 and be
> > a hypercall, for example.
> > 
> > This changes the instruction execution to just set lev from the low bit
> > of the field. Processors which don't support the LEV field will continue
> > to ignore it.
> > 
> > ISA v3.1 defines LEV to be 2 bits, in order to add the 'sc 2' ultracall
> > instruction. TCG does not support Ultravisor, so don't worry about
> > that bit.
> > 
> > Suggested-by: "Harsh Prateek Bora" <harshpb@linux.ibm.com>
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> > This should probably go ahead of the ISA 3.1 LEV in SRR1 patch. I
> > don't think they need to be backported to stable though, have not
> > caused any real problems.
> > 
> > Thanks to Harsh for spotting it.
> > 
> > Thanks,
> > Nick
> > 
> >   target/ppc/translate.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > index 15a00bd4fa..3c62f9188a 100644
> > --- a/target/ppc/translate.c
> > +++ b/target/ppc/translate.c
> > @@ -4424,7 +4424,12 @@ static void gen_sc(DisasContext *ctx)
> >   {
> >       uint32_t lev;
> >   
> > -    lev = (ctx->opcode >> 5) & 0x7F;
> > +    /*
> > +     * LEV is a 7-bit field, but the top 6 bits are treated as a reserved
> > +     * field (i.e., ignored). ISA v3.1 changes that to 5 bits, but that is
> > +     * for Ultravisor which TCG does not support, so just ignore the top 6.
> > +     */
> > +    lev = (ctx->opcode >> 5) & 0x1;
>
> should this change be applied to gen_scv() defined next to it as well ?

No. scv uses all LEV bits.

Thanks,
Nick

>
> Otherwise,
> Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>
> >       gen_exception_err(ctx, POWERPC_SYSCALL, lev);
> >   }
> >