Series comparison

-[PULL 0/2] target-arm queue
+[PULL 0/1] target-arm queue
-Hi; this pull request has a couple of fixes for bugs in
+The following changes since commit e3debd5e7d0ce031356024878a0a18b9d109354a:
 the Arm page-table-walk code, which arrived in the last
 day or so.
-I'm sending this out now in the hope it might just sneak
+  Merge tag 'pull-request-2023-03-24' of https://gitlab.com/thuth/qemu into staging (2023-03-24 16:08:46 +0000)
 in before rc2 gets tagged, so the fixes can get more
 testing time before the 7.2 release; but if they don't
 make it then this should go into rc3.
 thanks
 -- PMM
 The following changes since commit 6d71357a3b651ec9db126e4862b77e13165427f5:
   rtl8139: honor large send MSS value (2022-11-21 09:28:43 -0500)
 are available in the Git repository at:
-  https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20221122
+  https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20230328
-for you to fetch changes up to 15f8f4671afd22491ce99d28a296514717fead4f:
+for you to fetch changes up to 46e3b237c52e0c48bfd81bce020b51fbe300b23a:
-  target/arm: Use signed quantity to represent VMSAv8-64 translation level (2022-11-22 16:10:25 +0000)
+  target/arm/gdbstub: Only advertise M-profile features if TCG available (2023-03-28 10:53:40 +0100)
 ----------------------------------------------------------------
-target-arm:
+target-arm queue:
- * Fix broken 5-level pagetable handling
+ * fix part of the "TCG-disabled builds are broken" issue
  * Fix debug accesses when EL2 is present
 ----------------------------------------------------------------
-Ard Biesheuvel (1):
+Philippe Mathieu-Daudé (1):
-      target/arm: Use signed quantity to represent VMSAv8-64 translation level
+      target/arm/gdbstub: Only advertise M-profile features if TCG available
-Peter Maydell (1):
+ target/arm/gdbstub.c | 5 +++--
-      target/arm: Don't do two-stage lookup if stage 2 is disabled
+file changed, 3 insertions(+), 2 deletions(-)
- target/arm/ptw.c | 11 ++++++-----
-file changed, 6 insertions(+), 5 deletions(-)

-[PULL 1/2] target/arm: Don't do two-stage lookup if stage 2 is disabled
+Deleted patch
-In get_phys_addr_with_struct(), we call get_phys_addr_twostage() if
-the CPU supports EL2.  However, we don't check here that stage 2 is
-actually enabled.  Instead we only check that inside
-get_phys_addr_twostage() to skip stage 2 translation.  This means
-that even if stage 2 is disabled we still tell the stage 1 lookup to
-do its page table walks via stage 2.
-This works by luck for normal CPU accesses, but it breaks for debug
-accesses, which are used by the disassembler and also by semihosting
-file reads and writes, because the debug case takes a different code
-path inside S1_ptw_translate().
-This means that setups that use semihosting for file loads are broken
-(a regression since 7.1, introduced in recent ptw refactoring), and
-that sometimes disassembly in debug logs reports "unable to read
-memory" rather than showing the guest insns.
-Fix the bug by hoisting the "is stage 2 enabled?" check up to
-get_phys_addr_with_struct(), so that we handle S2 disabled the same
-way we do the "no EL2" case, with a simple single stage lookup.
-Reported-by: Jens Wiklander <jens.wiklander@linaro.org>
-Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
-Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
-Message-id: 20221121212404.1450382-1-peter.maydell@linaro.org
----
- target/arm/ptw.c | 7 ++++---
-file changed, 4 insertions(+), 3 deletions(-)
-diff --git a/target/arm/ptw.c b/target/arm/ptw.c
-index XXXXXXX..XXXXXXX 100644
---- a/target/arm/ptw.c
-+++ b/target/arm/ptw.c
-@@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw,
-     ret = get_phys_addr_with_struct(env, ptw, address, access_type, result, fi);
--    /* If S1 fails or S2 is disabled, return early.  */
--    if (ret || regime_translation_disabled(env, ARMMMUIdx_Stage2, is_secure)) {
-+    /* If S1 fails, return early.  */
-+    if (ret) {
-         return ret;
-     }
-@@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
-          * Otherwise, a stage1+stage2 translation is just stage 1.
-          */
-         ptw->in_mmu_idx = mmu_idx = s1_mmu_idx;
--        if (arm_feature(env, ARM_FEATURE_EL2)) {
-+        if (arm_feature(env, ARM_FEATURE_EL2) &&
-+            !regime_translation_disabled(env, ARMMMUIdx_Stage2, is_secure)) {
-             return get_phys_addr_twostage(env, ptw, address, access_type,
-                                           result, fi);
-         }
---
-.25.1

-[PULL 2/2] target/arm: Use signed quantity to represent VMSAv8-64 translation level
+[PULL 1/1] target/arm/gdbstub: Only advertise M-profile features if TCG available
-From: Ard Biesheuvel <ardb@kernel.org>
+From: Philippe Mathieu-Daudé <philmd@linaro.org>
-The LPA2 extension implements 52-bit virtual addressing for 4k and 16k
+Cortex-M profile is only emulable from TCG accelerator. Restrict
-translation granules, and for the former, this means an additional level
+the GDBstub features to its availability in order to avoid a link
-of translation is needed. This means we start counting at -1 instead of
+error when TCG is not enabled:
 when doing a walk, and so 'level' is now a signed quantity, and should
 be typed as such. So turn it from uint32_t into int32_t.
-This avoids a level of -1 getting misinterpreted as being >= 3, and
+  Undefined symbols for architecture arm64:
-terminating a page table walk prematurely with a bogus output address.
+    "_arm_v7m_get_sp_ptr", referenced from:
         _m_sysreg_get in target_arm_gdbstub.c.o
     "_arm_v7m_mrs_control", referenced from:
         _arm_gdb_get_m_systemreg in target_arm_gdbstub.c.o
   ld: symbol(s) not found for architecture arm64
   clang: error: linker command failed with exit code 1 (use -v to see invocation)
-Cc: Peter Maydell <peter.maydell@linaro.org>
+Fixes: 7d8b28b8b5 ("target/arm: Implement gdbstub m-profile systemreg and secext")
-Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
+Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
-Cc: Richard Henderson <richard.henderson@linaro.org>
+Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
-Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
+Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
-Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
+Message-id: 20230322142902.69511-3-philmd@linaro.org
 [PMM: add #include since I cherry-picked this patch from the series]
 Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
 ---
- target/arm/ptw.c | 4 ++--
+ target/arm/gdbstub.c | 5 +++--
-file changed, 2 insertions(+), 2 deletions(-)
+file changed, 3 insertions(+), 2 deletions(-)
-diff --git a/target/arm/ptw.c b/target/arm/ptw.c
+diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
 index XXXXXXX..XXXXXXX 100644
---- a/target/arm/ptw.c
+--- a/target/arm/gdbstub.c
-+++ b/target/arm/ptw.c
++++ b/target/arm/gdbstub.c
-@@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
+@@ -XXX,XX +XXX,XX @@
-     ARMCPU *cpu = env_archcpu(env);
+ #include "cpu.h"
-     ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
+ #include "exec/gdbstub.h"
-     bool is_secure = ptw->in_secure;
+ #include "gdbstub/helpers.h"
--    uint32_t level;
++#include "sysemu/tcg.h"
-+    int32_t level;
+ #include "internals.h"
-     ARMVAParameters param;
+ #include "cpregs.h"
-     uint64_t ttbr;
-     hwaddr descaddr, indexmask, indexmask_grainsize;
+@@ -XXX,XX +XXX,XX @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
-@@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
+, "arm-vfp-sysregs.xml", 0);
-          */
+         }
-         uint32_t sl0 = extract32(tcr, 6, 2);
+     }
-         uint32_t sl2 = extract64(tcr, 33, 1);
+-    if (cpu_isar_feature(aa32_mve, cpu)) {
--        uint32_t startlevel;
++    if (cpu_isar_feature(aa32_mve, cpu) && tcg_enabled()) {
-+        int32_t startlevel;
+         gdb_register_coprocessor(cs, mve_gdb_get_reg, mve_gdb_set_reg,
-         bool ok;
+, "arm-m-profile-mve.xml", 0);
+     }
-         /* SL2 is RES0 unless DS=1 & 4kb granule. */
+@@ -XXX,XX +XXX,XX @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
                               arm_gen_dynamic_sysreg_xml(cs, cs->gdb_num_regs),
                               "system-registers.xml", 0);
 -    if (arm_feature(env, ARM_FEATURE_M)) {
 +    if (arm_feature(env, ARM_FEATURE_M) && tcg_enabled()) {
          gdb_register_coprocessor(cs,
              arm_gdb_get_m_systemreg, arm_gdb_set_m_systemreg,
              arm_gen_dynamic_m_systemreg_xml(cs, cs->gdb_num_regs),
 --
-.25.1
+.34.1

Hi; this pull request has a couple of fixes for bugs in
the Arm page-table-walk code, which arrived in the last
day or so.

I'm sending this out now in the hope it might just sneak
in before rc2 gets tagged, so the fixes can get more
testing time before the 7.2 release; but if they don't
make it then this should go into rc3.

thanks
-- PMM

The following changes since commit 6d71357a3b651ec9db126e4862b77e13165427f5:

rtl8139: honor large send MSS value (2022-11-21 09:28:43 -0500)

are available in the Git repository at:

https://git.linaro.org/people/pmaydell/qemu-arm.git tags/pull-target-arm-20221122

for you to fetch changes up to 15f8f4671afd22491ce99d28a296514717fead4f:

target/arm: Use signed quantity to represent VMSAv8-64 translation level (2022-11-22 16:10:25 +0000)

----------------------------------------------------------------
target-arm:
 * Fix broken 5-level pagetable handling
 * Fix debug accesses when EL2 is present

----------------------------------------------------------------
Ard Biesheuvel (1):
      target/arm: Use signed quantity to represent VMSAv8-64 translation level

Peter Maydell (1):
      target/arm: Don't do two-stage lookup if stage 2 is disabled

target/arm/ptw.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

In get_phys_addr_with_struct(), we call get_phys_addr_twostage() if
the CPU supports EL2.  However, we don't check here that stage 2 is
actually enabled.  Instead we only check that inside
get_phys_addr_twostage() to skip stage 2 translation.  This means
that even if stage 2 is disabled we still tell the stage 1 lookup to
do its page table walks via stage 2.

This works by luck for normal CPU accesses, but it breaks for debug
accesses, which are used by the disassembler and also by semihosting
file reads and writes, because the debug case takes a different code
path inside S1_ptw_translate().

This means that setups that use semihosting for file loads are broken
(a regression since 7.1, introduced in recent ptw refactoring), and
that sometimes disassembly in debug logs reports "unable to read
memory" rather than showing the guest insns.

Fix the bug by hoisting the "is stage 2 enabled?" check up to
get_phys_addr_with_struct(), so that we handle S2 disabled the same
way we do the "no EL2" case, with a simple single stage lookup.

Reported-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Message-id: 20221121212404.1450382-1-peter.maydell@linaro.org
---
 target/arm/ptw.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index XXXXXXX..XXXXXXX 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_twostage(CPUARMState *env, S1Translate *ptw,
 
     ret = get_phys_addr_with_struct(env, ptw, address, access_type, result, fi);
 
-    /* If S1 fails or S2 is disabled, return early.  */
-    if (ret || regime_translation_disabled(env, ARMMMUIdx_Stage2, is_secure)) {
+    /* If S1 fails, return early.  */
+    if (ret) {
         return ret;
     }
 
@@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_with_struct(CPUARMState *env, S1Translate *ptw,
          * Otherwise, a stage1+stage2 translation is just stage 1.
          */
         ptw->in_mmu_idx = mmu_idx = s1_mmu_idx;
-        if (arm_feature(env, ARM_FEATURE_EL2)) {
+        if (arm_feature(env, ARM_FEATURE_EL2) &&
+            !regime_translation_disabled(env, ARMMMUIdx_Stage2, is_secure)) {
             return get_phys_addr_twostage(env, ptw, address, access_type,
                                           result, fi);
         }
-- 
2.25.1

From: Ard Biesheuvel <ardb@kernel.org>

The LPA2 extension implements 52-bit virtual addressing for 4k and 16k
translation granules, and for the former, this means an additional level
of translation is needed. This means we start counting at -1 instead of
0 when doing a walk, and so 'level' is now a signed quantity, and should
be typed as such. So turn it from uint32_t into int32_t.

This avoids a level of -1 getting misinterpreted as being >= 3, and
terminating a page table walk prematurely with a bogus output address.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/ptw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index XXXXXXX..XXXXXXX 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
     ARMCPU *cpu = env_archcpu(env);
     ARMMMUIdx mmu_idx = ptw->in_mmu_idx;
     bool is_secure = ptw->in_secure;
-    uint32_t level;
+    int32_t level;
     ARMVAParameters param;
     uint64_t ttbr;
     hwaddr descaddr, indexmask, indexmask_grainsize;
@@ -XXX,XX +XXX,XX @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw,
          */
         uint32_t sl0 = extract32(tcr, 6, 2);
         uint32_t sl2 = extract64(tcr, 33, 1);
-        uint32_t startlevel;
+        int32_t startlevel;
         bool ok;
 
         /* SL2 is RES0 unless DS=1 & 4kb granule. */
-- 
2.25.1

From: Philippe Mathieu-Daudé <philmd@linaro.org>

Cortex-M profile is only emulable from TCG accelerator. Restrict
the GDBstub features to its availability in order to avoid a link
error when TCG is not enabled:

Undefined symbols for architecture arm64:
    "_arm_v7m_get_sp_ptr", referenced from:
        _m_sysreg_get in target_arm_gdbstub.c.o
    "_arm_v7m_mrs_control", referenced from:
        _arm_gdb_get_m_systemreg in target_arm_gdbstub.c.o
  ld: symbol(s) not found for architecture arm64
  clang: error: linker command failed with exit code 1 (use -v to see invocation)

Fixes: 7d8b28b8b5 ("target/arm: Implement gdbstub m-profile systemreg and secext")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Message-id: 20230322142902.69511-3-philmd@linaro.org
[PMM: add #include since I cherry-picked this patch from the series]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/gdbstub.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index XXXXXXX..XXXXXXX 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -XXX,XX +XXX,XX @@
 #include "cpu.h"
 #include "exec/gdbstub.h"
 #include "gdbstub/helpers.h"
+#include "sysemu/tcg.h"
 #include "internals.h"
 #include "cpregs.h"
 
@@ -XXX,XX +XXX,XX @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
                                      2, "arm-vfp-sysregs.xml", 0);
         }
     }
-    if (cpu_isar_feature(aa32_mve, cpu)) {
+    if (cpu_isar_feature(aa32_mve, cpu) && tcg_enabled()) {
         gdb_register_coprocessor(cs, mve_gdb_get_reg, mve_gdb_set_reg,
                                  1, "arm-m-profile-mve.xml", 0);
     }
@@ -XXX,XX +XXX,XX @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
                              arm_gen_dynamic_sysreg_xml(cs, cs->gdb_num_regs),
                              "system-registers.xml", 0);
 
-    if (arm_feature(env, ARM_FEATURE_M)) {
+    if (arm_feature(env, ARM_FEATURE_M) && tcg_enabled()) {
         gdb_register_coprocessor(cs,
             arm_gdb_get_m_systemreg, arm_gdb_set_m_systemreg,
             arm_gen_dynamic_m_systemreg_xml(cs, cs->gdb_num_regs),
-- 
2.34.1