Re: [PATCH for-6.2 02/25] arm: Move systick device creation from NVIC to ARMv7M object

[Luc Michel <luc@lmichel.fr>]

On 10:33 Thu 12 Aug     , Peter Maydell wrote:
> There's no particular reason why the NVIC should be owning the
> SysTick device objects; move them into the ARMv7M container object
> instead, as part of consolidating the "create the devices which are
> built into an M-profile CPU and map them into their architected
> locations in the address space" work into one place.
> 
> This involves temporarily creating a duplicate copy of the
> nvic_sysreg_ns_ops struct and its read/write functions (renamed as
> v7m_sysreg_ns_*), but we will delete the NVIC's copy of this code in
> a subsequent patch.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Luc Michel <luc@lmichel.fr>

> ---
>  include/hw/arm/armv7m.h       |  12 ++++
>  include/hw/intc/armv7m_nvic.h |   4 --
>  hw/arm/armv7m.c               | 125 ++++++++++++++++++++++++++++++++++
>  hw/intc/armv7m_nvic.c         |  73 --------------------
>  4 files changed, 137 insertions(+), 77 deletions(-)
> 
> diff --git a/include/hw/arm/armv7m.h b/include/hw/arm/armv7m.h
> index 4cae0d7eeaa..360c35c5fb2 100644
> --- a/include/hw/arm/armv7m.h
> +++ b/include/hw/arm/armv7m.h
> @@ -60,11 +60,23 @@ struct ARMv7MState {
>      BitBandState bitband[ARMV7M_NUM_BITBANDS];
>      ARMCPU *cpu;
>      ARMv7MRAS ras;
> +    SysTickState systick[M_REG_NUM_BANKS];
>  
>      /* MemoryRegion we pass to the CPU, with our devices layered on
>       * top of the ones the board provides in board_memory.
>       */
>      MemoryRegion container;
> +    /*
> +     * MemoryRegion which passes the transaction to either the S or the
> +     * NS systick device depending on the transaction attributes
> +     */
> +    MemoryRegion systickmem;
> +    /*
> +     * MemoryRegion which enforces the S/NS handling of the systick
> +     * device NS alias region and passes the transaction to the
> +     * NS systick device if appropriate.
> +     */
> +    MemoryRegion systick_ns_mem;
>  
>      /* Properties */
>      char *cpu_type;
> diff --git a/include/hw/intc/armv7m_nvic.h b/include/hw/intc/armv7m_nvic.h
> index 33b6d8810c7..6a6a99090c7 100644
> --- a/include/hw/intc/armv7m_nvic.h
> +++ b/include/hw/intc/armv7m_nvic.h
> @@ -81,16 +81,12 @@ struct NVICState {
>  
>      MemoryRegion sysregmem;
>      MemoryRegion sysreg_ns_mem;
> -    MemoryRegion systickmem;
> -    MemoryRegion systick_ns_mem;
>      MemoryRegion container;
>      MemoryRegion defaultmem;
>  
>      uint32_t num_irq;
>      qemu_irq excpout;
>      qemu_irq sysresetreq;
> -
> -    SysTickState systick[M_REG_NUM_BANKS];
>  };
>  
>  #endif
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index 8964730d153..364ac069702 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -124,6 +124,85 @@ static const hwaddr bitband_output_addr[ARMV7M_NUM_BITBANDS] = {
>      0x22000000, 0x42000000
>  };
>  
> +static MemTxResult v7m_sysreg_ns_write(void *opaque, hwaddr addr,
> +                                       uint64_t value, unsigned size,
> +                                       MemTxAttrs attrs)
> +{
> +    MemoryRegion *mr = opaque;
> +
> +    if (attrs.secure) {
> +        /* S accesses to the alias act like NS accesses to the real region */
> +        attrs.secure = 0;
> +        return memory_region_dispatch_write(mr, addr, value,
> +                                            size_memop(size) | MO_TE, attrs);
> +    } else {
> +        /* NS attrs are RAZ/WI for privileged, and BusFault for user */
> +        if (attrs.user) {
> +            return MEMTX_ERROR;
> +        }
> +        return MEMTX_OK;
> +    }
> +}
> +
> +static MemTxResult v7m_sysreg_ns_read(void *opaque, hwaddr addr,
> +                                      uint64_t *data, unsigned size,
> +                                      MemTxAttrs attrs)
> +{
> +    MemoryRegion *mr = opaque;
> +
> +    if (attrs.secure) {
> +        /* S accesses to the alias act like NS accesses to the real region */
> +        attrs.secure = 0;
> +        return memory_region_dispatch_read(mr, addr, data,
> +                                           size_memop(size) | MO_TE, attrs);
> +    } else {
> +        /* NS attrs are RAZ/WI for privileged, and BusFault for user */
> +        if (attrs.user) {
> +            return MEMTX_ERROR;
> +        }
> +        *data = 0;
> +        return MEMTX_OK;
> +    }
> +}
> +
> +static const MemoryRegionOps v7m_sysreg_ns_ops = {
> +    .read_with_attrs = v7m_sysreg_ns_read,
> +    .write_with_attrs = v7m_sysreg_ns_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static MemTxResult v7m_systick_write(void *opaque, hwaddr addr,
> +                                     uint64_t value, unsigned size,
> +                                     MemTxAttrs attrs)
> +{
> +    ARMv7MState *s = opaque;
> +    MemoryRegion *mr;
> +
> +    /* Direct the access to the correct systick */
> +    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->systick[attrs.secure]), 0);
> +    return memory_region_dispatch_write(mr, addr, value,
> +                                        size_memop(size) | MO_TE, attrs);
> +}
> +
> +static MemTxResult v7m_systick_read(void *opaque, hwaddr addr,
> +                                    uint64_t *data, unsigned size,
> +                                    MemTxAttrs attrs)
> +{
> +    ARMv7MState *s = opaque;
> +    MemoryRegion *mr;
> +
> +    /* Direct the access to the correct systick */
> +    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->systick[attrs.secure]), 0);
> +    return memory_region_dispatch_read(mr, addr, data, size_memop(size) | MO_TE,
> +                                       attrs);
> +}
> +
> +static const MemoryRegionOps v7m_systick_ops = {
> +    .read_with_attrs = v7m_systick_read,
> +    .write_with_attrs = v7m_systick_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
>  static void armv7m_instance_init(Object *obj)
>  {
>      ARMv7MState *s = ARMV7M(obj);
> @@ -137,6 +216,13 @@ static void armv7m_instance_init(Object *obj)
>      object_property_add_alias(obj, "num-irq",
>                                OBJECT(&s->nvic), "num-irq");
>  
> +    object_initialize_child(obj, "systick-reg-ns", &s->systick[M_REG_NS],
> +                            TYPE_SYSTICK);
> +    /*
> +     * We can't initialize the secure systick here, as we don't know
> +     * yet if we need it.
> +     */
> +
>      for (i = 0; i < ARRAY_SIZE(s->bitband); i++) {
>          object_initialize_child(obj, "bitband[*]", &s->bitband[i],
>                                  TYPE_BITBAND);
> @@ -231,6 +317,45 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
>      memory_region_add_subregion(&s->container, 0xe0000000,
>                                  sysbus_mmio_get_region(sbd, 0));
>  
> +    /* Create and map the systick devices */
> +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->systick[M_REG_NS]), errp)) {
> +        return;
> +    }
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->systick[M_REG_NS]), 0,
> +                       qdev_get_gpio_in_named(DEVICE(&s->nvic),
> +                                              "systick-trigger", M_REG_NS));
> +
> +    if (arm_feature(&s->cpu->env, ARM_FEATURE_M_SECURITY)) {
> +        /*
> +         * We couldn't init the secure systick device in instance_init
> +         * as we didn't know then if the CPU had the security extensions;
> +         * so we have to do it here.
> +         */
> +        object_initialize_child(OBJECT(dev), "systick-reg-s",
> +                                &s->systick[M_REG_S], TYPE_SYSTICK);
> +
> +        if (!sysbus_realize(SYS_BUS_DEVICE(&s->systick[M_REG_S]), errp)) {
> +            return;
> +        }
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->systick[M_REG_S]), 0,
> +                           qdev_get_gpio_in_named(DEVICE(&s->nvic),
> +                                                  "systick-trigger", M_REG_S));
> +    }
> +
> +    memory_region_init_io(&s->systickmem, OBJECT(s),
> +                          &v7m_systick_ops, s,
> +                          "v7m_systick", 0xe0);
> +
> +    memory_region_add_subregion_overlap(&s->container, 0xe000e010,
> +                                        &s->systickmem, 1);
> +    if (arm_feature(&s->cpu->env, ARM_FEATURE_V8)) {
> +        memory_region_init_io(&s->systick_ns_mem, OBJECT(s),
> +                              &v7m_sysreg_ns_ops, &s->systickmem,
> +                              "v7m_systick_ns", 0xe0);
> +        memory_region_add_subregion_overlap(&s->container, 0xe002e010,
> +                                            &s->systick_ns_mem, 1);
> +    }
> +
>      /* If the CPU has RAS support, create the RAS register block */
>      if (cpu_isar_feature(aa32_ras, s->cpu)) {
>          object_initialize_child(OBJECT(dev), "armv7m-ras",
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index a5975592dfa..2b3e79a3da9 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -2517,38 +2517,6 @@ static const MemoryRegionOps nvic_sysreg_ns_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -static MemTxResult nvic_systick_write(void *opaque, hwaddr addr,
> -                                      uint64_t value, unsigned size,
> -                                      MemTxAttrs attrs)
> -{
> -    NVICState *s = opaque;
> -    MemoryRegion *mr;
> -
> -    /* Direct the access to the correct systick */
> -    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->systick[attrs.secure]), 0);
> -    return memory_region_dispatch_write(mr, addr, value,
> -                                        size_memop(size) | MO_TE, attrs);
> -}
> -
> -static MemTxResult nvic_systick_read(void *opaque, hwaddr addr,
> -                                     uint64_t *data, unsigned size,
> -                                     MemTxAttrs attrs)
> -{
> -    NVICState *s = opaque;
> -    MemoryRegion *mr;
> -
> -    /* Direct the access to the correct systick */
> -    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->systick[attrs.secure]), 0);
> -    return memory_region_dispatch_read(mr, addr, data, size_memop(size) | MO_TE,
> -                                       attrs);
> -}
> -
> -static const MemoryRegionOps nvic_systick_ops = {
> -    .read_with_attrs = nvic_systick_read,
> -    .write_with_attrs = nvic_systick_write,
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> -};
> -
>  /*
>   * Unassigned portions of the PPB space are RAZ/WI for privileged
>   * accesses, and fault for non-privileged accesses.
> @@ -2801,29 +2769,6 @@ static void armv7m_nvic_realize(DeviceState *dev, Error **errp)
>  
>      s->num_prio_bits = arm_feature(&s->cpu->env, ARM_FEATURE_V7) ? 8 : 2;
>  
> -    if (!sysbus_realize(SYS_BUS_DEVICE(&s->systick[M_REG_NS]), errp)) {
> -        return;
> -    }
> -    sysbus_connect_irq(SYS_BUS_DEVICE(&s->systick[M_REG_NS]), 0,
> -                       qdev_get_gpio_in_named(dev, "systick-trigger",
> -                                              M_REG_NS));
> -
> -    if (arm_feature(&s->cpu->env, ARM_FEATURE_M_SECURITY)) {
> -        /* We couldn't init the secure systick device in instance_init
> -         * as we didn't know then if the CPU had the security extensions;
> -         * so we have to do it here.
> -         */
> -        object_initialize_child(OBJECT(dev), "systick-reg-s",
> -                                &s->systick[M_REG_S], TYPE_SYSTICK);
> -
> -        if (!sysbus_realize(SYS_BUS_DEVICE(&s->systick[M_REG_S]), errp)) {
> -            return;
> -        }
> -        sysbus_connect_irq(SYS_BUS_DEVICE(&s->systick[M_REG_S]), 0,
> -                           qdev_get_gpio_in_named(dev, "systick-trigger",
> -                                                  M_REG_S));
> -    }
> -
>      /*
>       * This device provides a single sysbus memory region which
>       * represents the whole of the "System PPB" space. This is the
> @@ -2877,23 +2822,11 @@ static void armv7m_nvic_realize(DeviceState *dev, Error **errp)
>                            "nvic_sysregs", 0x1000);
>      memory_region_add_subregion(&s->container, 0xe000, &s->sysregmem);
>  
> -    memory_region_init_io(&s->systickmem, OBJECT(s),
> -                          &nvic_systick_ops, s,
> -                          "nvic_systick", 0xe0);
> -
> -    memory_region_add_subregion_overlap(&s->container, 0xe010,
> -                                        &s->systickmem, 1);
> -
>      if (arm_feature(&s->cpu->env, ARM_FEATURE_V8)) {
>          memory_region_init_io(&s->sysreg_ns_mem, OBJECT(s),
>                                &nvic_sysreg_ns_ops, &s->sysregmem,
>                                "nvic_sysregs_ns", 0x1000);
>          memory_region_add_subregion(&s->container, 0x2e000, &s->sysreg_ns_mem);
> -        memory_region_init_io(&s->systick_ns_mem, OBJECT(s),
> -                              &nvic_sysreg_ns_ops, &s->systickmem,
> -                              "nvic_systick_ns", 0xe0);
> -        memory_region_add_subregion_overlap(&s->container, 0x2e010,
> -                                            &s->systick_ns_mem, 1);
>      }
>  
>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->container);
> @@ -2905,12 +2838,6 @@ static void armv7m_nvic_instance_init(Object *obj)
>      NVICState *nvic = NVIC(obj);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>  
> -    object_initialize_child(obj, "systick-reg-ns", &nvic->systick[M_REG_NS],
> -                            TYPE_SYSTICK);
> -    /* We can't initialize the secure systick here, as we don't know
> -     * yet if we need it.
> -     */
> -
>      sysbus_init_irq(sbd, &nvic->excpout);
>      qdev_init_gpio_out_named(dev, &nvic->sysresetreq, "SYSRESETREQ", 1);
>      qdev_init_gpio_in_named(dev, nvic_systick_trigger, "systick-trigger",
> -- 
> 2.20.1
> 

--