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

[PATCH v2 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   | 28 +++++++++++++++++++++++-----
 3 files changed, 27 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH v2 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>
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) {
-- 
2.25.1

[PATCH v2 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 | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 909a092fde..25ee9f75b2 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 || (!ctx->hv && !psr && ctx->hr)) {
+        /*
+         * tlbiel is privileged except when PSR=0 and HR=1, making it
+         * hypervisor privileged.
+         */
+        GEN_PRIV;
+    }
 
     gen_helper_tlbie(cpu_env, cpu_gpr[rB(ctx->opcode)]);
 #endif /* defined(CONFIG_USER_ONLY) */
@@ -5529,12 +5537,20 @@ 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) {
+        /* tlbie is privileged... */
+        GEN_PRIV;
+    } else if (!ctx->hv) {
+        if (!ctx->gtse || (!psr && ctx->hr)) {
+            /*
+             * ... except when GTSE=0 or when PSR=0 and HR=1, making it
+             * hypervisor privileged.
+             */
+            GEN_PRIV;
+        }
     }
 
     if (NARROW_MODE(ctx)) {
-- 
2.25.1

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

[Daniel Henrique Barboza]

On 9/17/21 08:47, matheus.ferst@eldorado.org.br wrote:
> 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]


For some reason I didn't receive these 2 patches in my mailbox, just this cover
letter. I reviewed both using the qemu-devel archives.


Both patches:

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



Thanks,


Daniel


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

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1963 bytes --]

On Fri, Sep 17, 2021 at 08:47:49AM -0300, matheus.ferst@eldorado.org.br wrote:
> 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/

Applied to ppc-for-6.2, thanks.

> 
> 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   | 28 +++++++++++++++++++++++-----
>  3 files changed, 27 insertions(+), 5 deletions(-)
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]