Re: [Qemu-devel] [Qemu-arm] [PATCH 2/2] nvic: Make systick banked

[Philippe Mathieu-Daudé]

Hi Peter,

On 12/01/2017 03:51 PM, Peter Maydell wrote:
> For the v8M security extension, there should be two systick
> devices, which use separate banked systick exceptions. The
> register interface is banked in the same way as for other
> banked registers, including the existence of an NS alias
> region for secure code to access the nonsecure timer.
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  include/hw/intc/armv7m_nvic.h |  4 ++-
>  hw/intc/armv7m_nvic.c         | 81 
> ++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 72 insertions(+), 13 deletions(-)
> 
> diff --git a/include/hw/intc/armv7m_nvic.h b/include/hw/intc/armv7m_nvic.h
> index ac7997c..8bc2911 100644
> --- a/include/hw/intc/armv7m_nvic.h
> +++ b/include/hw/intc/armv7m_nvic.h
> @@ -78,13 +78,15 @@ typedef struct NVICState {
>  
>      MemoryRegion sysregmem;
>      MemoryRegion sysreg_ns_mem;
> +    MemoryRegion systickmem;
> +    MemoryRegion systick_ns_mem;
>      MemoryRegion container;
>  
>      uint32_t num_irq;
>      qemu_irq excpout;
>      qemu_irq sysresetreq;
>  
> -    SysTickState systick;
> +    SysTickState systick[M_REG_NUM_BANKS];
>  } NVICState;
>  
>  #endif
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 63f2743..5cb44f4 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -1827,6 +1827,36 @@ 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, 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, attrs);
> +}
> +
> +static const MemoryRegionOps nvic_systick_ops = {
> +    .read_with_attrs = nvic_systick_read,
> +    .write_with_attrs = nvic_systick_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
>  static int nvic_post_load(void *opaque, int version_id)
>  {
>      NVICState *s = opaque;
> @@ -2005,17 +2035,16 @@ static void nvic_systick_trigger(void *opaque, int n, 
> int level)
>          /* SysTick just asked us to pend its exception.
>           * (This is different from an external interrupt line's
>           * behaviour.)
> -         * TODO: when we implement the banked systicks we must make
> -         * this pend the correct banked exception.
> +         * n == 0 : NonSecure systick
> +         * n == 1 : Secure systick
>           */
> -        armv7m_nvic_set_pending(s, ARMV7M_EXCP_SYSTICK, false);
> +        armv7m_nvic_set_pending(s, ARMV7M_EXCP_SYSTICK, n);
>      }
>  }
>  
>  static void armv7m_nvic_realize(DeviceState *dev, Error **errp)
>  {
>      NVICState *s = NVIC(dev);
> -    SysBusDevice *systick_sbd;
>      Error *err = NULL;
>      int regionlen;
>  
> @@ -2032,15 +2061,30 @@ static void armv7m_nvic_realize(DeviceState *dev, 
> Error **errp)
>      /* include space for internal exception vectors */
>      s->num_irq += NVIC_FIRST_IRQ;
>  
> -    object_property_set_bool(OBJECT(&s->systick), true, "realized", &err);
> +    object_property_set_bool(OBJECT(&s->systick[M_REG_NS]), true,
> +                             "realized", &err);
>      if (err != NULL) {
>          error_propagate(errp, err);
>          return;
>      }
> -    systick_sbd = SYS_BUS_DEVICE(&s->systick);
> -    sysbus_connect_irq(systick_sbd, 0,
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->systick[M_REG_NS]), 0,
>                         qdev_get_gpio_in_named(dev, "systick-trigger", 0));
>  
> +    if (arm_feature(&s->cpu->env, ARM_FEATURE_M_SECURITY)) {
> +        object_initialize(&s->systick[M_REG_S], sizeof(s->systick[M_REG_NS]),
> +                          TYPE_SYSTICK);
> +        qdev_set_parent_bus(DEVICE(&s->systick[M_REG_S]), 
> sysbus_get_default());

I got stuck here some time wondering why you init the S systick in this
realize(), then continue and read your comment in
armv7m_nvic_instance_init() and it made sens. I'm tempted to add a
similar comment here.

> +
> +        object_property_set_bool(OBJECT(&s->systick[M_REG_S]), true,
> +                                 "realized", &err);
> +        if (err != NULL) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->systick[M_REG_S]), 0,
> +                           qdev_get_gpio_in_named(dev, "systick-trigger", 
> 1));
> +    }
> +
>      /* The NVIC and System Control Space (SCS) starts at 0xe000e000
>       * and looks like this:
>       *  0x004 - ICTR
> @@ -2073,15 +2117,24 @@ static void armv7m_nvic_realize(DeviceState *dev, 
> Error **errp)
>      memory_region_init_io(&s->sysregmem, OBJECT(s), &nvic_sysreg_ops, s,
>                            "nvic_sysregs", 0x1000);
>      memory_region_add_subregion(&s->container, 0, &s->sysregmem);
> +
> +    memory_region_init_io(&s->systickmem, OBJECT(s),
> +                          &nvic_systick_ops, s,
> +                          "nvic_systick", 0xe0);
> +
>      memory_region_add_subregion_overlap(&s->container, 0x10,
> -                                        sysbus_mmio_get_region(systick_sbd, 
> 0),
> -                                        1);
> +                                        &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, 0x20000, 
> &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, 0x20010,
> +                                            &s->systick_ns_mem, 1);
>      }
>  
>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->container);
> @@ -2099,12 +2152,16 @@ static void armv7m_nvic_instance_init(Object *obj)
>      NVICState *nvic = NVIC(obj);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>  
> -    object_initialize(&nvic->systick, sizeof(nvic->systick), TYPE_SYSTICK);
> -    qdev_set_parent_bus(DEVICE(&nvic->systick), sysbus_get_default());
> +    object_initialize(&nvic->systick[M_REG_NS],
> +                      sizeof(nvic->systick[M_REG_NS]), TYPE_SYSTICK);
> +    qdev_set_parent_bus(DEVICE(&nvic->systick[M_REG_NS]), 
> sysbus_get_default());
> +    /* 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", 1);
> +    qdev_init_gpio_in_named(dev, nvic_systick_trigger, "systick-trigger", 2);

Using this #define helps (me) while reviewing:

    qdev_init_gpio_in_named(dev, nvic_systick_trigger,
                            "systick-trigger", M_REG_NUM_BANKS);

One more line but you are still under a 100 diffstat series ;)

Anyway:
Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

>  }
>  
>  static void armv7m_nvic_class_init(ObjectClass *klass, void *data)
>