[PATCH] Remove unassigned_access CPU hook

Peter Maydell posted 1 patch 4 years, 6 months ago
Test docker-clang@ubuntu failed
Test asan passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test checkpatch passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190920125008.13604-1-peter.maydell@linaro.org
Maintainers: Eduardo Habkost <ehabkost@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <rth@twiddle.net>
There is a newer version of this series
include/hw/core/cpu.h | 24 ------------------------
accel/tcg/cputlb.c    |  1 -
memory.c              |  7 -------
3 files changed, 32 deletions(-)
[PATCH] Remove unassigned_access CPU hook
Posted by Peter Maydell 4 years, 6 months ago
All targets have now migrated away from the old unassigned_access
hook to the new do_transaction_failed hook. This means we can remove
the core-code infrastructure for that hook and the code that calls it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Based-on: <cover.1568762497.git.alistair.francis@wdc.com>
("[PATCH v1 0/2] RISC-V: Convert to do_transaction_failed hook")
 -- the last of the conversions isn't in master yet, but I figured
I might as well put the cleanup patch out for review.

 include/hw/core/cpu.h | 24 ------------------------
 accel/tcg/cputlb.c    |  1 -
 memory.c              |  7 -------
 3 files changed, 32 deletions(-)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index c7cda65c66d..a5a13e26a20 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -71,10 +71,6 @@ typedef enum MMUAccessType {
 
 typedef struct CPUWatchpoint CPUWatchpoint;
 
-typedef void (*CPUUnassignedAccess)(CPUState *cpu, hwaddr addr,
-                                    bool is_write, bool is_exec, int opaque,
-                                    unsigned size);
-
 struct TranslationBlock;
 
 /**
@@ -86,8 +82,6 @@ struct TranslationBlock;
  * @reset_dump_flags: #CPUDumpFlags to use for reset logging.
  * @has_work: Callback for checking if there is work to do.
  * @do_interrupt: Callback for interrupt handling.
- * @do_unassigned_access: Callback for unassigned access handling.
- * (this is deprecated: new targets should use do_transaction_failed instead)
  * @do_unaligned_access: Callback for unaligned access handling, if
  * the target defines #TARGET_ALIGNED_ONLY.
  * @do_transaction_failed: Callback for handling failed memory transactions
@@ -174,7 +168,6 @@ typedef struct CPUClass {
     int reset_dump_flags;
     bool (*has_work)(CPUState *cpu);
     void (*do_interrupt)(CPUState *cpu);
-    CPUUnassignedAccess do_unassigned_access;
     void (*do_unaligned_access)(CPUState *cpu, vaddr addr,
                                 MMUAccessType access_type,
                                 int mmu_idx, uintptr_t retaddr);
@@ -414,12 +407,6 @@ struct CPUState {
      */
     uintptr_t mem_io_pc;
     vaddr mem_io_vaddr;
-    /*
-     * This is only needed for the legacy cpu_unassigned_access() hook;
-     * when all targets using it have been converted to use
-     * cpu_transaction_failed() instead it can be removed.
-     */
-    MMUAccessType mem_io_access_type;
 
     int kvm_fd;
     struct KVMState *kvm_state;
@@ -879,17 +866,6 @@ void cpu_interrupt(CPUState *cpu, int mask);
 #ifdef NEED_CPU_H
 
 #ifdef CONFIG_SOFTMMU
-static inline void cpu_unassigned_access(CPUState *cpu, hwaddr addr,
-                                         bool is_write, bool is_exec,
-                                         int opaque, unsigned size)
-{
-    CPUClass *cc = CPU_GET_CLASS(cpu);
-
-    if (cc->do_unassigned_access) {
-        cc->do_unassigned_access(cpu, addr, is_write, is_exec, opaque, size);
-    }
-}
-
 static inline void cpu_unaligned_access(CPUState *cpu, vaddr addr,
                                         MMUAccessType access_type,
                                         int mmu_idx, uintptr_t retaddr)
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index abae79650c0..9c21b320eb4 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -914,7 +914,6 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
     }
 
     cpu->mem_io_vaddr = addr;
-    cpu->mem_io_access_type = access_type;
 
     if (mr->global_locking && !qemu_mutex_iothread_locked()) {
         qemu_mutex_lock_iothread();
diff --git a/memory.c b/memory.c
index b9dd6b94cac..93a05395cff 100644
--- a/memory.c
+++ b/memory.c
@@ -1278,10 +1278,6 @@ static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
 #ifdef DEBUG_UNASSIGNED
     printf("Unassigned mem read " TARGET_FMT_plx "\n", addr);
 #endif
-    if (current_cpu != NULL) {
-        bool is_exec = current_cpu->mem_io_access_type == MMU_INST_FETCH;
-        cpu_unassigned_access(current_cpu, addr, false, is_exec, 0, size);
-    }
     return 0;
 }
 
@@ -1291,9 +1287,6 @@ static void unassigned_mem_write(void *opaque, hwaddr addr,
 #ifdef DEBUG_UNASSIGNED
     printf("Unassigned mem write " TARGET_FMT_plx " = 0x%"PRIx64"\n", addr, val);
 #endif
-    if (current_cpu != NULL) {
-        cpu_unassigned_access(current_cpu, addr, true, false, 0, size);
-    }
 }
 
 static bool unassigned_mem_accepts(void *opaque, hwaddr addr,
-- 
2.20.1


Re: [PATCH] Remove unassigned_access CPU hook
Posted by Alistair Francis 4 years, 6 months ago
On Fri, Sep 20, 2019 at 5:52 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> All targets have now migrated away from the old unassigned_access
> hook to the new do_transaction_failed hook. This means we can remove
> the core-code infrastructure for that hook and the code that calls it.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> Based-on: <cover.1568762497.git.alistair.francis@wdc.com>
> ("[PATCH v1 0/2] RISC-V: Convert to do_transaction_failed hook")
>  -- the last of the conversions isn't in master yet, but I figured
> I might as well put the cleanup patch out for review.
>
>  include/hw/core/cpu.h | 24 ------------------------
>  accel/tcg/cputlb.c    |  1 -
>  memory.c              |  7 -------
>  3 files changed, 32 deletions(-)
>
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index c7cda65c66d..a5a13e26a20 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -71,10 +71,6 @@ typedef enum MMUAccessType {
>
>  typedef struct CPUWatchpoint CPUWatchpoint;
>
> -typedef void (*CPUUnassignedAccess)(CPUState *cpu, hwaddr addr,
> -                                    bool is_write, bool is_exec, int opaque,
> -                                    unsigned size);
> -
>  struct TranslationBlock;
>
>  /**
> @@ -86,8 +82,6 @@ struct TranslationBlock;
>   * @reset_dump_flags: #CPUDumpFlags to use for reset logging.
>   * @has_work: Callback for checking if there is work to do.
>   * @do_interrupt: Callback for interrupt handling.
> - * @do_unassigned_access: Callback for unassigned access handling.
> - * (this is deprecated: new targets should use do_transaction_failed instead)
>   * @do_unaligned_access: Callback for unaligned access handling, if
>   * the target defines #TARGET_ALIGNED_ONLY.
>   * @do_transaction_failed: Callback for handling failed memory transactions
> @@ -174,7 +168,6 @@ typedef struct CPUClass {
>      int reset_dump_flags;
>      bool (*has_work)(CPUState *cpu);
>      void (*do_interrupt)(CPUState *cpu);
> -    CPUUnassignedAccess do_unassigned_access;
>      void (*do_unaligned_access)(CPUState *cpu, vaddr addr,
>                                  MMUAccessType access_type,
>                                  int mmu_idx, uintptr_t retaddr);
> @@ -414,12 +407,6 @@ struct CPUState {
>       */
>      uintptr_t mem_io_pc;
>      vaddr mem_io_vaddr;
> -    /*
> -     * This is only needed for the legacy cpu_unassigned_access() hook;
> -     * when all targets using it have been converted to use
> -     * cpu_transaction_failed() instead it can be removed.
> -     */
> -    MMUAccessType mem_io_access_type;
>
>      int kvm_fd;
>      struct KVMState *kvm_state;
> @@ -879,17 +866,6 @@ void cpu_interrupt(CPUState *cpu, int mask);
>  #ifdef NEED_CPU_H
>
>  #ifdef CONFIG_SOFTMMU
> -static inline void cpu_unassigned_access(CPUState *cpu, hwaddr addr,
> -                                         bool is_write, bool is_exec,
> -                                         int opaque, unsigned size)
> -{
> -    CPUClass *cc = CPU_GET_CLASS(cpu);
> -
> -    if (cc->do_unassigned_access) {
> -        cc->do_unassigned_access(cpu, addr, is_write, is_exec, opaque, size);
> -    }
> -}
> -
>  static inline void cpu_unaligned_access(CPUState *cpu, vaddr addr,
>                                          MMUAccessType access_type,
>                                          int mmu_idx, uintptr_t retaddr)
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index abae79650c0..9c21b320eb4 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -914,7 +914,6 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>      }
>
>      cpu->mem_io_vaddr = addr;
> -    cpu->mem_io_access_type = access_type;
>
>      if (mr->global_locking && !qemu_mutex_iothread_locked()) {
>          qemu_mutex_lock_iothread();
> diff --git a/memory.c b/memory.c
> index b9dd6b94cac..93a05395cff 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1278,10 +1278,6 @@ static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
>  #ifdef DEBUG_UNASSIGNED
>      printf("Unassigned mem read " TARGET_FMT_plx "\n", addr);
>  #endif
> -    if (current_cpu != NULL) {
> -        bool is_exec = current_cpu->mem_io_access_type == MMU_INST_FETCH;
> -        cpu_unassigned_access(current_cpu, addr, false, is_exec, 0, size);
> -    }
>      return 0;
>  }
>
> @@ -1291,9 +1287,6 @@ static void unassigned_mem_write(void *opaque, hwaddr addr,
>  #ifdef DEBUG_UNASSIGNED
>      printf("Unassigned mem write " TARGET_FMT_plx " = 0x%"PRIx64"\n", addr, val);
>  #endif
> -    if (current_cpu != NULL) {
> -        cpu_unassigned_access(current_cpu, addr, true, false, 0, size);
> -    }
>  }
>
>  static bool unassigned_mem_accepts(void *opaque, hwaddr addr,
> --
> 2.20.1
>
>

Re: [PATCH] Remove unassigned_access CPU hook
Posted by Philippe Mathieu-Daudé 4 years, 6 months ago
Hi Peter,

On 9/20/19 2:50 PM, Peter Maydell wrote:
> All targets have now migrated away from the old unassigned_access
> hook to the new do_transaction_failed hook. This means we can remove
> the core-code infrastructure for that hook and the code that calls it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Based-on: <cover.1568762497.git.alistair.francis@wdc.com>
> ("[PATCH v1 0/2] RISC-V: Convert to do_transaction_failed hook")
>  -- the last of the conversions isn't in master yet, but I figured
> I might as well put the cleanup patch out for review.

Hopefully this hook is neither implemented by the RX/AVR targets posted
on the list recently.

>  include/hw/core/cpu.h | 24 ------------------------
>  accel/tcg/cputlb.c    |  1 -
>  memory.c              |  7 -------
>  3 files changed, 32 deletions(-)
> 
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index c7cda65c66d..a5a13e26a20 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -71,10 +71,6 @@ typedef enum MMUAccessType {
>  
>  typedef struct CPUWatchpoint CPUWatchpoint;
>  
> -typedef void (*CPUUnassignedAccess)(CPUState *cpu, hwaddr addr,
> -                                    bool is_write, bool is_exec, int opaque,
> -                                    unsigned size);
> -
>  struct TranslationBlock;
>  
>  /**
> @@ -86,8 +82,6 @@ struct TranslationBlock;
>   * @reset_dump_flags: #CPUDumpFlags to use for reset logging.
>   * @has_work: Callback for checking if there is work to do.
>   * @do_interrupt: Callback for interrupt handling.
> - * @do_unassigned_access: Callback for unassigned access handling.
> - * (this is deprecated: new targets should use do_transaction_failed instead)
>   * @do_unaligned_access: Callback for unaligned access handling, if
>   * the target defines #TARGET_ALIGNED_ONLY.
>   * @do_transaction_failed: Callback for handling failed memory transactions
> @@ -174,7 +168,6 @@ typedef struct CPUClass {
>      int reset_dump_flags;
>      bool (*has_work)(CPUState *cpu);
>      void (*do_interrupt)(CPUState *cpu);
> -    CPUUnassignedAccess do_unassigned_access;
>      void (*do_unaligned_access)(CPUState *cpu, vaddr addr,
>                                  MMUAccessType access_type,
>                                  int mmu_idx, uintptr_t retaddr);
> @@ -414,12 +407,6 @@ struct CPUState {
>       */
>      uintptr_t mem_io_pc;
>      vaddr mem_io_vaddr;
> -    /*
> -     * This is only needed for the legacy cpu_unassigned_access() hook;
> -     * when all targets using it have been converted to use
> -     * cpu_transaction_failed() instead it can be removed.
> -     */
> -    MMUAccessType mem_io_access_type;
>  
>      int kvm_fd;
>      struct KVMState *kvm_state;
> @@ -879,17 +866,6 @@ void cpu_interrupt(CPUState *cpu, int mask);
>  #ifdef NEED_CPU_H
>  
>  #ifdef CONFIG_SOFTMMU
> -static inline void cpu_unassigned_access(CPUState *cpu, hwaddr addr,
> -                                         bool is_write, bool is_exec,
> -                                         int opaque, unsigned size)
> -{
> -    CPUClass *cc = CPU_GET_CLASS(cpu);
> -
> -    if (cc->do_unassigned_access) {
> -        cc->do_unassigned_access(cpu, addr, is_write, is_exec, opaque, size);
> -    }
> -}
> -
>  static inline void cpu_unaligned_access(CPUState *cpu, vaddr addr,
>                                          MMUAccessType access_type,
>                                          int mmu_idx, uintptr_t retaddr)
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index abae79650c0..9c21b320eb4 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -914,7 +914,6 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
>      }
>  
>      cpu->mem_io_vaddr = addr;
> -    cpu->mem_io_access_type = access_type;
>  
>      if (mr->global_locking && !qemu_mutex_iothread_locked()) {
>          qemu_mutex_lock_iothread();
> diff --git a/memory.c b/memory.c
> index b9dd6b94cac..93a05395cff 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1278,10 +1278,6 @@ static uint64_t unassigned_mem_read(void *opaque, hwaddr addr,
>  #ifdef DEBUG_UNASSIGNED
>      printf("Unassigned mem read " TARGET_FMT_plx "\n", addr);
>  #endif
> -    if (current_cpu != NULL) {
> -        bool is_exec = current_cpu->mem_io_access_type == MMU_INST_FETCH;
> -        cpu_unassigned_access(current_cpu, addr, false, is_exec, 0, size);
> -    }
>      return 0;
>  }
>  
> @@ -1291,9 +1287,6 @@ static void unassigned_mem_write(void *opaque, hwaddr addr,
>  #ifdef DEBUG_UNASSIGNED
>      printf("Unassigned mem write " TARGET_FMT_plx " = 0x%"PRIx64"\n", addr, val);
>  #endif
> -    if (current_cpu != NULL) {
> -        cpu_unassigned_access(current_cpu, addr, true, false, 0, size);
> -    }
>  }

Having unassigned_mem_read/unassigned_mem_write with
CPUReadMemoryFunc/CPUWriteMemoryFunc prototype only used for logging is
rather confusing. We can kill them and use trace events instead in
memory_region_dispatch_read/write. I'll send a follow-up cleanup patch.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>  
>  static bool unassigned_mem_accepts(void *opaque, hwaddr addr,
> 


Re: [PATCH] Remove unassigned_access CPU hook
Posted by Peter Maydell 4 years, 6 months ago
On Fri, 20 Sep 2019 at 14:36, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Hi Peter,
>
> On 9/20/19 2:50 PM, Peter Maydell wrote:
> > All targets have now migrated away from the old unassigned_access
> > hook to the new do_transaction_failed hook. This means we can remove
> > the core-code infrastructure for that hook and the code that calls it.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > Based-on: <cover.1568762497.git.alistair.francis@wdc.com>
> > ("[PATCH v1 0/2] RISC-V: Convert to do_transaction_failed hook")
> >  -- the last of the conversions isn't in master yet, but I figured
> > I might as well put the cleanup patch out for review.
>
> Hopefully this hook is neither implemented by the RX/AVR targets posted
> on the list recently.

Good point -- luckily a quick email archive search says they don't
try to implement the old hook.

> > @@ -1291,9 +1287,6 @@ static void unassigned_mem_write(void *opaque, hwaddr addr,
> >  #ifdef DEBUG_UNASSIGNED
> >      printf("Unassigned mem write " TARGET_FMT_plx " = 0x%"PRIx64"\n", addr, val);
> >  #endif
> > -    if (current_cpu != NULL) {
> > -        cpu_unassigned_access(current_cpu, addr, true, false, 0, size);
> > -    }
> >  }
>
> Having unassigned_mem_read/unassigned_mem_write with
> CPUReadMemoryFunc/CPUWriteMemoryFunc prototype only used for logging is
> rather confusing. We can kill them and use trace events instead in
> memory_region_dispatch_read/write. I'll send a follow-up cleanup patch.

You still need some function to do the "return 0" on read, though, don't you?
(Having unassigned_mem_accepts returning false also leaves me a bit
confused about when these functions would actually get called, now
I look at the code again...)

thanks
-- PMM