[PATCH 0/2] Require hypervisor privilege for tlbie[l] when PSR=0 and HR=1.

[PATCH 0/2] Require hypervisor privilege for tlbie[l] when PSR=0 and HR=1.

[matheus.ferst]

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

While working on FreeBSD radix support, Leandro Lupori (CC'ed) noticed
that the latest build still fails in KVM but works in TCG[1]. This
difference occurs because the current implementation of "tlbiel" does
not validate the instruction parameters and always check for supervisor
privilege.

This patch series partially address this problem by requiring hypervisor
privilege for radix mode when PSR=0. The validation of other parameters
can be done when we move storage control instructions to decodetree.

[1] To reproduce the issue, grab an ISO from [2] run qemu as

qemu-system-ppc64 -cpu power9 -m 2G \
    -machine pseries,cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off \
    -boot d -vga none -nographic -cdrom FreeBSD-14.0-CURRENT-powerpc-*.iso

or

qemu-system-ppc64 -cpu power9 -m 2G -enable-kvm \
    -machine pseries,cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off \
    -boot d -vga none -nographic -cdrom FreeBSD-14.0-CURRENT-powerpc-*.iso

Stop the boot at the prompt and use

OK set radix_mmu=1
OK boot

[2] https://download.freebsd.org/ftp/snapshots/powerpc/powerpc64/ISO-IMAGES/14.0/

Matheus Ferst (2):
  target/ppc: add LPCR[HR] to DisasContext and hflags
  target/ppc: Check privilege level based on PSR and LPCR[HR] in
    tlbie[l]

 target/ppc/cpu.h         |  1 +
 target/ppc/helper_regs.c |  3 +++
 target/ppc/translate.c   | 23 ++++++++++++++++++-----
 3 files changed, 22 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH 1/2] target/ppc: add LPCR[HR] to DisasContext and hflags

[matheus.ferst]

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

Add a Host Radix field (hr) in DisasContext with LPCR[HR] value to allow
us to decide between Radix and HPT while validating instructions
arguments. Note that PowerISA v3.1 does not require LPCR[HR] and PATE.HR
to match if the thread is in ultravisor/hypervisor real addressing mode,
so ctx->hr may be invalid if ctx->hv and ctx->dr are set.

Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
 target/ppc/cpu.h         | 1 +
 target/ppc/helper_regs.c | 3 +++
 target/ppc/translate.c   | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 500205229c..e1b8d343cd 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -600,6 +600,7 @@ enum {
     HFLAGS_64 = 2,   /* computed from MSR_CE and MSR_SF */
     HFLAGS_GTSE = 3, /* computed from SPR_LPCR[GTSE] */
     HFLAGS_DR = 4,   /* MSR_DR */
+    HFLAGS_HR = 5,   /* computed from SPR_LPCR[HR] */
     HFLAGS_SPE = 6,  /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR */
     HFLAGS_TM = 8,   /* computed from MSR_TM */
     HFLAGS_BE = 9,   /* MSR_BE -- from elsewhere on embedded ppc */
diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index 405450d863..1bfb480ecf 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -106,6 +106,9 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
     if (env->spr[SPR_LPCR] & LPCR_GTSE) {
         hflags |= 1 << HFLAGS_GTSE;
     }
+    if (env->spr[SPR_LPCR] & LPCR_HR) {
+        hflags |= 1 << HFLAGS_HR;
+    }
 
 #ifndef CONFIG_USER_ONLY
     if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 171b216e17..909a092fde 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -175,6 +175,7 @@ struct DisasContext {
     bool spe_enabled;
     bool tm_enabled;
     bool gtse;
+    bool hr;
     ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
     int singlestep_enabled;
     uint32_t flags;
@@ -8539,6 +8540,7 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     ctx->vsx_enabled = (hflags >> HFLAGS_VSX) & 1;
     ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
     ctx->gtse = (hflags >> HFLAGS_GTSE) & 1;
+    ctx->hr = (hflags >> HFLAGS_HR) & 1;
 
     ctx->singlestep_enabled = 0;
     if ((hflags >> HFLAGS_SE) & 1) {
-- 
2.25.1

Re: [PATCH 1/2] target/ppc: add LPCR[HR] to DisasContext and hflags

[Daniel Henrique Barboza]

On 9/9/21 5:34 PM, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> 
> Add a Host Radix field (hr) in DisasContext with LPCR[HR] value to allow
> us to decide between Radix and HPT while validating instructions
> arguments. Note that PowerISA v3.1 does not require LPCR[HR] and PATE.HR
> to match if the thread is in ultravisor/hypervisor real addressing mode,
> so ctx->hr may be invalid if ctx->hv and ctx->dr are set.
> 
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   target/ppc/cpu.h         | 1 +
>   target/ppc/helper_regs.c | 3 +++
>   target/ppc/translate.c   | 2 ++
>   3 files changed, 6 insertions(+)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 500205229c..e1b8d343cd 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -600,6 +600,7 @@ enum {
>       HFLAGS_64 = 2,   /* computed from MSR_CE and MSR_SF */
>       HFLAGS_GTSE = 3, /* computed from SPR_LPCR[GTSE] */
>       HFLAGS_DR = 4,   /* MSR_DR */
> +    HFLAGS_HR = 5,   /* computed from SPR_LPCR[HR] */
>       HFLAGS_SPE = 6,  /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR */
>       HFLAGS_TM = 8,   /* computed from MSR_TM */
>       HFLAGS_BE = 9,   /* MSR_BE -- from elsewhere on embedded ppc */
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 405450d863..1bfb480ecf 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -106,6 +106,9 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
>       if (env->spr[SPR_LPCR] & LPCR_GTSE) {
>           hflags |= 1 << HFLAGS_GTSE;
>       }
> +    if (env->spr[SPR_LPCR] & LPCR_HR) {
> +        hflags |= 1 << HFLAGS_HR;
> +    }
>   
>   #ifndef CONFIG_USER_ONLY
>       if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 171b216e17..909a092fde 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -175,6 +175,7 @@ struct DisasContext {
>       bool spe_enabled;
>       bool tm_enabled;
>       bool gtse;
> +    bool hr;
>       ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
>       int singlestep_enabled;
>       uint32_t flags;
> @@ -8539,6 +8540,7 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>       ctx->vsx_enabled = (hflags >> HFLAGS_VSX) & 1;
>       ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
>       ctx->gtse = (hflags >> HFLAGS_GTSE) & 1;
> +    ctx->hr = (hflags >> HFLAGS_HR) & 1;
>   
>       ctx->singlestep_enabled = 0;
>       if ((hflags >> HFLAGS_SE) & 1) {
> 

[PATCH 2/2] target/ppc: Check privilege level based on PSR and LPCR[HR] in tlbie[l]

[matheus.ferst]

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

PowerISA v3.0B made tlbie[l] hypervisor privileged when PSR=0 and HR=1.
To allow the check at translation time, we'll use the HR bit of LPCR to
check the MMU mode instead of the PATE.HR.

Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
 target/ppc/translate.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 909a092fde..154ab26872 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -5517,7 +5517,15 @@ static void gen_tlbiel(DisasContext *ctx)
 #if defined(CONFIG_USER_ONLY)
     GEN_PRIV;
 #else
-    CHK_SV;
+    bool psr = (ctx->opcode >> 17) & 0x1;
+
+    if (ctx->pr) {
+        GEN_PRIV;
+    } else if (!ctx->hv) {
+        if (!psr && ctx->hr) {
+            GEN_PRIV;
+        }
+    }
 
     gen_helper_tlbie(cpu_env, cpu_gpr[rB(ctx->opcode)]);
 #endif /* defined(CONFIG_USER_ONLY) */
@@ -5529,12 +5537,15 @@ static void gen_tlbie(DisasContext *ctx)
 #if defined(CONFIG_USER_ONLY)
     GEN_PRIV;
 #else
+    bool psr = (ctx->opcode >> 17) & 0x1;
     TCGv_i32 t1;
 
-    if (ctx->gtse) {
-        CHK_SV; /* If gtse is set then tlbie is supervisor privileged */
-    } else {
-        CHK_HV; /* Else hypervisor privileged */
+    if (ctx->pr) {
+        GEN_PRIV;
+    } else if (!ctx->hv) {
+        if (!ctx->gtse || (!psr && ctx->hr)) {
+            GEN_PRIV;
+        }
     }
 
     if (NARROW_MODE(ctx)) {
-- 
2.25.1

Re: [PATCH 2/2] target/ppc: Check privilege level based on PSR and LPCR[HR] in tlbie[l]

[Daniel Henrique Barboza]

On 9/9/21 5:34 PM, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> 
> PowerISA v3.0B made tlbie[l] hypervisor privileged when PSR=0 and HR=1.
> To allow the check at translation time, we'll use the HR bit of LPCR to
> check the MMU mode instead of the PATE.HR.
> 
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> ---




>   target/ppc/translate.c | 21 ++++++++++++++++-----
>   1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 909a092fde..154ab26872 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -5517,7 +5517,15 @@ static void gen_tlbiel(DisasContext *ctx)
>   #if defined(CONFIG_USER_ONLY)
>       GEN_PRIV;
>   #else
> -    CHK_SV;
> +    bool psr = (ctx->opcode >> 17) & 0x1;
> +
> +    if (ctx->pr) {
> +        GEN_PRIV;
> +    } else if (!ctx->hv) {
> +        if (!psr && ctx->hr) {
> +            GEN_PRIV;
> +        }
> +    }

You can avoid the third 'if' clause by adding all the conditions of the
second GEN_PRIV in the second if:


> +    if (ctx->pr) {
> +        GEN_PRIV;
> +    } else if (!ctx->hv && !psr && ctx->hr) {
> +        GEN_PRIV;
> +    }

Or, since all the code is doing is executing GEN_PRIV anyways:

> + if (ctx->pr || (!ctx->hv && !psr && ctx->hr)) {
> +     GEN_PRIV;
> + }


I think this is clearer than chaining 'if' clauses.

>   
>       gen_helper_tlbie(cpu_env, cpu_gpr[rB(ctx->opcode)]);
>   #endif /* defined(CONFIG_USER_ONLY) */
> @@ -5529,12 +5537,15 @@ static void gen_tlbie(DisasContext *ctx)
>   #if defined(CONFIG_USER_ONLY)
>       GEN_PRIV;
>   #else
> +    bool psr = (ctx->opcode >> 17) & 0x1;
>       TCGv_i32 t1;
>   
> -    if (ctx->gtse) {
> -        CHK_SV; /* If gtse is set then tlbie is supervisor privileged */
> -    } else {
> -        CHK_HV; /* Else hypervisor privileged */
> +    if (ctx->pr) {
> +        GEN_PRIV;
> +    } else if (!ctx->hv) {
> +        if (!ctx->gtse || (!psr && ctx->hr)) {
> +            GEN_PRIV;
> +        }
>       }

The same idea I mentioned above could be done here as well, but these are
not straightforward conditions to be done in a single IF clause and will
impact code reading. This is fine as is.


Thanks,


Daniel


>   
>       if (NARROW_MODE(ctx)) {
>