[Qemu-devel] [PULL 0/4] MIPS queue for September 12th, 2019

[Qemu-devel] [PULL 0/4] MIPS queue for September 12th, 2019

[Aleksandar Markovic]

From: Aleksandar Markovic <amarkovic@wavecomp.com>

The following changes since commit 6d2fdde42c3344099262431df6a3f429c509291d:

  Merge remote-tracking branch 'remotes/stsquad/tags/pull-testing-next-100919-2' into staging (2019-09-10 14:52:09 +0100)

are available in the git repository at:

  https://github.com/AMarkovic/qemu tags/mips-queue-sep-12-2019

for you to fetch changes up to d1cc1533509012916dceeb7f23accda8a9fee85c:

  target/mips: gdbstub: Revert commit 8e0b373 (2019-09-12 18:25:34 +0200)

----------------------------------------------------------------

MIPS queue for September 12th, 2019

Highlights:

  - switch to using do_transaction_failed() hook for MIPS' Jazz boards
  - revert a commit that caused problems with gdb's packet 'g' for MIPS

----------------------------------------------------------------

Libo Zhou (1):
  target/mips: gdbstub: Revert commit 8e0b373

Peter Maydell (3):
  hw/mips/mips_jazz: Override do_transaction_failed hook
  target/mips: Switch to do_transaction_failed() hook
  hw/mips/mips_jazz: Remove no-longer-necessary override of
    do_unassigned_access

 hw/mips/mips_jazz.c     | 47 +++++++++++++++++++++++++++++++++--------------
 target/mips/cpu.c       |  2 +-
 target/mips/gdbstub.c   |  3 +--
 target/mips/internal.h  |  8 +++++---
 target/mips/op_helper.c | 24 ++++++++----------------
 5 files changed, 48 insertions(+), 36 deletions(-)

-- 
2.7.4

[Qemu-devel] [PULL 1/4] hw/mips/mips_jazz: Override do_transaction_failed hook

[Aleksandar Markovic]

From: Peter Maydell <peter.maydell@linaro.org>

The MIPS Jazz ('magnum' and 'pica61') boards have some code which
overrides the CPU's do_unassigned_access hook, so they can intercept
it and not raise exceptions on data accesses to invalid addresses,
only for instruction fetches.

We want to switch MIPS over to using the do_transaction_failed
hook instead, so add an intercept for that as well, and make
the board code install whichever hook the CPU is actually using.
Once we've changed the CPU implementation we can remove the
redundant code for the old hook.

Note: I am suspicious that the behaviour as implemented here may not
be what the hardware really does.  It was added in commit
54e755588cf1e90f0b14 to restore the behaviour that was broken by
commit c658b94f6e8c206c59d.  But prior to commit c658b94f6e8c206c59d
every MIPS board generated exceptions for instruction access to
invalid addresses but not for data accesses; and other boards,
notably Malta, were fixed by making all invalid accesses behave as
reads-as-zero (see the call to empty_slot_init() in
mips_malta_init()).  Hardware that raises exceptions for instruction
access and not data access seems to me to be an unlikely design, and
it's possible that the right way to emulate this is to make the Jazz
boards do what we did with Malta (or some variation of that).
Nonetheless, since I don't have access to real hardware to test
against I have taken the approach of "make QEMU continue to behave
the same way it did before this commit".  I have updated the comment
to correct the parts that are no longer accurate and note that
the hardware might behave differently.

The test case for the need for the hook-hijacking is in
https://bugs.launchpad.net/qemu/+bug/1245924 That BIOS will boot OK
either with this overriding of both hooks, or with a simple "global
memory region to ignore bad accesses of all types", so it doesn't
provide evidence either way, unfortunately.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Hervé Poussineau <hpoussin@reactos.org>
Message-Id: <20190802160458.25681-2-peter.maydell@linaro.org>
---
 hw/mips/mips_jazz.c | 54 +++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 46 insertions(+), 8 deletions(-)

diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c
index 388c15c..1a8e847 100644
--- a/hw/mips/mips_jazz.c
+++ b/hw/mips/mips_jazz.c
@@ -123,6 +123,28 @@ static void mips_jazz_do_unassigned_access(CPUState *cpu, hwaddr addr,
     (*real_do_unassigned_access)(cpu, addr, is_write, is_exec, opaque, size);
 }
 
+static void (*real_do_transaction_failed)(CPUState *cpu, hwaddr physaddr,
+                                          vaddr addr, unsigned size,
+                                          MMUAccessType access_type,
+                                          int mmu_idx, MemTxAttrs attrs,
+                                          MemTxResult response,
+                                          uintptr_t retaddr);
+
+static void mips_jazz_do_transaction_failed(CPUState *cs, hwaddr physaddr,
+                                            vaddr addr, unsigned size,
+                                            MMUAccessType access_type,
+                                            int mmu_idx, MemTxAttrs attrs,
+                                            MemTxResult response,
+                                            uintptr_t retaddr)
+{
+    if (access_type != MMU_INST_FETCH) {
+        /* ignore invalid access (ie do not raise exception) */
+        return;
+    }
+    (*real_do_transaction_failed)(cs, physaddr, addr, size, access_type,
+                                  mmu_idx, attrs, response, retaddr);
+}
+
 static void mips_jazz_init(MachineState *machine,
                            enum jazz_model_e jazz_model)
 {
@@ -157,16 +179,32 @@ static void mips_jazz_init(MachineState *machine,
     env = &cpu->env;
     qemu_register_reset(main_cpu_reset, cpu);
 
-    /* Chipset returns 0 in invalid reads and do not raise data exceptions.
+    /*
+     * Chipset returns 0 in invalid reads and do not raise data exceptions.
      * However, we can't simply add a global memory region to catch
-     * everything, as memory core directly call unassigned_mem_read/write
-     * on some invalid accesses, which call do_unassigned_access on the
-     * CPU, which raise an exception.
-     * Handle that case by hijacking the do_unassigned_access method on
-     * the CPU, and do not raise exceptions for data access. */
+     * everything, as this would make all accesses including instruction
+     * accesses be ignored and not raise exceptions.
+     * So instead we hijack either the do_unassigned_access method or
+     * the do_transaction_failed method on the CPU, and do not raise exceptions
+     * for data access.
+     *
+     * NOTE: this behaviour of raising exceptions for bad instruction
+     * fetches but not bad data accesses was added in commit 54e755588cf1e9
+     * to restore behaviour broken by c658b94f6e8c206, but it is not clear
+     * whether the real hardware behaves this way. It is possible that
+     * real hardware ignores bad instruction fetches as well -- if so then
+     * we could replace this hijacking of CPU methods with a simple global
+     * memory region that catches all memory accesses, as we do on Malta.
+     */
     cc = CPU_GET_CLASS(cpu);
-    real_do_unassigned_access = cc->do_unassigned_access;
-    cc->do_unassigned_access = mips_jazz_do_unassigned_access;
+    if (cc->do_unassigned_access) {
+        real_do_unassigned_access = cc->do_unassigned_access;
+        cc->do_unassigned_access = mips_jazz_do_unassigned_access;
+    }
+    if (cc->do_transaction_failed) {
+        real_do_transaction_failed = cc->do_transaction_failed;
+        cc->do_transaction_failed = mips_jazz_do_transaction_failed;
+    }
 
     /* allocate RAM */
     memory_region_allocate_system_memory(ram, NULL, "mips_jazz.ram",
-- 
2.7.4

[Qemu-devel] [PULL 2/4] target/mips: Switch to do_transaction_failed() hook

[Aleksandar Markovic]

From: Peter Maydell <peter.maydell@linaro.org>

Switch the MIPS target from the old unassigned_access hook to the new
do_transaction_failed hook.

Unlike the old hook, do_transaction_failed is only ever called from
the TCG memory access paths, so there is no need for the "ignore this
if we're using KVM" hack that we were previously using to work around
the way unassigned_access was called for all kinds of memory accesses
to unassigned physical addresses.

The MIPS target does not ever do direct memory reads by physical
address (via either ldl_phys etc or address_space_ldl etc), so the
only memory accesses this affects are the 'normal' guest loads and
stores, which will be handled by the new hook; their behaviour is
unchanged.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Hervé Poussineau <hpoussin@reactos.org>
Message-Id: <20190802160458.25681-3-peter.maydell@linaro.org>
---
 target/mips/cpu.c       |  2 +-
 target/mips/internal.h  |  8 +++++---
 target/mips/op_helper.c | 24 ++++++++----------------
 3 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 3ffa342..bbcf7ca 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -202,7 +202,7 @@ static void mips_cpu_class_init(ObjectClass *c, void *data)
     cc->gdb_read_register = mips_cpu_gdb_read_register;
     cc->gdb_write_register = mips_cpu_gdb_write_register;
 #ifndef CONFIG_USER_ONLY
-    cc->do_unassigned_access = mips_cpu_unassigned_access;
+    cc->do_transaction_failed = mips_cpu_do_transaction_failed;
     cc->do_unaligned_access = mips_cpu_do_unaligned_access;
     cc->get_phys_page_debug = mips_cpu_get_phys_page_debug;
     cc->vmsd = &vmstate_mips_cpu;
diff --git a/target/mips/internal.h b/target/mips/internal.h
index ae29b57..685e8d6 100644
--- a/target/mips/internal.h
+++ b/target/mips/internal.h
@@ -139,9 +139,11 @@ void r4k_helper_tlbinv(CPUMIPSState *env);
 void r4k_helper_tlbinvf(CPUMIPSState *env);
 void r4k_invalidate_tlb(CPUMIPSState *env, int idx, int use_extra);
 
-void mips_cpu_unassigned_access(CPUState *cpu, hwaddr addr,
-                                bool is_write, bool is_exec, int unused,
-                                unsigned size);
+void mips_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
+                                    vaddr addr, unsigned size,
+                                    MMUAccessType access_type,
+                                    int mmu_idx, MemTxAttrs attrs,
+                                    MemTxResult response, uintptr_t retaddr);
 hwaddr cpu_mips_translate_address(CPUMIPSState *env, target_ulong address,
                                   int rw);
 #endif
diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
index 01b9e78..4de6465 100644
--- a/target/mips/op_helper.c
+++ b/target/mips/op_helper.c
@@ -2668,27 +2668,19 @@ void mips_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
     do_raise_exception_err(env, excp, error_code, retaddr);
 }
 
-void mips_cpu_unassigned_access(CPUState *cs, hwaddr addr,
-                                bool is_write, bool is_exec, int unused,
-                                unsigned size)
+void mips_cpu_do_transaction_failed(CPUState *cs, hwaddr physaddr,
+                                    vaddr addr, unsigned size,
+                                    MMUAccessType access_type,
+                                    int mmu_idx, MemTxAttrs attrs,
+                                    MemTxResult response, uintptr_t retaddr)
 {
     MIPSCPU *cpu = MIPS_CPU(cs);
     CPUMIPSState *env = &cpu->env;
 
-    /*
-     * Raising an exception with KVM enabled will crash because it won't be from
-     * the main execution loop so the longjmp won't have a matching setjmp.
-     * Until we can trigger a bus error exception through KVM lets just ignore
-     * the access.
-     */
-    if (kvm_enabled()) {
-        return;
-    }
-
-    if (is_exec) {
-        raise_exception(env, EXCP_IBE);
+    if (access_type == MMU_INST_FETCH) {
+        do_raise_exception(env, EXCP_IBE, retaddr);
     } else {
-        raise_exception(env, EXCP_DBE);
+        do_raise_exception(env, EXCP_DBE, retaddr);
     }
 }
 #endif /* !CONFIG_USER_ONLY */
-- 
2.7.4

[Qemu-devel] [PULL 3/4] hw/mips/mips_jazz: Remove no-longer-necessary override of do_unassigned_access

[Aleksandar Markovic]

From: Peter Maydell <peter.maydell@linaro.org>

Now that the MIPS CPU implementation uses the new
do_transaction_failed hook, we can remove the old code that handled
the do_unassigned_access hook.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Hervé Poussineau <hpoussin@reactos.org>
Message-Id: <20190802160458.25681-4-peter.maydell@linaro.org>
---
 hw/mips/mips_jazz.c | 27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c
index 1a8e847..c967b97 100644
--- a/hw/mips/mips_jazz.c
+++ b/hw/mips/mips_jazz.c
@@ -111,18 +111,6 @@ static const MemoryRegionOps dma_dummy_ops = {
 #define MAGNUM_BIOS_SIZE_MAX 0x7e000
 #define MAGNUM_BIOS_SIZE (BIOS_SIZE < MAGNUM_BIOS_SIZE_MAX ? BIOS_SIZE : MAGNUM_BIOS_SIZE_MAX)
 
-static CPUUnassignedAccess real_do_unassigned_access;
-static void mips_jazz_do_unassigned_access(CPUState *cpu, hwaddr addr,
-                                           bool is_write, bool is_exec,
-                                           int opaque, unsigned size)
-{
-    if (!is_exec) {
-        /* ignore invalid access (ie do not raise exception) */
-        return;
-    }
-    (*real_do_unassigned_access)(cpu, addr, is_write, is_exec, opaque, size);
-}
-
 static void (*real_do_transaction_failed)(CPUState *cpu, hwaddr physaddr,
                                           vaddr addr, unsigned size,
                                           MMUAccessType access_type,
@@ -184,9 +172,8 @@ static void mips_jazz_init(MachineState *machine,
      * However, we can't simply add a global memory region to catch
      * everything, as this would make all accesses including instruction
      * accesses be ignored and not raise exceptions.
-     * So instead we hijack either the do_unassigned_access method or
-     * the do_transaction_failed method on the CPU, and do not raise exceptions
-     * for data access.
+     * So instead we hijack the do_transaction_failed method on the CPU, and
+     * do not raise exceptions for data access.
      *
      * NOTE: this behaviour of raising exceptions for bad instruction
      * fetches but not bad data accesses was added in commit 54e755588cf1e9
@@ -197,14 +184,8 @@ static void mips_jazz_init(MachineState *machine,
      * memory region that catches all memory accesses, as we do on Malta.
      */
     cc = CPU_GET_CLASS(cpu);
-    if (cc->do_unassigned_access) {
-        real_do_unassigned_access = cc->do_unassigned_access;
-        cc->do_unassigned_access = mips_jazz_do_unassigned_access;
-    }
-    if (cc->do_transaction_failed) {
-        real_do_transaction_failed = cc->do_transaction_failed;
-        cc->do_transaction_failed = mips_jazz_do_transaction_failed;
-    }
+    real_do_transaction_failed = cc->do_transaction_failed;
+    cc->do_transaction_failed = mips_jazz_do_transaction_failed;
 
     /* allocate RAM */
     memory_region_allocate_system_memory(ram, NULL, "mips_jazz.ram",
-- 
2.7.4

[Qemu-devel] [PULL 4/4] target/mips: gdbstub: Revert commit 8e0b373

[Aleksandar Markovic]

From: Libo Zhou <zhlb29@foxmail.com>

Multiple reports from users were received regarding failures of
packet 'g' communication with gdb for some MIPS configurations.
It was found out (by bisecting) that the problematic commit is
8e0b373. Revert that commit until a better solution is developed.

Suggested-by: Aleksandar Markovic <amarkovic@wavecomp.com>
Signed-off-by: Libo Zhou <zhlb29@foxmail.com>
Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>
Message-Id: <1568207966-25202-1-git-send-email-aleksandar.markovic@rt-rk.com>
---
 target/mips/gdbstub.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/mips/gdbstub.c b/target/mips/gdbstub.c
index ebcc98b..bbb2544 100644
--- a/target/mips/gdbstub.c
+++ b/target/mips/gdbstub.c
@@ -38,7 +38,7 @@ int mips_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
             return gdb_get_regl(mem_buf, (int32_t)env->active_fpu.fcr0);
         default:
             if (env->CP0_Status & (1 << CP0St_FR)) {
-                return gdb_get_reg64(mem_buf,
+                return gdb_get_regl(mem_buf,
                     env->active_fpu.fpr[n - 38].d);
             } else {
                 return gdb_get_regl(mem_buf,
@@ -99,7 +99,6 @@ int mips_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
             break;
         default:
             if (env->CP0_Status & (1 << CP0St_FR)) {
-                uint64_t tmp = ldq_p(mem_buf);
                 env->active_fpu.fpr[n - 38].d = tmp;
             } else {
                 env->active_fpu.fpr[n - 38].w[FP_ENDIAN_IDX] = tmp;
-- 
2.7.4

Re: [Qemu-devel] [PULL 0/4] MIPS queue for September 12th, 2019

[Peter Maydell]

On Thu, 12 Sep 2019 at 17:30, Aleksandar Markovic
<aleksandar.markovic@rt-rk.com> wrote:
>
> From: Aleksandar Markovic <amarkovic@wavecomp.com>
>
> The following changes since commit 6d2fdde42c3344099262431df6a3f429c509291d:
>
>   Merge remote-tracking branch 'remotes/stsquad/tags/pull-testing-next-100919-2' into staging (2019-09-10 14:52:09 +0100)
>
> are available in the git repository at:
>
>   https://github.com/AMarkovic/qemu tags/mips-queue-sep-12-2019
>
> for you to fetch changes up to d1cc1533509012916dceeb7f23accda8a9fee85c:
>
>   target/mips: gdbstub: Revert commit 8e0b373 (2019-09-12 18:25:34 +0200)
>
> ----------------------------------------------------------------
>
> MIPS queue for September 12th, 2019
>
> Highlights:
>
>   - switch to using do_transaction_failed() hook for MIPS' Jazz boards
>   - revert a commit that caused problems with gdb's packet 'g' for MIPS


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.2
for any user-visible changes.

-- PMM