| Message ID | 1505668548-16616-8-git-send-email-mark.cave-ayland@ilande.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Sep 17, 2017 at 06:15:47PM +0100, Mark Cave-Ayland wrote: > Observation of the code shows indicates that several timer fields are > missing from the migration stream. So, adding new things to the migration stream always requires some thought. The commit message should contain a rationale for why the proposed representation is a good one. In particular is it one that's unlikely to be difficult if the device changes internal details in future. When possible it's best to stick to architected documented state of the modelled hardware, though I can see you're probably going to need some extras in this case. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/intc/openpic.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c > index 10d6e87..debfcbf 100644 > --- a/hw/intc/openpic.c > +++ b/hw/intc/openpic.c > @@ -1570,11 +1570,14 @@ static const VMStateDescription vmstate_openpic_irqsource = { > > static const VMStateDescription vmstate_openpic_timer = { > .name = "openpic_timer", > - .version_id = 0, > - .minimum_version_id = 0, > + .version_id = 1, > + .minimum_version_id = 1, > .fields = (VMStateField[]) { > VMSTATE_UINT32(tccr, OpenPICTimer), > VMSTATE_UINT32(tbcr, OpenPICTimer), > + VMSTATE_BOOL(qemu_timer_active, OpenPICTimer), > + VMSTATE_TIMER_PTR(qemu_timer, OpenPICTimer), > + VMSTATE_UINT64(origin_time, OpenPICTimer), > VMSTATE_END_OF_LIST() > } > }; > @@ -1620,7 +1623,7 @@ static const VMStateDescription vmstate_openpic = { > VMSTATE_UINT32_EQUAL(nb_cpus, OpenPICState, NULL), > VMSTATE_STRUCT_VARRAY_UINT32(dst, OpenPICState, nb_cpus, 0, > vmstate_openpic_irqdest, IRQDest), > - VMSTATE_STRUCT_ARRAY(timers, OpenPICState, OPENPIC_MAX_TMR, 0, > + VMSTATE_STRUCT_ARRAY(timers, OpenPICState, OPENPIC_MAX_TMR, 1, > vmstate_openpic_timer, OpenPICTimer), > VMSTATE_STRUCT_ARRAY(msi, OpenPICState, MAX_MSI, 0, > vmstate_openpic_msi, OpenPICMSI),
On 18/09/17 23:44, David Gibson wrote: > So, adding new things to the migration stream always requires some > thought. The commit message should contain a rationale for why the > proposed representation is a good one. In particular is it one that's > unlikely to be difficult if the device changes internal details in > future. When possible it's best to stick to architected documented > state of the modelled hardware, though I can see you're probably going > to need some extras in this case. Yes, I think that's the case here. Definitely the timer can't be armed correctly without origin_time, and if qemu_timer_active isn't present and migration occurs with an active timer then a subsequent call to openpic_tmr_get_timer() will be incorrect. Does that sound right to you? ATB, Mark.
On Tue, Sep 19, 2017 at 08:50:49PM +0100, Mark Cave-Ayland wrote: > On 18/09/17 23:44, David Gibson wrote: > > > So, adding new things to the migration stream always requires some > > thought. The commit message should contain a rationale for why the > > proposed representation is a good one. In particular is it one that's > > unlikely to be difficult if the device changes internal details in > > future. When possible it's best to stick to architected documented > > state of the modelled hardware, though I can see you're probably going > > to need some extras in this case. > > Yes, I think that's the case here. Definitely the timer can't be armed > correctly without origin_time, and if qemu_timer_active isn't present > and migration occurs with an active timer then a subsequent call to > openpic_tmr_get_timer() will be incorrect. Does that sound right to > you? You'll certainly need some sort of time variable, which could be expressed in various ways equivalently. I don't know if you'll need an explicit extra "active" boolean - are there any openpic registers which would indicate if a timer is active? It's generally best to represent state using documented forms from the hardware whenever possible.
diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c index 10d6e87..debfcbf 100644 --- a/hw/intc/openpic.c +++ b/hw/intc/openpic.c @@ -1570,11 +1570,14 @@ static const VMStateDescription vmstate_openpic_irqsource = { static const VMStateDescription vmstate_openpic_timer = { .name = "openpic_timer", - .version_id = 0, - .minimum_version_id = 0, + .version_id = 1, + .minimum_version_id = 1, .fields = (VMStateField[]) { VMSTATE_UINT32(tccr, OpenPICTimer), VMSTATE_UINT32(tbcr, OpenPICTimer), + VMSTATE_BOOL(qemu_timer_active, OpenPICTimer), + VMSTATE_TIMER_PTR(qemu_timer, OpenPICTimer), + VMSTATE_UINT64(origin_time, OpenPICTimer), VMSTATE_END_OF_LIST() } }; @@ -1620,7 +1623,7 @@ static const VMStateDescription vmstate_openpic = { VMSTATE_UINT32_EQUAL(nb_cpus, OpenPICState, NULL), VMSTATE_STRUCT_VARRAY_UINT32(dst, OpenPICState, nb_cpus, 0, vmstate_openpic_irqdest, IRQDest), - VMSTATE_STRUCT_ARRAY(timers, OpenPICState, OPENPIC_MAX_TMR, 0, + VMSTATE_STRUCT_ARRAY(timers, OpenPICState, OPENPIC_MAX_TMR, 1, vmstate_openpic_timer, OpenPICTimer), VMSTATE_STRUCT_ARRAY(msi, OpenPICState, MAX_MSI, 0, vmstate_openpic_msi, OpenPICMSI),
Observation of the code shows indicates that several timer fields are missing from the migration stream. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/intc/openpic.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)