diff mbox series

Remove unassigned_access CPU hook

Message ID 20190920125008.13604-1-peter.maydell@linaro.org
State Superseded
Headers show
Series Remove unassigned_access CPU hook | expand

Commit Message

Peter Maydell Sept. 20, 2019, 12:50 p.m. UTC 
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(-)

-- 
2.20.1

Comments

Philippe Mathieu-Daudé Sept. 20, 2019, 1:36 p.m. UTC | #1 
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,

>
Peter Maydell Sept. 20, 2019, 1:44 p.m. UTC | #2 
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
Alistair Francis Sept. 20, 2019, 6:45 p.m. UTC | #3 
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

>

>
diff mbox series

Patch

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,