| Message ID | 1299270860-8927-2-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 03/04/2011 09:34 PM, Peter Maydell wrote: > .name = "realview_sysctl", > - .version_id = 1, > - .minimum_version_id = 1, > + .version_id = 2, > + .minimum_version_id = 2, > .fields = (VMStateField[]) { > VMSTATE_UINT32(leds, arm_sysctl_state), > VMSTATE_UINT16(lockval, arm_sysctl_state), > @@ -41,6 +44,9 @@ static const VMStateDescription vmstate_arm_sysctl = { > VMSTATE_UINT32(flags, arm_sysctl_state), > VMSTATE_UINT32(nvflags, arm_sysctl_state), > VMSTATE_UINT32(resetlevel, arm_sysctl_state), > + VMSTATE_UINT32(sys_cfgdata, arm_sysctl_state), > + VMSTATE_UINT32(sys_cfgctrl, arm_sysctl_state), > + VMSTATE_UINT32(sys_cfgstat, arm_sysctl_state), > VMSTATE_END_OF_LIST() > } You need to present the fields as version 2-only. Paolo
On 5 March 2011 12:11, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 03/04/2011 09:34 PM, Peter Maydell wrote: >> >> .name = "realview_sysctl", >> - .version_id = 1, >> - .minimum_version_id = 1, >> + .version_id = 2, >> + .minimum_version_id = 2, >> .fields = (VMStateField[]) { >> VMSTATE_UINT32(leds, arm_sysctl_state), >> VMSTATE_UINT16(lockval, arm_sysctl_state), >> @@ -41,6 +44,9 @@ static const VMStateDescription vmstate_arm_sysctl = { >> VMSTATE_UINT32(flags, arm_sysctl_state), >> VMSTATE_UINT32(nvflags, arm_sysctl_state), >> VMSTATE_UINT32(resetlevel, arm_sysctl_state), >> + VMSTATE_UINT32(sys_cfgdata, arm_sysctl_state), >> + VMSTATE_UINT32(sys_cfgctrl, arm_sysctl_state), >> + VMSTATE_UINT32(sys_cfgstat, arm_sysctl_state), >> VMSTATE_END_OF_LIST() >> } > > You need to present the fields as version 2-only. Can you give an example/explanation? docs/migration.txt doesn't seem to cover this... thanks -- PMM
On 03/05/2011 01:34 PM, Peter Maydell wrote: >>> >> + VMSTATE_UINT32(sys_cfgdata, arm_sysctl_state), >>> >> + VMSTATE_UINT32(sys_cfgctrl, arm_sysctl_state), >>> >> + VMSTATE_UINT32(sys_cfgstat, arm_sysctl_state), >>> >> VMSTATE_END_OF_LIST() >>> >> } >> > >> > You need to present the fields as version 2-only. > Can you give an example/explanation? docs/migration.txt doesn't > seem to cover this... Sure, sorry for being terse. It simply needs to be: VMSTATE_UINT32_V(sys_cfgdata, arm_sysctl_state, 2), VMSTATE_UINT32_V(sys_cfgctrl, arm_sysctl_state, 2), VMSTATE_UINT32_V(sys_cfgstat, arm_sysctl_state, 2), Also, minimum_version_id needs to remain 1 since you do support loading version 1 saved virtual machines. Paolo
On 5 March 2011 14:59, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 03/05/2011 01:34 PM, Peter Maydell wrote: >> Can you give an example/explanation? docs/migration.txt doesn't >> seem to cover this... > > Sure, sorry for being terse. It simply needs to be: > > VMSTATE_UINT32_V(sys_cfgdata, arm_sysctl_state, 2), > VMSTATE_UINT32_V(sys_cfgctrl, arm_sysctl_state, 2), > VMSTATE_UINT32_V(sys_cfgstat, arm_sysctl_state, 2), > > Also, minimum_version_id needs to remain 1 since you do support loading > version 1 saved virtual machines. Ah. I was just following Juan's suggestion: > - if you don't care about backward compatibility, just add +1 to all the > version fields and you are done. but I guess if it's that trivial we might as well support it. (My guess is that basically nobody is doing any kind of migration of ARM TCG VMs, so I think it's all a bit moot.) What do the new fields get set to when you do a load from a v1 snapshot? Other random snapshot/vmstate questions while I'm here: (1) Is there supposed to be any kind of guard on trying to do a vmsave on a system with devices that don't implement save/load? IME it just produces a snapshot which doesn't work when you reload it... (2) How do you track incompatible changes at the machine level? For instance, suppose we accidentally forgot to model a NOT gate in an IRQ line, so we add a qemu_irq_invert() to the machine init function. None of the devices have changed, but you can't load the state of an old version of the machine into a new version, because then the devices on either end of the inverter would be inconsistent about the state of the line. But there's no version number for a machine as far as I can see. I've appended a draft of a suggested extra section for docs/migration.txt so you can tell me if I've misunderstood it all :-) ---begin--- === Adding state fields to a device === If you make a bugfix or enhancement to a device which requires the addition of extra state, you need to add these new state fields to the VMStateDescription so that: (1) they are saved and loaded correctly (2) migration between the new and old versions either works or fails cleanly. If the change is such that migration between versions would not work anyway, you can just add the new fields using the usual VMSTATE_* macros, increment the version_id and set the minimum_version_id to be the same as the version_id. Migration from the old version to the new version can be supported if it is OK for the new fields to remain in their default state [XXX is this right? are they zeroed, or do they get the value the device's reset function sets them to, or something else?] when the state of an old-version snapshot is loaded. To implement this you need to use the VMSTATE_*_V macros which let you specify the version in which a field was introduced, for instance: VMSTATE_UINT32_V(sys_cfgdata, arm_sysctl_state, 2) for a field introduced in version 2. You should also increment the version_id, but leave the minimum_version_id unchanged. Newly added VMSTATE_*_V fields should go at the end of the VMState description. ---endit--- thanks -- PMM
On 03/05/2011 05:50 PM, Peter Maydell wrote: > (1) Is there supposed to be any kind of guard on trying to > do a vmsave on a system with devices that don't implement > save/load? IME it just produces a snapshot which doesn't > work when you reload it... I think you're right, devices currently have to call register_device_unmigratable manually. I guess you could add support to qdev, so that qdev-ified devices could specify a special "forbid migration" value for the vmsd field. Alternatively, you could have NULL mean "forbid migration" and a special value for "do not save anything, it will just work". > (2) How do you track incompatible changes at the machine > level? For instance, suppose we accidentally forgot to > model a NOT gate in an IRQ line, so we add a qemu_irq_invert() > to the machine init function. None of the devices have > changed, but you can't load the state of an old version > of the machine into a new version, because then the > devices on either end of the inverter would be inconsistent > about the state of the line. But there's no version number > for a machine as far as I can see. You can change the machine model and keep the incompatible machine as a separate model. A machine can specify compatibility properties that are meant exactly for this kind of bug-compatible behavior. Reloading the VM must be done with the correct -M switch for the old model. Examples of how to do this are in hw/pc_piix.c (which will provide good ideas for further grepping, too :)). Paolo
Peter Maydell <peter.maydell@linaro.org> wrote: > On 5 March 2011 14:59, Paolo Bonzini <pbonzini@redhat.com> wrote: >> On 03/05/2011 01:34 PM, Peter Maydell wrote: sorry, I miss this email before. > > Ah. I was just following Juan's suggestion: >> - if you don't care about backward compatibility, just add +1 to all the >> version fields and you are done. > > but I guess if it's that trivial we might as well support it. > (My guess is that basically nobody is doing any kind of > migration of ARM TCG VMs, so I think it's all a bit moot.) > > What do the new fields get set to when you do a load from > a v1 snapshot? > > Other random snapshot/vmstate questions while I'm here: > > (1) Is there supposed to be any kind of guard on trying to > do a vmsave on a system with devices that don't implement > save/load? IME it just produces a snapshot which doesn't > work when you reload it... only real way to do it is to set a device that is not migratable. Only having devices that don't have a savevm section would not work, because for instance usb devices don't need a savevm section. > (2) How do you track incompatible changes at the machine > level? For instance, suppose we accidentally forgot to > model a NOT gate in an IRQ line, so we add a qemu_irq_invert() > to the machine init function. None of the devices have > changed, but you can't load the state of an old version > of the machine into a new version, because then the > devices on either end of the inverter would be inconsistent > about the state of the line. But there's no version number > for a machine as far as I can see. Dunno. I have only worked to x86*, no clue about integrated boards that have "interesting" conections. At some point we would have to "define" the machine board better, just now it is not there. > I've appended a draft of a suggested extra section for > docs/migration.txt so you can tell me if I've misunderstood > it all :-) > > ---begin--- > === Adding state fields to a device === > > If you make a bugfix or enhancement to a device which requires the > addition of extra state, you need to add these new state fields > to the VMStateDescription so that: > (1) they are saved and loaded correctly > (2) migration between the new and old versions either works > or fails cleanly. > > If the change is such that migration between versions would not > work anyway, you can just add the new fields using the usual > VMSTATE_* macros, increment the version_id and set the > minimum_version_id to be the same as the version_id. > > Migration from the old version to the new version can be supported > if it is OK for the new fields to remain in their default state > [XXX is this right? are they zeroed, or do they get the value > the device's reset function sets them to, or something else?] You can initialize in your init function at the value that you want, or use foo_post_load() function (that receives the version as a parameter) to assign to any correct values that you need. > when the state of an old-version snapshot is loaded. To implement > this you need to use the VMSTATE_*_V macros which let you specify > the version in which a field was introduced, for instance: > > VMSTATE_UINT32_V(sys_cfgdata, arm_sysctl_state, 2) > > for a field introduced in version 2. You should also increment > the version_id, but leave the minimum_version_id unchanged. > Newly added VMSTATE_*_V fields should go at the end of the > VMState description. Just to make things more complicated, this has been "deprecated" O:-) Ok, I am going to try to explain it myself. We have one vmstate section. const VMStateDescription vmstate_foo = { .name = "foo", .version_id = 0, .minimum_version_id = 0, .minimum_version_id_old = 0, .fields = (VMStateField []) { VMSTATE_INT32(bar, FOOState), VMSTATE_END_OF_LIST() } }; And we then notice that we need another int (bar2) on the state. Posibilities: - We know that old device was wrong, and that there is no way we can load (reliabely) from version 0. Then we just increase the version: const VMStateDescription vmstate_foo = { .name = "foo", .version_id = 1, .minimum_version_id = 1, .minimum_version_id_old = 1, .fields = (VMStateField []) { VMSTATE_INT32(bar, FOOState), VMSTATE_INT32(bar2, FOOState), VMSTATE_END_OF_LIST() } }; And we are done. - We know that we can load from v1. But that we want to always sent bar2 for migration, then we just increase versions to: const VMStateDescription vmstate_foo = { .name = "foo", .version_id = 2, .minimum_version_id = 1, .minimum_version_id_old = 1, .fields = (VMStateField []) { VMSTATE_INT32(bar, FOOState), VMSTATE_INT32_V(bar2, FOOState, 1), VMSTATE_END_OF_LIST() } }; And we are done. We are able to receive state 0 and 1, and we would always sent version 1. - We know that bar2 is only need sometimes, and we know what sometimes mean. Then we use a new subsection. /* this function returns true if bar2 is needed */ static bool foo_bar2_needed(void *opaque) { FOOState *s = opaque; return ......; } const VMStateDescription vmstate_bar2 = { .name = "foo/bar2", .version_id = 0, .minimum_version_id = 0, .minimum_version_id_old = 0, .fields = (VMStateField []) { VMSTATE_INT32(bar2, FOOState), VMSTATE_END_OF_LIST() } }; const VMStateDescription vmstate_foo = { .name = "foo", .version_id = 2, .minimum_version_id = 1, .minimum_version_id_old = 1, .fields = (VMStateField []) { VMSTATE_INT32(bar, FOOState), VMSTATE_INT32_V(bar2, FOOState, 1), VMSTATE_END_OF_LIST() } , .subsections = (VMStateSubsection []) { { .vmsd = &vmstate_bar2, .needed = foo_bar2_needed, }, { /* empty */ } } }; Why do we want to go to all this trouble? To be able to do a migration from a new version to one old version. If bar2 is not needed, we know that we can _not_ send bar2 subsection. If bar2 is needed, we just sent it and we fail the migration to the old version. Notice that this only makes sense if foo_bar2_needed() normally returns false, if it returns always true, it is just as good to just increase the version. An example on when this is useful is with ide. When we defined vmstate_ide_drive, we forgot the pio state. So, it always worked well execpt if we were in the middle of a pio operation. PIO operations are only used (normally) during boot. Adding a new subsection means that we normally don't sent the pio state, except if it is really needed. Have I manage to explain myself a little bit? Later, Juan.
diff --git a/hw/arm_sysctl.c b/hw/arm_sysctl.c index 799b007..e4a17f5 100644 --- a/hw/arm_sysctl.c +++ b/hw/arm_sysctl.c @@ -27,12 +27,15 @@ typedef struct { uint32_t resetlevel; uint32_t proc_id; uint32_t sys_mci; + uint32_t sys_cfgdata; + uint32_t sys_cfgctrl; + uint32_t sys_cfgstat; } arm_sysctl_state; static const VMStateDescription vmstate_arm_sysctl = { .name = "realview_sysctl", - .version_id = 1, - .minimum_version_id = 1, + .version_id = 2, + .minimum_version_id = 2, .fields = (VMStateField[]) { VMSTATE_UINT32(leds, arm_sysctl_state), VMSTATE_UINT16(lockval, arm_sysctl_state), @@ -41,6 +44,9 @@ static const VMStateDescription vmstate_arm_sysctl = { VMSTATE_UINT32(flags, arm_sysctl_state), VMSTATE_UINT32(nvflags, arm_sysctl_state), VMSTATE_UINT32(resetlevel, arm_sysctl_state), + VMSTATE_UINT32(sys_cfgdata, arm_sysctl_state), + VMSTATE_UINT32(sys_cfgctrl, arm_sysctl_state), + VMSTATE_UINT32(sys_cfgstat, arm_sysctl_state), VMSTATE_END_OF_LIST() } }; @@ -53,6 +59,7 @@ static const VMStateDescription vmstate_arm_sysctl = { #define BOARD_ID_EB 0x140 #define BOARD_ID_PBA8 0x178 #define BOARD_ID_PBX 0x182 +#define BOARD_ID_VEXPRESS 0x190 static int board_id(arm_sysctl_state *s) { @@ -104,6 +111,10 @@ static uint32_t arm_sysctl_read(void *opaque, target_phys_addr_t offset) case 0x38: /* NVFLAGS */ return s->nvflags; case 0x40: /* RESETCTL */ + if (board_id(s) == BOARD_ID_VEXPRESS) { + /* reserved: RAZ/WI */ + return 0; + } return s->resetlevel; case 0x44: /* PCICTL */ return 1; @@ -142,7 +153,23 @@ static uint32_t arm_sysctl_read(void *opaque, target_phys_addr_t offset) case 0xcc: /* SYS_TEST_OSC3 */ case 0xd0: /* SYS_TEST_OSC4 */ return 0; + case 0xa0: /* SYS_CFGDATA */ + if (board_id(s) != BOARD_ID_VEXPRESS) { + goto bad_reg; + } + return s->sys_cfgdata; + case 0xa4: /* SYS_CFGCTRL */ + if (board_id(s) != BOARD_ID_VEXPRESS) { + goto bad_reg; + } + return s->sys_cfgctrl; + case 0xa8: /* SYS_CFGSTAT */ + if (board_id(s) != BOARD_ID_VEXPRESS) { + goto bad_reg; + } + return s->sys_cfgstat; default: + bad_reg: printf ("arm_sysctl_read: Bad register offset 0x%x\n", (int)offset); return 0; } @@ -190,6 +217,10 @@ static void arm_sysctl_write(void *opaque, target_phys_addr_t offset, s->nvflags &= ~val; break; case 0x40: /* RESETCTL */ + if (board_id(s) == BOARD_ID_VEXPRESS) { + /* reserved: RAZ/WI */ + break; + } if (s->lockval == LOCK_VALUE) { s->resetlevel = val; if (val & 0x100) @@ -216,7 +247,37 @@ static void arm_sysctl_write(void *opaque, target_phys_addr_t offset, case 0x98: /* OSCRESET3 */ case 0x9c: /* OSCRESET4 */ break; + case 0xa0: /* SYS_CFGDATA */ + if (board_id(s) != BOARD_ID_VEXPRESS) { + goto bad_reg; + } + s->sys_cfgdata = val; + return; + case 0xa4: /* SYS_CFGCTRL */ + if (board_id(s) != BOARD_ID_VEXPRESS) { + goto bad_reg; + } + s->sys_cfgctrl = val & ~(3 << 18); + s->sys_cfgstat = 1; /* complete */ + switch (s->sys_cfgctrl) { + case 0xc0800000: /* SYS_CFG_SHUTDOWN to motherboard */ + qemu_system_shutdown_request(); + break; + case 0xc0900000: /* SYS_CFG_REBOOT to motherboard */ + qemu_system_reset_request(); + break; + default: + s->sys_cfgstat |= 2; /* error */ + } + return; + case 0xa8: /* SYS_CFGSTAT */ + if (board_id(s) != BOARD_ID_VEXPRESS) { + goto bad_reg; + } + s->sys_cfgstat = val & 3; + return; default: + bad_reg: printf ("arm_sysctl_write: Bad register offset 0x%x\n", (int)offset); return; }
Add support for the Versatile Express SYS_CFG registers, which provide a generic means of reading or writing configuration information from various parts of the board. We only implement shutdown and reset. Also make the RESETCTL register RAZ/WI on Versatile Express rather than reset the board. Other system registers are generally the same as Versatile and Realview. This includes a VMState version number bump for arm_sysctl, since we have new register state to preserve. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/arm_sysctl.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 63 insertions(+), 2 deletions(-)