[PATCH 0/9] target/arm: Housekeeping around NVIC

[PATCH 0/9] target/arm: Housekeeping around NVIC

[Philippe Mathieu-Daudé]

Few cleanups while using link properties between CPU/NVIC:
- Restrict code to sysemu|useremu or tcg
- Simplify ID_PFR1 on useremu
- Move NVIC helpersto "hw/intc/armv7m_nvic.h"

Something odd occurs when an ARM CPU is realized, the
CPU reset handler is called, ending calling
armv7m_nvic_neg_prio_requested() on an unrealized NVIC.
I kludged by checking whether the NVIC is realized, but
this rather looks like a code smell...

Philippe Mathieu-Daudé (9):
  target/arm: Restrict v7-M MMU helpers to sysemu TCG
  target/arm: Constify ID_PFR1 on user emulation
  target/arm: Avoid resetting CPUARMState::eabi field
  target/arm: Restrict CPUARMState::arm_boot_info to sysemu
  target/arm: Restrict CPUARMState::gicv3state to sysemu
  target/arm: Restrict CPUARMState::nvic to sysemu and store as
    NVICState*
  target/arm: Declare CPU <-> NVIC helpers in 'hw/intc/armv7m_nvic.h'
  hw/intc/armv7m_nvic: Allow calling neg_prio_requested on unrealized
    NVIC
  hw/arm/armv7m: Pass CPU/NVIC using object_property_add_const_link()

 hw/arm/armv7m.c               |   4 +-
 hw/intc/armv7m_nvic.c         |  44 +++++------
 include/hw/intc/armv7m_nvic.h | 128 ++++++++++++++++++++++++++++++-
 target/arm/cpu.c              |   8 +-
 target/arm/cpu.h              | 137 ++--------------------------------
 target/arm/cpu_tcg.c          |   3 +
 target/arm/helper.c           |  14 +++-
 target/arm/m_helper.c         |   9 ++-
 8 files changed, 179 insertions(+), 168 deletions(-)

-- 
2.38.1

[PATCH 1/9] target/arm: Restrict v7-M MMU helpers to sysemu TCG

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/helper.c   | 2 +-
 target/arm/m_helper.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index c62ed05c12..5dbeade787 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -11774,7 +11774,7 @@ int arm_mmu_idx_to_el(ARMMMUIdx mmu_idx)
     }
 }
 
-#ifndef CONFIG_TCG
+#if !defined(CONFIG_TCG) || defined(CONFIG_USER_ONLY)
 ARMMMUIdx arm_v7m_mmu_idx_for_secstate(CPUARMState *env, bool secstate)
 {
     g_assert_not_reached();
diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index e7e746ea18..1e7e4e33bd 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -2854,8 +2854,6 @@ uint32_t HELPER(v7m_tt)(CPUARMState *env, uint32_t addr, uint32_t op)
     return tt_resp;
 }
 
-#endif /* !CONFIG_USER_ONLY */
-
 ARMMMUIdx arm_v7m_mmu_idx_all(CPUARMState *env,
                               bool secstate, bool priv, bool negpri)
 {
@@ -2892,3 +2890,5 @@ ARMMMUIdx arm_v7m_mmu_idx_for_secstate(CPUARMState *env, bool secstate)
 
     return arm_v7m_mmu_idx_for_secstate_and_priv(env, secstate, priv);
 }
+
+#endif /* !CONFIG_USER_ONLY */
-- 
2.38.1

Re: [PATCH 1/9] target/arm: Restrict v7-M MMU helpers to sysemu TCG

[Richard Henderson]

On 2/6/23 02:17, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/helper.c   | 2 +-
>   target/arm/m_helper.c | 4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)

This seems like it would break linux-user with -cpu cortex-m3, which is used for gcc 
testing, iirc.

(Real m-profile can't run linux, but using the same thumb abi makes testing easier than 
using semihosting and initializing a board model.)

OTOH, we should be able to hard-code the mmuidx for user-only:

     mprofile ? ARMMMUIdx_MUser : ARMMMUIdx_E10_0


r~

[PATCH 2/9] target/arm: Constify ID_PFR1 on user emulation

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/helper.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 5dbeade787..b58800a1a5 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7021,6 +7021,7 @@ static void define_pmu_regs(ARMCPU *cpu)
     }
 }
 
+#ifndef CONFIG_USER_ONLY
 /*
  * We don't know until after realize whether there's a GICv3
  * attached, and that is what registers the gicv3 sysregs.
@@ -7038,7 +7039,6 @@ static uint64_t id_pfr1_read(CPUARMState *env, const ARMCPRegInfo *ri)
     return pfr1;
 }
 
-#ifndef CONFIG_USER_ONLY
 static uint64_t id_aa64pfr0_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
     ARMCPU *cpu = env_archcpu(env);
@@ -7998,8 +7998,16 @@ void register_cp_regs_for_features(ARMCPU *cpu)
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 1,
               .access = PL1_R, .type = ARM_CP_NO_RAW,
               .accessfn = access_aa32_tid3,
+#ifdef CONFIG_USER_ONLY
+              .type = ARM_CP_CONST,
+              .resetvalue = cpu->isar.id_pfr1,
+#else
+              .type = ARM_CP_NO_RAW,
+              .accessfn = access_aa32_tid3,
               .readfn = id_pfr1_read,
-              .writefn = arm_cp_write_ignore },
+              .writefn = arm_cp_write_ignore
+#endif
+            },
             { .name = "ID_DFR0", .state = ARM_CP_STATE_BOTH,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 2,
               .access = PL1_R, .type = ARM_CP_CONST,
-- 
2.38.1

Re: [PATCH 2/9] target/arm: Constify ID_PFR1 on user emulation

[Richard Henderson]

On 2/6/23 02:17, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/helper.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 5dbeade787..b58800a1a5 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -7021,6 +7021,7 @@ static void define_pmu_regs(ARMCPU *cpu)
>       }
>   }
>   
> +#ifndef CONFIG_USER_ONLY
>   /*
>    * We don't know until after realize whether there's a GICv3
>    * attached, and that is what registers the gicv3 sysregs.
> @@ -7038,7 +7039,6 @@ static uint64_t id_pfr1_read(CPUARMState *env, const ARMCPRegInfo *ri)
>       return pfr1;
>   }
>   
> -#ifndef CONFIG_USER_ONLY
>   static uint64_t id_aa64pfr0_read(CPUARMState *env, const ARMCPRegInfo *ri)
>   {
>       ARMCPU *cpu = env_archcpu(env);
> @@ -7998,8 +7998,16 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>                 .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 1,
>                 .access = PL1_R, .type = ARM_CP_NO_RAW,
>                 .accessfn = access_aa32_tid3,
> +#ifdef CONFIG_USER_ONLY
> +              .type = ARM_CP_CONST,
> +              .resetvalue = cpu->isar.id_pfr1,
> +#else
> +              .type = ARM_CP_NO_RAW,
> +              .accessfn = access_aa32_tid3,
>                 .readfn = id_pfr1_read,
> -              .writefn = arm_cp_write_ignore },
> +              .writefn = arm_cp_write_ignore
> +#endif
> +            },
>               { .name = "ID_DFR0", .state = ARM_CP_STATE_BOTH,
>                 .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 2,
>                 .access = PL1_R, .type = ARM_CP_CONST,

[PATCH 3/9] target/arm: Avoid resetting CPUARMState::eabi field

[Philippe Mathieu-Daudé]

Although the 'eabi' field is only used in user emulation where
CPU reset doesn't occur, it doesn't belong to the area to reset.
Move it after the 'end_reset_fields' for consistency.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/cpu.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 7bc97fece9..bbbcf2e153 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -721,11 +721,6 @@ typedef struct CPUArchState {
     ARMVectorReg zarray[ARM_MAX_VQ * 16];
 #endif
 
-#if defined(CONFIG_USER_ONLY)
-    /* For usermode syscall translation.  */
-    int eabi;
-#endif
-
     struct CPUBreakpoint *cpu_breakpoint[16];
     struct CPUWatchpoint *cpu_watchpoint[16];
 
@@ -772,6 +767,10 @@ typedef struct CPUArchState {
         uint32_t ctrl;
     } sau;
 
+#if defined(CONFIG_USER_ONLY)
+    /* For usermode syscall translation.  */
+    int eabi;
+#endif
     void *nvic;
     const struct arm_boot_info *boot_info;
     /* Store GICv3CPUState to access from this struct */
-- 
2.38.1

Re: [PATCH 3/9] target/arm: Avoid resetting CPUARMState::eabi field

[Richard Henderson]

On 2/6/23 02:17, Philippe Mathieu-Daudé wrote:
> Although the 'eabi' field is only used in user emulation where
> CPU reset doesn't occur, it doesn't belong to the area to reset.
> Move it after the 'end_reset_fields' for consistency.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/cpu.h | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 7bc97fece9..bbbcf2e153 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -721,11 +721,6 @@ typedef struct CPUArchState {
>       ARMVectorReg zarray[ARM_MAX_VQ * 16];
>   #endif
>   
> -#if defined(CONFIG_USER_ONLY)
> -    /* For usermode syscall translation.  */
> -    int eabi;
> -#endif
> -
>       struct CPUBreakpoint *cpu_breakpoint[16];
>       struct CPUWatchpoint *cpu_watchpoint[16];
>   
> @@ -772,6 +767,10 @@ typedef struct CPUArchState {
>           uint32_t ctrl;
>       } sau;
>   
> +#if defined(CONFIG_USER_ONLY)
> +    /* For usermode syscall translation.  */
> +    int eabi;
> +#endif

As a follow-up, this could be bool.  And thus this might pack better just before 
tagged_addr_enable.

Other than placement,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 4/9] target/arm: Restrict CPUARMState::arm_boot_info to sysemu

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/cpu.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index bbbcf2e153..01d478e9ce 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -770,9 +770,10 @@ typedef struct CPUArchState {
 #if defined(CONFIG_USER_ONLY)
     /* For usermode syscall translation.  */
     int eabi;
+#else
+    const struct arm_boot_info *boot_info;
 #endif
     void *nvic;
-    const struct arm_boot_info *boot_info;
     /* Store GICv3CPUState to access from this struct */
     void *gicv3state;
 
-- 
2.38.1

Re: [PATCH 4/9] target/arm: Restrict CPUARMState::arm_boot_info to sysemu

[Richard Henderson]

On 2/6/23 02:17, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/cpu.h | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index bbbcf2e153..01d478e9ce 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -770,9 +770,10 @@ typedef struct CPUArchState {
>   #if defined(CONFIG_USER_ONLY)
>       /* For usermode syscall translation.  */
>       int eabi;
> +#else
> +    const struct arm_boot_info *boot_info;
>   #endif
>       void *nvic;
> -    const struct arm_boot_info *boot_info;
>       /* Store GICv3CPUState to access from this struct */
>       void *gicv3state;
>   

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 5/9] target/arm: Restrict CPUARMState::gicv3state to sysemu

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/cpu.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 01d478e9ce..61681101a5 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -772,10 +772,10 @@ typedef struct CPUArchState {
     int eabi;
 #else
     const struct arm_boot_info *boot_info;
-#endif
-    void *nvic;
     /* Store GICv3CPUState to access from this struct */
     void *gicv3state;
+#endif
+    void *nvic;
 
 #ifdef TARGET_TAGGED_ADDRESSES
     /* Linux syscall tagged address support */
-- 
2.38.1

Re: [PATCH 5/9] target/arm: Restrict CPUARMState::gicv3state to sysemu

[Richard Henderson]

On 2/6/23 02:17, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/arm/cpu.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 01d478e9ce..61681101a5 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -772,10 +772,10 @@ typedef struct CPUArchState {
>       int eabi;
>   #else
>       const struct arm_boot_info *boot_info;
> -#endif
> -    void *nvic;
>       /* Store GICv3CPUState to access from this struct */
>       void *gicv3state;
> +#endif
> +    void *nvic;
>   
>   #ifdef TARGET_TAGGED_ADDRESSES
>       /* Linux syscall tagged address support */

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 6/9] target/arm: Restrict CPUARMState::nvic to sysemu and store as NVICState*

[Philippe Mathieu-Daudé]

There is no point in using a void pointer to access the NVIC.
Use the real type to avoid casting it while debugging.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/intc/armv7m_nvic.c         | 38 ++++++++++-------------------
 include/hw/intc/armv7m_nvic.h |  5 +---
 target/arm/cpu.c              |  1 +
 target/arm/cpu.h              | 46 ++++++++++++++++++-----------------
 target/arm/m_helper.c         |  2 +-
 5 files changed, 40 insertions(+), 52 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 1f7763964c..e54553283f 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -389,7 +389,7 @@ static inline int nvic_exec_prio(NVICState *s)
     return MIN(running, s->exception_prio);
 }
 
-bool armv7m_nvic_neg_prio_requested(void *opaque, bool secure)
+bool armv7m_nvic_neg_prio_requested(NVICState *s, bool secure)
 {
     /* Return true if the requested execution priority is negative
      * for the specified security state, ie that security state
@@ -399,8 +399,6 @@ bool armv7m_nvic_neg_prio_requested(void *opaque, bool secure)
      * mean we don't allow FAULTMASK_NS to actually make the execution
      * priority negative). Compare pseudocode IsReqExcPriNeg().
      */
-    NVICState *s = opaque;
-
     if (s->cpu->env.v7m.faultmask[secure]) {
         return true;
     }
@@ -418,17 +416,13 @@ bool armv7m_nvic_neg_prio_requested(void *opaque, bool secure)
     return false;
 }
 
-bool armv7m_nvic_can_take_pending_exception(void *opaque)
+bool armv7m_nvic_can_take_pending_exception(NVICState *s)
 {
-    NVICState *s = opaque;
-
     return nvic_exec_prio(s) > nvic_pending_prio(s);
 }
 
-int armv7m_nvic_raw_execution_priority(void *opaque)
+int armv7m_nvic_raw_execution_priority(NVICState *s)
 {
-    NVICState *s = opaque;
-
     return s->exception_prio;
 }
 
@@ -506,9 +500,8 @@ static void nvic_irq_update(NVICState *s)
  * if @secure is true and @irq does not specify one of the fixed set
  * of architecturally banked exceptions.
  */
-static void armv7m_nvic_clear_pending(void *opaque, int irq, bool secure)
+static void armv7m_nvic_clear_pending(NVICState *s, int irq, bool secure)
 {
-    NVICState *s = (NVICState *)opaque;
     VecInfo *vec;
 
     assert(irq > ARMV7M_EXCP_RESET && irq < s->num_irq);
@@ -666,17 +659,17 @@ static void do_armv7m_nvic_set_pending(void *opaque, int irq, bool secure,
     }
 }
 
-void armv7m_nvic_set_pending(void *opaque, int irq, bool secure)
+void armv7m_nvic_set_pending(NVICState *s, int irq, bool secure)
 {
-    do_armv7m_nvic_set_pending(opaque, irq, secure, false);
+    do_armv7m_nvic_set_pending(s, irq, secure, false);
 }
 
-void armv7m_nvic_set_pending_derived(void *opaque, int irq, bool secure)
+void armv7m_nvic_set_pending_derived(NVICState *s, int irq, bool secure)
 {
-    do_armv7m_nvic_set_pending(opaque, irq, secure, true);
+    do_armv7m_nvic_set_pending(s, irq, secure, true);
 }
 
-void armv7m_nvic_set_pending_lazyfp(void *opaque, int irq, bool secure)
+void armv7m_nvic_set_pending_lazyfp(NVICState *s, int irq, bool secure)
 {
     /*
      * Pend an exception during lazy FP stacking. This differs
@@ -684,7 +677,6 @@ void armv7m_nvic_set_pending_lazyfp(void *opaque, int irq, bool secure)
      * whether we should escalate depends on the saved context
      * in the FPCCR register, not on the current state of the CPU/NVIC.
      */
-    NVICState *s = (NVICState *)opaque;
     bool banked = exc_is_banked(irq);
     VecInfo *vec;
     bool targets_secure;
@@ -773,9 +765,8 @@ void armv7m_nvic_set_pending_lazyfp(void *opaque, int irq, bool secure)
 }
 
 /* Make pending IRQ active.  */
-void armv7m_nvic_acknowledge_irq(void *opaque)
+void armv7m_nvic_acknowledge_irq(NVICState *s)
 {
-    NVICState *s = (NVICState *)opaque;
     CPUARMState *env = &s->cpu->env;
     const int pending = s->vectpending;
     const int running = nvic_exec_prio(s);
@@ -814,10 +805,9 @@ static bool vectpending_targets_secure(NVICState *s)
         exc_targets_secure(s, s->vectpending);
 }
 
-void armv7m_nvic_get_pending_irq_info(void *opaque,
+void armv7m_nvic_get_pending_irq_info(NVICState *s,
                                       int *pirq, bool *ptargets_secure)
 {
-    NVICState *s = (NVICState *)opaque;
     const int pending = s->vectpending;
     bool targets_secure;
 
@@ -831,9 +821,8 @@ void armv7m_nvic_get_pending_irq_info(void *opaque,
     *pirq = pending;
 }
 
-int armv7m_nvic_complete_irq(void *opaque, int irq, bool secure)
+int armv7m_nvic_complete_irq(NVICState *s, int irq, bool secure)
 {
-    NVICState *s = (NVICState *)opaque;
     VecInfo *vec = NULL;
     int ret = 0;
 
@@ -915,7 +904,7 @@ int armv7m_nvic_complete_irq(void *opaque, int irq, bool secure)
     return ret;
 }
 
-bool armv7m_nvic_get_ready_status(void *opaque, int irq, bool secure)
+bool armv7m_nvic_get_ready_status(NVICState *s, int irq, bool secure)
 {
     /*
      * Return whether an exception is "ready", i.e. it is enabled and is
@@ -926,7 +915,6 @@ bool armv7m_nvic_get_ready_status(void *opaque, int irq, bool secure)
      * for non-banked exceptions secure is always false; for banked exceptions
      * it indicates which of the exceptions is required.
      */
-    NVICState *s = (NVICState *)opaque;
     bool banked = exc_is_banked(irq);
     VecInfo *vec;
     int running = nvic_exec_prio(s);
diff --git a/include/hw/intc/armv7m_nvic.h b/include/hw/intc/armv7m_nvic.h
index 0180c7b0ca..07f9c21a5f 100644
--- a/include/hw/intc/armv7m_nvic.h
+++ b/include/hw/intc/armv7m_nvic.h
@@ -16,10 +16,7 @@
 #include "qom/object.h"
 
 #define TYPE_NVIC "armv7m_nvic"
-
-typedef struct NVICState NVICState;
-DECLARE_INSTANCE_CHECKER(NVICState, NVIC,
-                         TYPE_NVIC)
+OBJECT_DECLARE_SIMPLE_TYPE(NVICState, NVIC)
 
 /* Highest permitted number of exceptions (architectural limit) */
 #define NVIC_MAX_VECTORS 512
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 5f63316dbf..b3a2275b08 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -36,6 +36,7 @@
 #if !defined(CONFIG_USER_ONLY)
 #include "hw/loader.h"
 #include "hw/boards.h"
+#include "hw/intc/armv7m_nvic.h"
 #endif
 #include "sysemu/tcg.h"
 #include "sysemu/qtest.h"
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 61681101a5..683e186599 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -227,6 +227,8 @@ typedef struct CPUARMTBFlags {
 
 typedef struct ARMMMUFaultInfo ARMMMUFaultInfo;
 
+typedef struct NVICState NVICState;
+
 typedef struct CPUArchState {
     /* Regs for current mode.  */
     uint32_t regs[16];
@@ -774,8 +776,8 @@ typedef struct CPUArchState {
     const struct arm_boot_info *boot_info;
     /* Store GICv3CPUState to access from this struct */
     void *gicv3state;
+    NVICState *nvic;
 #endif
-    void *nvic;
 
 #ifdef TARGET_TAGGED_ADDRESSES
     /* Linux syscall tagged address support */
@@ -2559,16 +2561,16 @@ uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
 
 /* Interface between CPU and Interrupt controller.  */
 #ifndef CONFIG_USER_ONLY
-bool armv7m_nvic_can_take_pending_exception(void *opaque);
+bool armv7m_nvic_can_take_pending_exception(NVICState *s);
 #else
-static inline bool armv7m_nvic_can_take_pending_exception(void *opaque)
+static inline bool armv7m_nvic_can_take_pending_exception(NVICState *s)
 {
     return true;
 }
 #endif
 /**
  * armv7m_nvic_set_pending: mark the specified exception as pending
- * @opaque: the NVIC
+ * @s: the NVIC
  * @irq: the exception number to mark pending
  * @secure: false for non-banked exceptions or for the nonsecure
  * version of a banked exception, true for the secure version of a banked
@@ -2578,10 +2580,10 @@ static inline bool armv7m_nvic_can_take_pending_exception(void *opaque)
  * if @secure is true and @irq does not specify one of the fixed set
  * of architecturally banked exceptions.
  */
-void armv7m_nvic_set_pending(void *opaque, int irq, bool secure);
+void armv7m_nvic_set_pending(NVICState *s, int irq, bool secure);
 /**
  * armv7m_nvic_set_pending_derived: mark this derived exception as pending
- * @opaque: the NVIC
+ * @s: the NVIC
  * @irq: the exception number to mark pending
  * @secure: false for non-banked exceptions or for the nonsecure
  * version of a banked exception, true for the secure version of a banked
@@ -2591,10 +2593,10 @@ void armv7m_nvic_set_pending(void *opaque, int irq, bool secure);
  * exceptions (exceptions generated in the course of trying to take
  * a different exception).
  */
-void armv7m_nvic_set_pending_derived(void *opaque, int irq, bool secure);
+void armv7m_nvic_set_pending_derived(NVICState *s, int irq, bool secure);
 /**
  * armv7m_nvic_set_pending_lazyfp: mark this lazy FP exception as pending
- * @opaque: the NVIC
+ * @s: the NVIC
  * @irq: the exception number to mark pending
  * @secure: false for non-banked exceptions or for the nonsecure
  * version of a banked exception, true for the secure version of a banked
@@ -2603,11 +2605,11 @@ void armv7m_nvic_set_pending_derived(void *opaque, int irq, bool secure);
  * Similar to armv7m_nvic_set_pending(), but specifically for exceptions
  * generated in the course of lazy stacking of FP registers.
  */
-void armv7m_nvic_set_pending_lazyfp(void *opaque, int irq, bool secure);
+void armv7m_nvic_set_pending_lazyfp(NVICState *s, int irq, bool secure);
 /**
  * armv7m_nvic_get_pending_irq_info: return highest priority pending
  *    exception, and whether it targets Secure state
- * @opaque: the NVIC
+ * @s: the NVIC
  * @pirq: set to pending exception number
  * @ptargets_secure: set to whether pending exception targets Secure
  *
@@ -2617,20 +2619,20 @@ void armv7m_nvic_set_pending_lazyfp(void *opaque, int irq, bool secure);
  * to true if the current highest priority pending exception should
  * be taken to Secure state, false for NS.
  */
-void armv7m_nvic_get_pending_irq_info(void *opaque, int *pirq,
+void armv7m_nvic_get_pending_irq_info(NVICState *s, int *pirq,
                                       bool *ptargets_secure);
 /**
  * armv7m_nvic_acknowledge_irq: make highest priority pending exception active
- * @opaque: the NVIC
+ * @s: the NVIC
  *
  * Move the current highest priority pending exception from the pending
  * state to the active state, and update v7m.exception to indicate that
  * it is the exception currently being handled.
  */
-void armv7m_nvic_acknowledge_irq(void *opaque);
+void armv7m_nvic_acknowledge_irq(NVICState *s);
 /**
  * armv7m_nvic_complete_irq: complete specified interrupt or exception
- * @opaque: the NVIC
+ * @s: the NVIC
  * @irq: the exception number to complete
  * @secure: true if this exception was secure
  *
@@ -2639,10 +2641,10 @@ void armv7m_nvic_acknowledge_irq(void *opaque);
  *           0 if there is still an irq active after this one was completed
  * (Ignoring -1, this is the same as the RETTOBASE value before completion.)
  */
-int armv7m_nvic_complete_irq(void *opaque, int irq, bool secure);
+int armv7m_nvic_complete_irq(NVICState *s, int irq, bool secure);
 /**
  * armv7m_nvic_get_ready_status(void *opaque, int irq, bool secure)
- * @opaque: the NVIC
+ * @s: the NVIC
  * @irq: the exception number to mark pending
  * @secure: false for non-banked exceptions or for the nonsecure
  * version of a banked exception, true for the secure version of a banked
@@ -2653,28 +2655,28 @@ int armv7m_nvic_complete_irq(void *opaque, int irq, bool secure);
  * interrupt the current execution priority. This controls whether the
  * RDY bit for it in the FPCCR is set.
  */
-bool armv7m_nvic_get_ready_status(void *opaque, int irq, bool secure);
+bool armv7m_nvic_get_ready_status(NVICState *s, int irq, bool secure);
 /**
  * armv7m_nvic_raw_execution_priority: return the raw execution priority
- * @opaque: the NVIC
+ * @s: the NVIC
  *
  * Returns: the raw execution priority as defined by the v8M architecture.
  * This is the execution priority minus the effects of AIRCR.PRIS,
  * and minus any PRIMASK/FAULTMASK/BASEPRI priority boosting.
  * (v8M ARM ARM I_PKLD.)
  */
-int armv7m_nvic_raw_execution_priority(void *opaque);
+int armv7m_nvic_raw_execution_priority(NVICState *s);
 /**
  * armv7m_nvic_neg_prio_requested: return true if the requested execution
  * priority is negative for the specified security state.
- * @opaque: the NVIC
+ * @s: the NVIC
  * @secure: the security state to test
  * This corresponds to the pseudocode IsReqExecPriNeg().
  */
 #ifndef CONFIG_USER_ONLY
-bool armv7m_nvic_neg_prio_requested(void *opaque, bool secure);
+bool armv7m_nvic_neg_prio_requested(NVICState *s, bool secure);
 #else
-static inline bool armv7m_nvic_neg_prio_requested(void *opaque, bool secure)
+static inline bool armv7m_nvic_neg_prio_requested(NVICState *s, bool secure)
 {
     return false;
 }
diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index 1e7e4e33bd..f73d3f2264 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -973,7 +973,7 @@ static void v7m_update_fpccr(CPUARMState *env, uint32_t frameptr,
      * that we will need later in order to do lazy FP reg stacking.
      */
     bool is_secure = env->v7m.secure;
-    void *nvic = env->nvic;
+    NVICState *nvic = env->nvic;
     /*
      * Some bits are unbanked and live always in fpccr[M_REG_S]; some bits
      * are banked and we want to update the bit in the bank for the
-- 
2.38.1

Re: [PATCH 6/9] target/arm: Restrict CPUARMState::nvic to sysemu and store as NVICState*

[Richard Henderson]

On 2/6/23 02:17, Philippe Mathieu-Daudé wrote:
> There is no point in using a void pointer to access the NVIC.
> Use the real type to avoid casting it while debugging.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

This is doing several things at once.  The nvic interface change needn't be done 
simultaneously.


r~

Re: [PATCH 6/9] target/arm: Restrict CPUARMState::nvic to sysemu and store as NVICState*

[Philippe Mathieu-Daudé]

On 6/2/23 19:57, Richard Henderson wrote:
> On 2/6/23 02:17, Philippe Mathieu-Daudé wrote:
>> There is no point in using a void pointer to access the NVIC.
>> Use the real type to avoid casting it while debugging.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
> 
> This is doing several things at once.  The nvic interface change needn't 
> be done simultaneously.

You mean this change?

-typedef struct NVICState NVICState;
-DECLARE_INSTANCE_CHECKER(NVICState, NVIC,
-                         TYPE_NVIC)
+OBJECT_DECLARE_SIMPLE_TYPE(NVICState, NVIC)

This is a No-OP, converting from the older DECLARE_INSTANCE_CHECKER
style to the newer OBJECT_DECLARE_SIMPLE_TYPE. But OK, unrelated, I'll
remove it from the patch.

Re: [PATCH 6/9] target/arm: Restrict CPUARMState::nvic to sysemu and store as NVICState*

[Richard Henderson]

On 2/6/23 09:00, Philippe Mathieu-Daudé wrote:
> On 6/2/23 19:57, Richard Henderson wrote:
>> On 2/6/23 02:17, Philippe Mathieu-Daudé wrote:
>>> There is no point in using a void pointer to access the NVIC.
>>> Use the real type to avoid casting it while debugging.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>
>> This is doing several things at once.  The nvic interface change needn't be done 
>> simultaneously.
> 
> You mean this change?
> 
> -typedef struct NVICState NVICState;
> -DECLARE_INSTANCE_CHECKER(NVICState, NVIC,
> -                         TYPE_NVIC)
> +OBJECT_DECLARE_SIMPLE_TYPE(NVICState, NVIC)
> 
> This is a No-OP, converting from the older DECLARE_INSTANCE_CHECKER
> style to the newer OBJECT_DECLARE_SIMPLE_TYPE. But OK, unrelated, I'll
> remove it from the patch.

That and the movement of cpu->nvic into the ifdef.


r~

[PATCH 7/9] target/arm: Declare CPU <-> NVIC helpers in 'hw/intc/armv7m_nvic.h'

[Philippe Mathieu-Daudé]

While dozens of files include "cpu.h", only 3 files require
these NVIC helper declarations.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/intc/armv7m_nvic.h | 123 ++++++++++++++++++++++++++++++++++
 target/arm/cpu.c              |   4 +-
 target/arm/cpu.h              | 123 ----------------------------------
 target/arm/cpu_tcg.c          |   3 +
 target/arm/m_helper.c         |   3 +
 5 files changed, 132 insertions(+), 124 deletions(-)

diff --git a/include/hw/intc/armv7m_nvic.h b/include/hw/intc/armv7m_nvic.h
index 07f9c21a5f..1ca262fbf8 100644
--- a/include/hw/intc/armv7m_nvic.h
+++ b/include/hw/intc/armv7m_nvic.h
@@ -83,4 +83,127 @@ struct NVICState {
     qemu_irq sysresetreq;
 };
 
+/* Interface between CPU and Interrupt controller.  */
+/**
+ * armv7m_nvic_set_pending: mark the specified exception as pending
+ * @s: the NVIC
+ * @irq: the exception number to mark pending
+ * @secure: false for non-banked exceptions or for the nonsecure
+ * version of a banked exception, true for the secure version of a banked
+ * exception.
+ *
+ * Marks the specified exception as pending. Note that we will assert()
+ * if @secure is true and @irq does not specify one of the fixed set
+ * of architecturally banked exceptions.
+ */
+void armv7m_nvic_set_pending(NVICState *s, int irq, bool secure);
+/**
+ * armv7m_nvic_set_pending_derived: mark this derived exception as pending
+ * @s: the NVIC
+ * @irq: the exception number to mark pending
+ * @secure: false for non-banked exceptions or for the nonsecure
+ * version of a banked exception, true for the secure version of a banked
+ * exception.
+ *
+ * Similar to armv7m_nvic_set_pending(), but specifically for derived
+ * exceptions (exceptions generated in the course of trying to take
+ * a different exception).
+ */
+void armv7m_nvic_set_pending_derived(NVICState *s, int irq, bool secure);
+/**
+ * armv7m_nvic_set_pending_lazyfp: mark this lazy FP exception as pending
+ * @s: the NVIC
+ * @irq: the exception number to mark pending
+ * @secure: false for non-banked exceptions or for the nonsecure
+ * version of a banked exception, true for the secure version of a banked
+ * exception.
+ *
+ * Similar to armv7m_nvic_set_pending(), but specifically for exceptions
+ * generated in the course of lazy stacking of FP registers.
+ */
+void armv7m_nvic_set_pending_lazyfp(NVICState *s, int irq, bool secure);
+/**
+ * armv7m_nvic_get_pending_irq_info: return highest priority pending
+ *    exception, and whether it targets Secure state
+ * @s: the NVIC
+ * @pirq: set to pending exception number
+ * @ptargets_secure: set to whether pending exception targets Secure
+ *
+ * This function writes the number of the highest priority pending
+ * exception (the one which would be made active by
+ * armv7m_nvic_acknowledge_irq()) to @pirq, and sets @ptargets_secure
+ * to true if the current highest priority pending exception should
+ * be taken to Secure state, false for NS.
+ */
+void armv7m_nvic_get_pending_irq_info(NVICState *s, int *pirq,
+                                      bool *ptargets_secure);
+/**
+ * armv7m_nvic_acknowledge_irq: make highest priority pending exception active
+ * @s: the NVIC
+ *
+ * Move the current highest priority pending exception from the pending
+ * state to the active state, and update v7m.exception to indicate that
+ * it is the exception currently being handled.
+ */
+void armv7m_nvic_acknowledge_irq(NVICState *s);
+/**
+ * armv7m_nvic_complete_irq: complete specified interrupt or exception
+ * @s: the NVIC
+ * @irq: the exception number to complete
+ * @secure: true if this exception was secure
+ *
+ * Returns: -1 if the irq was not active
+ *           1 if completing this irq brought us back to base (no active irqs)
+ *           0 if there is still an irq active after this one was completed
+ * (Ignoring -1, this is the same as the RETTOBASE value before completion.)
+ */
+int armv7m_nvic_complete_irq(NVICState *s, int irq, bool secure);
+/**
+ * armv7m_nvic_get_ready_status(void *opaque, int irq, bool secure)
+ * @s: the NVIC
+ * @irq: the exception number to mark pending
+ * @secure: false for non-banked exceptions or for the nonsecure
+ * version of a banked exception, true for the secure version of a banked
+ * exception.
+ *
+ * Return whether an exception is "ready", i.e. whether the exception is
+ * enabled and is configured at a priority which would allow it to
+ * interrupt the current execution priority. This controls whether the
+ * RDY bit for it in the FPCCR is set.
+ */
+bool armv7m_nvic_get_ready_status(NVICState *s, int irq, bool secure);
+/**
+ * armv7m_nvic_raw_execution_priority: return the raw execution priority
+ * @s: the NVIC
+ *
+ * Returns: the raw execution priority as defined by the v8M architecture.
+ * This is the execution priority minus the effects of AIRCR.PRIS,
+ * and minus any PRIMASK/FAULTMASK/BASEPRI priority boosting.
+ * (v8M ARM ARM I_PKLD.)
+ */
+int armv7m_nvic_raw_execution_priority(NVICState *s);
+/**
+ * armv7m_nvic_neg_prio_requested: return true if the requested execution
+ * priority is negative for the specified security state.
+ * @s: the NVIC
+ * @secure: the security state to test
+ * This corresponds to the pseudocode IsReqExecPriNeg().
+ */
+#ifndef CONFIG_USER_ONLY
+bool armv7m_nvic_neg_prio_requested(NVICState *s, bool secure);
+#else
+static inline bool armv7m_nvic_neg_prio_requested(NVICState *s, bool secure)
+{
+    return false;
+}
+#endif
+#ifndef CONFIG_USER_ONLY
+bool armv7m_nvic_can_take_pending_exception(NVICState *s);
+#else
+static inline bool armv7m_nvic_can_take_pending_exception(NVICState *s)
+{
+    return true;
+}
+#endif
+
 #endif
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index b3a2275b08..876ab8f3bf 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -36,8 +36,10 @@
 #if !defined(CONFIG_USER_ONLY)
 #include "hw/loader.h"
 #include "hw/boards.h"
+#ifdef CONFIG_TCG
 #include "hw/intc/armv7m_nvic.h"
-#endif
+#endif /* CONFIG_TCG */
+#endif /* !CONFIG_USER_ONLY */
 #include "sysemu/tcg.h"
 #include "sysemu/qtest.h"
 #include "sysemu/hw_accel.h"
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 683e186599..a6543c2153 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2559,129 +2559,6 @@ void arm_cpu_list(void);
 uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
                                  uint32_t cur_el, bool secure);
 
-/* Interface between CPU and Interrupt controller.  */
-#ifndef CONFIG_USER_ONLY
-bool armv7m_nvic_can_take_pending_exception(NVICState *s);
-#else
-static inline bool armv7m_nvic_can_take_pending_exception(NVICState *s)
-{
-    return true;
-}
-#endif
-/**
- * armv7m_nvic_set_pending: mark the specified exception as pending
- * @s: the NVIC
- * @irq: the exception number to mark pending
- * @secure: false for non-banked exceptions or for the nonsecure
- * version of a banked exception, true for the secure version of a banked
- * exception.
- *
- * Marks the specified exception as pending. Note that we will assert()
- * if @secure is true and @irq does not specify one of the fixed set
- * of architecturally banked exceptions.
- */
-void armv7m_nvic_set_pending(NVICState *s, int irq, bool secure);
-/**
- * armv7m_nvic_set_pending_derived: mark this derived exception as pending
- * @s: the NVIC
- * @irq: the exception number to mark pending
- * @secure: false for non-banked exceptions or for the nonsecure
- * version of a banked exception, true for the secure version of a banked
- * exception.
- *
- * Similar to armv7m_nvic_set_pending(), but specifically for derived
- * exceptions (exceptions generated in the course of trying to take
- * a different exception).
- */
-void armv7m_nvic_set_pending_derived(NVICState *s, int irq, bool secure);
-/**
- * armv7m_nvic_set_pending_lazyfp: mark this lazy FP exception as pending
- * @s: the NVIC
- * @irq: the exception number to mark pending
- * @secure: false for non-banked exceptions or for the nonsecure
- * version of a banked exception, true for the secure version of a banked
- * exception.
- *
- * Similar to armv7m_nvic_set_pending(), but specifically for exceptions
- * generated in the course of lazy stacking of FP registers.
- */
-void armv7m_nvic_set_pending_lazyfp(NVICState *s, int irq, bool secure);
-/**
- * armv7m_nvic_get_pending_irq_info: return highest priority pending
- *    exception, and whether it targets Secure state
- * @s: the NVIC
- * @pirq: set to pending exception number
- * @ptargets_secure: set to whether pending exception targets Secure
- *
- * This function writes the number of the highest priority pending
- * exception (the one which would be made active by
- * armv7m_nvic_acknowledge_irq()) to @pirq, and sets @ptargets_secure
- * to true if the current highest priority pending exception should
- * be taken to Secure state, false for NS.
- */
-void armv7m_nvic_get_pending_irq_info(NVICState *s, int *pirq,
-                                      bool *ptargets_secure);
-/**
- * armv7m_nvic_acknowledge_irq: make highest priority pending exception active
- * @s: the NVIC
- *
- * Move the current highest priority pending exception from the pending
- * state to the active state, and update v7m.exception to indicate that
- * it is the exception currently being handled.
- */
-void armv7m_nvic_acknowledge_irq(NVICState *s);
-/**
- * armv7m_nvic_complete_irq: complete specified interrupt or exception
- * @s: the NVIC
- * @irq: the exception number to complete
- * @secure: true if this exception was secure
- *
- * Returns: -1 if the irq was not active
- *           1 if completing this irq brought us back to base (no active irqs)
- *           0 if there is still an irq active after this one was completed
- * (Ignoring -1, this is the same as the RETTOBASE value before completion.)
- */
-int armv7m_nvic_complete_irq(NVICState *s, int irq, bool secure);
-/**
- * armv7m_nvic_get_ready_status(void *opaque, int irq, bool secure)
- * @s: the NVIC
- * @irq: the exception number to mark pending
- * @secure: false for non-banked exceptions or for the nonsecure
- * version of a banked exception, true for the secure version of a banked
- * exception.
- *
- * Return whether an exception is "ready", i.e. whether the exception is
- * enabled and is configured at a priority which would allow it to
- * interrupt the current execution priority. This controls whether the
- * RDY bit for it in the FPCCR is set.
- */
-bool armv7m_nvic_get_ready_status(NVICState *s, int irq, bool secure);
-/**
- * armv7m_nvic_raw_execution_priority: return the raw execution priority
- * @s: the NVIC
- *
- * Returns: the raw execution priority as defined by the v8M architecture.
- * This is the execution priority minus the effects of AIRCR.PRIS,
- * and minus any PRIMASK/FAULTMASK/BASEPRI priority boosting.
- * (v8M ARM ARM I_PKLD.)
- */
-int armv7m_nvic_raw_execution_priority(NVICState *s);
-/**
- * armv7m_nvic_neg_prio_requested: return true if the requested execution
- * priority is negative for the specified security state.
- * @s: the NVIC
- * @secure: the security state to test
- * This corresponds to the pseudocode IsReqExecPriNeg().
- */
-#ifndef CONFIG_USER_ONLY
-bool armv7m_nvic_neg_prio_requested(NVICState *s, bool secure);
-#else
-static inline bool armv7m_nvic_neg_prio_requested(NVICState *s, bool secure)
-{
-    return false;
-}
-#endif
-
 /* Interface for defining coprocessor registers.
  * Registers are defined in tables of arm_cp_reginfo structs
  * which are passed to define_arm_cp_regs().
diff --git a/target/arm/cpu_tcg.c b/target/arm/cpu_tcg.c
index ccde5080eb..df0c45e523 100644
--- a/target/arm/cpu_tcg.c
+++ b/target/arm/cpu_tcg.c
@@ -19,6 +19,9 @@
 #include "hw/boards.h"
 #endif
 #include "cpregs.h"
+#if !defined(CONFIG_USER_ONLY) && defined(CONFIG_TCG)
+#include "hw/intc/armv7m_nvic.h"
+#endif
 
 
 /* Share AArch32 -cpu max features with AArch64. */
diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index f73d3f2264..4dcb594a6b 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -18,6 +18,9 @@
 #include "exec/cpu_ldst.h"
 #include "semihosting/common-semi.h"
 #endif
+#if !defined(CONFIG_USER_ONLY)
+#include "hw/intc/armv7m_nvic.h"
+#endif
 
 static void v7m_msr_xpsr(CPUARMState *env, uint32_t mask,
                          uint32_t reg, uint32_t val)
-- 
2.38.1

Re: [PATCH 7/9] target/arm: Declare CPU <-> NVIC helpers in 'hw/intc/armv7m_nvic.h'

[Richard Henderson]

On 2/6/23 02:17, Philippe Mathieu-Daudé wrote:
> While dozens of files include "cpu.h", only 3 files require
> these NVIC helper declarations.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   include/hw/intc/armv7m_nvic.h | 123 ++++++++++++++++++++++++++++++++++
>   target/arm/cpu.c              |   4 +-
>   target/arm/cpu.h              | 123 ----------------------------------
>   target/arm/cpu_tcg.c          |   3 +
>   target/arm/m_helper.c         |   3 +
>   5 files changed, 132 insertions(+), 124 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 8/9] hw/intc/armv7m_nvic: Allow calling neg_prio_requested on unrealized NVIC

[Philippe Mathieu-Daudé]

armv7m_nvic_neg_prio_requested() is called via arm_cpu_reset_hold()
during CPU realize() time, when the NVIC isn't yet realized:

(lldb) bt
  * frame #0:  0x10059ed5c armv7m_nvic_neg_prio_requested(opaque=0x1180087b0, secure=true) at armv7m_nvic.c:404:9
    frame #1:  0x100383018 arm_v7m_mmu_idx_for_secstate [inlined] arm_v7m_mmu_idx_for_secstate_and_priv(...) at m_helper.c:2882:19
    frame #2:  0x10038300c arm_v7m_mmu_idx_for_secstate(..., secstate=true) at m_helper.c:2893:12
    frame #3:  0x10036e9bc arm_mmu_idx_el(...) at helper.c:11799:16 [artificial]
    frame #4:  0x100366cd4 arm_rebuild_hflags [inlined] rebuild_hflags_internal(env=0x118411f30) at helper.c:12129:25
    frame #5:  0x100366c18 arm_rebuild_hflags(env=0x118411f30) at helper.c:12142:19
    frame #6:  0x10035f1c4 arm_cpu_reset_hold(...) at cpu.c:541:5 [artificial]
    frame #7:  0x10066b354 resettable_phase_hold(obj=0x118410000, opaque=0x000000000, ...) at resettable.c:0
    frame #8:  0x10066ac40 resettable_assert_reset(obj=0x118410000, ...) at resettable.c:60:5
    frame #9:  0x10066ab1c resettable_reset(obj=0x118410000, type=RESET_TYPE_COLD) at resettable.c:45:5
    frame #10: 0x100669568 device_cold_reset(...) at qdev.c:255:5 [artificial]
    frame #11: 0x10000ca28 cpu_reset(cpu=0x118410000) at cpu-common.c:114:5
    frame #12: 0x10035ec74 arm_cpu_realizefn(dev=0x118410000, errp=0x16fdfb910) at cpu.c:2145:5
    frame #13: 0x10066a3e0 device_set_realized(...) at qdev.c:519:13
    frame #14: 0x100671b98 property_set_bool(obj=0x118410000, ...) at object.c:2285:5
    frame #15: 0x10066fdf4 object_property_set(obj=0x118410000, name="realized", ...) at object.c:1420:5
    frame #16: 0x100673da8 object_property_set_qobject(...) at qom-qobject.c:28:10
    frame #17: 0x10067026c object_property_set_bool(...) at object.c:1489:15
    frame #18: 0x100669600 qdev_realize(...) at qdev.c:292:12 [artificial]
    frame #19: 0x1003101bc armv7m_realize(dev=0x118008480, ...) at armv7m.c:344:10
    frame #20: 0x10066a3e0 device_set_realized(...) at qdev.c:519:13
    frame #21: 0x100671b98 property_set_bool(obj=0x118008480, ...) at object.c:2285:5
    frame #22: 0x10066fdf4 object_property_set(obj=0x118008480, name="realized", ...) at object.c:1420:5
    frame #23: 0x100673da8 object_property_set_qobject(...) at qom-qobject.c:28:10
    frame #24: 0x10067026c object_property_set_bool(...) at object.c:1489:15
    frame #25: 0x100669600 qdev_realize(...) at qdev.c:292:12 [artificial]
    frame #26: 0x100092da8 sysbus_realize(...) at sysbus.c:256:12 [artificial]
    frame #27: 0x100350e1c armsse_realize(dev=0x118008150, ...) at armsse.c:1043:14
    frame #28: 0x10066a3e0 device_set_realized(...) at qdev.c:519:13
    frame #29: 0x100671b98 property_set_bool(obj=0x118008150, ...) at object.c:2285:5
    frame #30: 0x10066fdf4 object_property_set(obj=0x118008150, name="realized", ...) at object.c:1420:5
    frame #31: 0x100673da8 object_property_set_qobject(...) at qom-qobject.c:28:10
    frame #32: 0x10067026c object_property_set_bool(...) at object.c:1489:15
    frame #33: 0x100669600 qdev_realize(...) at qdev.c:292:12 [artificial]
    frame #34: 0x100092da8 sysbus_realize(...) at sysbus.c:256:12 [artificial]
    frame #35: 0x100349354 mps2tz_common_init(machine=0x118008000) at mps2-tz.c:834:5
    frame #36: 0x10008e6b8 machine_run_board_init(machine=0x118008000, ...) at machine.c:1405:5
(lldb) frame select 12
frame #12: 0x10035ec74 arm_cpu_realizefn(dev=0x118410000, errp=0x16fdfb910) at cpu.c:2145:5
   2142	    }
   2143
   2144	    qemu_init_vcpu(cs);
-> 2145	    cpu_reset(cs);
   2146
   2147	    acc->parent_realize(dev, errp);
   2148	}

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/intc/armv7m_nvic.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index e54553283f..d9c7e414bc 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -399,6 +399,11 @@ bool armv7m_nvic_neg_prio_requested(NVICState *s, bool secure)
      * mean we don't allow FAULTMASK_NS to actually make the execution
      * priority negative). Compare pseudocode IsReqExcPriNeg().
      */
+
+    if (!DEVICE(s)->realized) { /* XXX Why are we called while not realized? */
+        return false;
+    }
+
     if (s->cpu->env.v7m.faultmask[secure]) {
         return true;
     }
-- 
2.38.1

[PATCH 9/9] hw/arm/armv7m: Pass CPU/NVIC using object_property_add_const_link()

[Philippe Mathieu-Daudé]

Avoid having QOM objects poke at each other internals.
Instead, pass references using QOM link properties.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/arm/armv7m.c       | 4 ++--
 hw/intc/armv7m_nvic.c | 3 ++-
 target/arm/cpu.c      | 3 ++-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index 50a9507c0b..edde774da2 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -338,8 +338,8 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
      * Tell the CPU where the NVIC is; it will fail realize if it doesn't
      * have one. Similarly, tell the NVIC where its CPU is.
      */
-    s->cpu->env.nvic = &s->nvic;
-    s->nvic.cpu = s->cpu;
+    object_property_add_const_link(OBJECT(s->cpu), "nvic", OBJECT(&s->nvic));
+    object_property_add_const_link(OBJECT(&s->nvic), "cpu", OBJECT(s->cpu));
 
     if (!qdev_realize(DEVICE(s->cpu), NULL, errp)) {
         return;
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index d9c7e414bc..e43898a9e0 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -2668,7 +2668,8 @@ static void armv7m_nvic_realize(DeviceState *dev, Error **errp)
     NVICState *s = NVIC(dev);
 
     /* The armv7m container object will have set our CPU pointer */
-    if (!s->cpu || !arm_feature(&s->cpu->env, ARM_FEATURE_M)) {
+    s->cpu = ARM_CPU(object_property_get_link(OBJECT(dev), "cpu", &error_abort));
+    if (!arm_feature(&s->cpu->env, ARM_FEATURE_M)) {
         error_setg(errp, "The NVIC can only be used with a Cortex-M CPU");
         return;
     }
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 876ab8f3bf..f081861947 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1573,12 +1573,13 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
      * error and will result in segfaults if not caught here.
      */
     if (arm_feature(env, ARM_FEATURE_M)) {
+        env->nvic = NVIC(object_property_get_link(OBJECT(dev), "nvic", NULL));
         if (!env->nvic) {
             error_setg(errp, "This board cannot be used with Cortex-M CPUs");
             return;
         }
     } else {
-        if (env->nvic) {
+        if (object_property_find(OBJECT(dev), "nvic")) {
             error_setg(errp, "This board can only be used with Cortex-M CPUs");
             return;
         }
-- 
2.38.1

Re: [PATCH 9/9] hw/arm/armv7m: Pass CPU/NVIC using object_property_add_const_link()

[Philippe Mathieu-Daudé]

On 6/2/23 13:17, Philippe Mathieu-Daudé wrote:
> Avoid having QOM objects poke at each other internals.
> Instead, pass references using QOM link properties.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/arm/armv7m.c       | 4 ++--
>   hw/intc/armv7m_nvic.c | 3 ++-
>   target/arm/cpu.c      | 3 ++-
>   3 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index 50a9507c0b..edde774da2 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -338,8 +338,8 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
>        * Tell the CPU where the NVIC is; it will fail realize if it doesn't
>        * have one. Similarly, tell the NVIC where its CPU is.
>        */
> -    s->cpu->env.nvic = &s->nvic;
> -    s->nvic.cpu = s->cpu;
> +    object_property_add_const_link(OBJECT(s->cpu), "nvic", OBJECT(&s->nvic));
> +    object_property_add_const_link(OBJECT(&s->nvic), "cpu", OBJECT(s->cpu));

Markus mentioned this converts build time types checks to runtime.
Also harder to debug. And the code becomes harder to read.

>       if (!qdev_realize(DEVICE(s->cpu), NULL, errp)) {
>           return;
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index d9c7e414bc..e43898a9e0 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -2668,7 +2668,8 @@ static void armv7m_nvic_realize(DeviceState *dev, Error **errp)
>       NVICState *s = NVIC(dev);
>   
>       /* The armv7m container object will have set our CPU pointer */
> -    if (!s->cpu || !arm_feature(&s->cpu->env, ARM_FEATURE_M)) {
> +    s->cpu = ARM_CPU(object_property_get_link(OBJECT(dev), "cpu", &error_abort));
> +    if (!arm_feature(&s->cpu->env, ARM_FEATURE_M)) {
>           error_setg(errp, "The NVIC can only be used with a Cortex-M CPU");
>           return;
>       }
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 876ab8f3bf..f081861947 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1573,12 +1573,13 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>        * error and will result in segfaults if not caught here.
>        */
>       if (arm_feature(env, ARM_FEATURE_M)) {
> +        env->nvic = NVIC(object_property_get_link(OBJECT(dev), "nvic", NULL));
>           if (!env->nvic) {
>               error_setg(errp, "This board cannot be used with Cortex-M CPUs");
>               return;
>           }
>       } else {
> -        if (env->nvic) {
> +        if (object_property_find(OBJECT(dev), "nvic")) {
>               error_setg(errp, "This board can only be used with Cortex-M CPUs");
>               return;
>           }