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

[Michael Davidsaver]

On 11/17/2015 12:58 PM, Peter Maydell wrote:
> 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.

Just so, removed.

>> +
>> +    /* 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.)

Fixed

>> +
>>  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.

I'll try to do this.