diff mbox series

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

Message ID 20210909203439.4114179-3-matheus.ferst@eldorado.org.br (mailing list archive)
State New, archived
Headers show
Series Require hypervisor privilege for tlbie[l] when PSR=0 and HR=1. | expand

Commit Message

Matheus K. Ferst Sept. 9, 2021, 8:34 p.m. UTC
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(-)

Comments

Daniel Henrique Barboza Sept. 13, 2021, 7:38 p.m. UTC | #1
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)) {
>
diff mbox series

Patch

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