Re: [Qemu-arm] [PATCH 09/18] armv7m: NVIC update vmstate

[Peter Maydell]

On 9 November 2015 at 01:11, Michael Davidsaver <address@hidden> wrote:
> Signed-off-by: Michael Davidsaver <address@hidden>
> ---
>  hw/intc/armv7m_nvic.c | 64 
> +++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 62 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 3b10dee..c860b36 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -810,15 +810,75 @@ static const MemoryRegionOps nvic_sysreg_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>
> +static
> +int nvic_post_load(void *opaque, int version_id)
> +{
> +    nvic_state *s = opaque;
> +    unsigned i;
> +
> +    /* evil hack to get ARMCPU* ahead of time */
> +    assert(cpus.tqh_first);
> +    assert(!CPU_NEXT(cpus.tqh_first));
> +    s->cpu = ARM_CPU(cpus.tqh_first);
> +    assert(s->cpu);

Why do we need to do this? By the time we get to loading
state into the system, s->cpu should already have been set.

> +
> +    /* recalculate priorities */
> +    for (i = 4; i < s->num_irq; i++) {
> +        set_prio(s, i, s->vectors[i].raw_prio);
> +    }
> +
> +    nvic_irq_update(s, highest_runnable_prio(s->cpu));
> +
> +    return 0;
> +}
> +
> +static
> +int vec_info_get(QEMUFile *f, void *pv, size_t size)
> +{
> +    vec_info *I = pv;
> +    I->prio_sub = qemu_get_be16(f);
> +    I->prio_group = qemu_get_byte(f);
> +    I->raw_prio = qemu_get_ubyte(f);
> +    I->enabled = qemu_get_ubyte(f);
> +    I->pending = qemu_get_ubyte(f);
> +    I->active = qemu_get_ubyte(f);
> +    I->level = qemu_get_ubyte(f);
> +    return 0;
> +}
> +
> +static
> +void vec_info_put(QEMUFile *f, void *pv, size_t size)
> +{
> +    vec_info *I = pv;
> +    qemu_put_be16(f, I->prio_sub);
> +    qemu_put_byte(f, I->prio_group);
> +    qemu_put_ubyte(f, I->raw_prio);
> +    qemu_put_ubyte(f, I->enabled);
> +    qemu_put_ubyte(f, I->pending);
> +    qemu_put_ubyte(f, I->active);
> +    qemu_put_ubyte(f, I->level);
> +}
> +
> +static const VMStateInfo vmstate_info = {
> +    .name = "armv7m_nvic_info",
> +    .get = vec_info_get,
> +    .put = vec_info_put,
> +};

I don't think there's any need to use get and put functions here:
better to define a VMStateDescripton for the struct vec_info,
and then you can just refer to that from the main vmstate_nvic
description. (hw/audio/pl041.c has some examples of this.)

> +
>  static const VMStateDescription vmstate_nvic = {
>      .name = "armv7m_nvic",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
> +    .post_load = &nvic_post_load,
>      .fields = (VMStateField[]) {
> +        VMSTATE_ARRAY(vectors, nvic_state, NVIC_MAX_VECTORS, 0,
> +                      vmstate_info, vec_info),
> +        VMSTATE_UINT8(prigroup, nvic_state),
>          VMSTATE_UINT32(systick.control, nvic_state),
>          VMSTATE_UINT32(systick.reload, nvic_state),
>          VMSTATE_INT64(systick.tick, nvic_state),
>          VMSTATE_TIMER_PTR(systick.timer, nvic_state),
> +        VMSTATE_UINT32(num_irq, nvic_state),
>          VMSTATE_END_OF_LIST()
>      }
>  };

Ideally the VMState should be added in the same patches that
add new fields or structures, rather than in its own patch
at the end.

thanks
-- PMM