| Message ID | 1319540983-4248-5-git-send-email-benoit.canet@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 25 October 2011 12:09, Benoît Canet <benoit.canet@gmail.com> wrote: > +static const VMStateDescription vmstate_integratorcm = { > + .name = "integratorcm", > + .version_id = 1, > + .minimum_version_id = 1, > + .minimum_version_id_old = 1, > + .fields = (VMStateField[]) { > + VMSTATE_UINT32(memsz, integratorcm_state), > + VMSTATE_BOOL(flash_mapped, integratorcm_state), This raises a question. flash_mapped here is a flag which just tracks whether the associated MemoryRegion is currently mapped or unmapped. Do we need to do anything special to ensure that the MemoryRegion itself is reset to the correct mapped/unmapped state on restore? I recall discussing this kind of thing with Avi on IRC but I can't remember what the conclusion was. -- PMM
2011/10/26 Peter Maydell <peter.maydell@linaro.org>: > On 25 October 2011 12:09, Benoît Canet <benoit.canet@gmail.com> wrote: >> +static const VMStateDescription vmstate_integratorcm = { >> + .name = "integratorcm", >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .minimum_version_id_old = 1, >> + .fields = (VMStateField[]) { >> + VMSTATE_UINT32(memsz, integratorcm_state), >> + VMSTATE_BOOL(flash_mapped, integratorcm_state), > > This raises a question. flash_mapped here is a flag which just > tracks whether the associated MemoryRegion is currently mapped > or unmapped. Do we need to do anything special to ensure that > the MemoryRegion itself is reset to the correct mapped/unmapped > state on restore? > > I recall discussing this kind of thing with Avi on IRC but I > can't remember what the conclusion was. Avi, ping? I'm still interested in the answer to this question. thanks -- PMM
On 11/08/2011 04:07 AM, Peter Maydell wrote: > 2011/10/26 Peter Maydell <peter.maydell@linaro.org>: > > On 25 October 2011 12:09, Benoît Canet <benoit.canet@gmail.com> wrote: > >> +static const VMStateDescription vmstate_integratorcm = { > >> + .name = "integratorcm", > >> + .version_id = 1, > >> + .minimum_version_id = 1, > >> + .minimum_version_id_old = 1, > >> + .fields = (VMStateField[]) { > >> + VMSTATE_UINT32(memsz, integratorcm_state), > >> + VMSTATE_BOOL(flash_mapped, integratorcm_state), > > > > This raises a question. flash_mapped here is a flag which just > > tracks whether the associated MemoryRegion is currently mapped > > or unmapped. Do we need to do anything special to ensure that > > the MemoryRegion itself is reset to the correct mapped/unmapped > > state on restore? > > > > I recall discussing this kind of thing with Avi on IRC but I > > can't remember what the conclusion was. > > Avi, ping? I'm still interested in the answer to this question. Sorry, missed this. Yes, you need to ensure the memory region mapping reflects flash_mapped. btw, is flash_mapped a real device state (corresponds to a real memory bit on the device) or just an artefact? It's usually a bad idea to cast implementation artefacts in vmstate concrete.
On 8 November 2011 06:33, Avi Kivity <avi@redhat.com> wrote: > On 11/08/2011 04:07 AM, Peter Maydell wrote: >> 2011/10/26 Peter Maydell <peter.maydell@linaro.org>: >> > On 25 October 2011 12:09, Benoît Canet <benoit.canet@gmail.com> wrote: >> >> +static const VMStateDescription vmstate_integratorcm = { >> >> + .name = "integratorcm", >> >> + .version_id = 1, >> >> + .minimum_version_id = 1, >> >> + .minimum_version_id_old = 1, >> >> + .fields = (VMStateField[]) { >> >> + VMSTATE_UINT32(memsz, integratorcm_state), >> >> + VMSTATE_BOOL(flash_mapped, integratorcm_state), >> > >> > This raises a question. flash_mapped here is a flag which just >> > tracks whether the associated MemoryRegion is currently mapped >> > or unmapped. Do we need to do anything special to ensure that >> > the MemoryRegion itself is reset to the correct mapped/unmapped >> > state on restore? >> > >> > I recall discussing this kind of thing with Avi on IRC but I >> > can't remember what the conclusion was. >> >> Avi, ping? I'm still interested in the answer to this question. > > Sorry, missed this. Yes, you need to ensure the memory region mapping > reflects flash_mapped. This needs to be in the MemoryRegion core implementation, please. I don't want to have to redo it for every device that has a remappable region. In particular, at the moment memory.c's add/delete region functions will assert if the region was already added/deleted, which means the device has to keep track in a not-vm-saved state flag whether the memory was mapped so it can resync things on load (by comparing the non-saved flag and the saved-state). Ideally memory.c should just ensure that the memory hierarchy is saved and restored without devices having to do anything. > btw, is flash_mapped a real device state (corresponds to a real memory > bit on the device) or just an artefact? It's usually a bad idea to cast > implementation artefacts in vmstate concrete. It's an implementation artefact that you introduced when you did the memoryregion conversion :-) I have a half-a-patch to fix that lurking somewhere but it stalled because at the moment the non-saved flag is required in order to work around memory.c being unhelpful. -- PMM
On 11/08/2011 02:15 PM, Peter Maydell wrote: > >> > >> Avi, ping? I'm still interested in the answer to this question. > > > > Sorry, missed this. Yes, you need to ensure the memory region mapping > > reflects flash_mapped. > > This needs to be in the MemoryRegion core implementation, please. Right; see the memory/mutators branch. I plan to push this as soon as everything is converted. > I don't want to have to redo it for every device that has a > remappable region. In particular, at the moment memory.c's > add/delete region functions will assert if the region was already > added/deleted, which means the device has to keep track in a > not-vm-saved state flag whether the memory was mapped so it can > resync things on load (by comparing the non-saved flag and the > saved-state). > > Ideally memory.c should just ensure that the memory hierarchy > is saved and restored without devices having to do anything. That's a bit far-fetched - I don't want the memory core involved in save/restore. But if/when we integrate the memory core into QOM, then yes, that layer can take care of it. If we have an observable attribute (that fires off a callback when changed), we can link memory API properties into that attribute. > > btw, is flash_mapped a real device state (corresponds to a real memory > > bit on the device) or just an artefact? It's usually a bad idea to cast > > implementation artefacts in vmstate concrete. > > It's an implementation artefact that you introduced when you > did the memoryregion conversion :-) > > I have a half-a-patch to fix that lurking somewhere but it > stalled because at the moment the non-saved flag is required > in order to work around memory.c being unhelpful. Ideally we'd have a way to separate implementation state from real state (after working hard to eliminate implementation state).
On 8 November 2011 12:21, Avi Kivity <avi@redhat.com> wrote: > On 11/08/2011 02:15 PM, Peter Maydell wrote: >> This needs to be in the MemoryRegion core implementation, please. > > Right; see the memory/mutators branch. I plan to push this as soon as > everything is converted. OK, then this save/restore patch should wait until that has landed. >> Ideally memory.c should just ensure that the memory hierarchy >> is saved and restored without devices having to do anything. > > That's a bit far-fetched - I don't want the memory core involved in > save/restore. But if/when we integrate the memory core into QOM, then > yes, that layer can take care of it. If we have an observable attribute > (that fires off a callback when changed), we can link memory API > properties into that attribute. The memory hierarchy is a visible part of the state which can change as the guest does things -- it has to get saved and restored somehow. I think it would be better done in the core where it will always be done right, rather than relying on each device to handle it correctly (especially since it's not always obvious if it's been missed out from the device.) -- PMM
On 11/08/2011 02:30 PM, Peter Maydell wrote: > On 8 November 2011 12:21, Avi Kivity <avi@redhat.com> wrote: > > On 11/08/2011 02:15 PM, Peter Maydell wrote: > >> This needs to be in the MemoryRegion core implementation, please. > > > > Right; see the memory/mutators branch. I plan to push this as soon as > > everything is converted. > > OK, then this save/restore patch should wait until that has landed. Please don't add interdependencies, especially as I can't promise a firm date as to when everything is converted (however I'll take this opportunity to remind you that patches, especially for omap, are more welcome than I can possibly articulate). > >> Ideally memory.c should just ensure that the memory hierarchy > >> is saved and restored without devices having to do anything. > > > > That's a bit far-fetched - I don't want the memory core involved in > > save/restore. But if/when we integrate the memory core into QOM, then > > yes, that layer can take care of it. If we have an observable attribute > > (that fires off a callback when changed), we can link memory API > > properties into that attribute. > > The memory hierarchy is a visible part of the state which can > change as the guest does things -- it has to get saved and > restored somehow. I think it would be better done in the core > where it will always be done right, rather than relying on > each device to handle it correctly (especially since it's not > always obvious if it's been missed out from the device.) We agree, the only difference is in what "core" refers to. I don't want memory.c do become intermingled with everything else. It should be in a separate layer. Devices would then talk to this layer, and not to the gluing by themselves as they have to now. Or maybe memory.c will be layered on top of QOM, and then it can take care of everything. I really don't know QOM well enough to say. Copying Anthony for more insight.
On 8 November 2011 12:38, Avi Kivity <avi@redhat.com> wrote: > On 11/08/2011 02:30 PM, Peter Maydell wrote: >> On 8 November 2011 12:21, Avi Kivity <avi@redhat.com> wrote: >> > On 11/08/2011 02:15 PM, Peter Maydell wrote: >> >> This needs to be in the MemoryRegion core implementation, please. >> > >> > Right; see the memory/mutators branch. I plan to push this as soon as >> > everything is converted. >> >> OK, then this save/restore patch should wait until that has landed. > > Please don't add interdependencies, especially as I can't promise a firm > date as to when everything is converted (however I'll take this > opportunity to remind you that patches, especially for omap, are more > welcome than I can possibly articulate). I'm really not going to add workarounds for API deficiencies. Fortunately these devices (and omap_gpmc) don't have save/load already so nothing's going to be broken by that. -- PMM
On 11/08/2011 06:38 AM, Avi Kivity wrote: > On 11/08/2011 02:30 PM, Peter Maydell wrote: >> On 8 November 2011 12:21, Avi Kivity<avi@redhat.com> wrote: >>> On 11/08/2011 02:15 PM, Peter Maydell wrote: >>>> This needs to be in the MemoryRegion core implementation, please. >>> >>> Right; see the memory/mutators branch. I plan to push this as soon as >>> everything is converted. >> >> OK, then this save/restore patch should wait until that has landed. > > Please don't add interdependencies, especially as I can't promise a firm > date as to when everything is converted (however I'll take this > opportunity to remind you that patches, especially for omap, are more > welcome than I can possibly articulate). > >>>> Ideally memory.c should just ensure that the memory hierarchy >>>> is saved and restored without devices having to do anything. >>> >>> That's a bit far-fetched - I don't want the memory core involved in >>> save/restore. But if/when we integrate the memory core into QOM, then >>> yes, that layer can take care of it. If we have an observable attribute >>> (that fires off a callback when changed), we can link memory API >>> properties into that attribute. >> >> The memory hierarchy is a visible part of the state which can >> change as the guest does things -- it has to get saved and >> restored somehow. I think it would be better done in the core >> where it will always be done right, rather than relying on >> each device to handle it correctly (especially since it's not >> always obvious if it's been missed out from the device.) > > We agree, the only difference is in what "core" refers to. I don't want > memory.c do become intermingled with everything else. It should be in a > separate layer. Devices would then talk to this layer, and not to the > gluing by themselves as they have to now. > > Or maybe memory.c will be layered on top of QOM, and then it can take > care of everything. I really don't know QOM well enough to say. > Copying Anthony for more insight. QOM fixes all problems, but I don't think this has anything to do with QOM :-) We fundamentally should do save/restore using a rigorous, automatically generated mechanism where every field is save/restored[1]. That means we should have a VMSTATE_MEMORY_REGION(). VMSTATE_MEMORY_REGION should save off the state of the memory region, and restore it appropriately. VMSTATE_MEMORY_REGION's implementation does not need to live in memory.c. It can certainly live in savevm.c or somewhere else more appropriate. Where the device is mapped within the address space is no longer a property of the device, so restoring the mapping really should happen at whatever owns the address space (in this case sysbus). In this case, integratorcp is the one mapping things into its own address space so flash_mapped ought to be saved by integratorcp. [1] Deciding that a field doesn't need to be saved should be an exception. Regards, Anthony Liguori >
On 11/08/2011 03:50 PM, Anthony Liguori wrote: >> We agree, the only difference is in what "core" refers to. I don't want >> memory.c do become intermingled with everything else. It should be in a >> separate layer. Devices would then talk to this layer, and not to the >> gluing by themselves as they have to now. >> >> Or maybe memory.c will be layered on top of QOM, and then it can take >> care of everything. I really don't know QOM well enough to say. >> Copying Anthony for more insight. > > > QOM fixes all problems, but I don't think this has anything to do with > QOM :-) > > We fundamentally should do save/restore using a rigorous, > automatically generated mechanism where every field is save/restored[1]. "fundamentally", "rigrous", "automatically", "generated", "mechanism", "every", "field", and even "a" are all words that I associate with QOM. Am I misunderstanding anything? > That means we should have a VMSTATE_MEMORY_REGION(). > > VMSTATE_MEMORY_REGION should save off the state of the memory region, > and restore it appropriately. VMSTATE_MEMORY_REGION's implementation > does not need to live in memory.c. It can certainly live in savevm.c > or somewhere else more appropriate. What state is that? Some devices have fixed size, offset, parent, and enable/disable state (is there a word for that?), so there is no state that needs to be transferred. For other devices this is all dynamic. The way I see it, we create a link between some device state (a register) and a memory API field (like the offset). This way, when one changes, so does the other. In complicated devices we'll have to write a callback. > > Where the device is mapped within the address space is no longer a > property of the device, so restoring the mapping really should happen > at whatever owns the address space (in this case sysbus). > > In this case, integratorcp is the one mapping things into its own > address space so flash_mapped ought to be saved by integratorcp. flash_mapped always reflects a bit in a real register. We shouldn't duplicate state. > [1] Deciding that a field doesn't need to be saved should be an > exception. It should indicate that the field doesn't need to exist.
On 11/08/2011 08:38 AM, Avi Kivity wrote: > On 11/08/2011 03:50 PM, Anthony Liguori wrote: >>> We agree, the only difference is in what "core" refers to. I don't want >>> memory.c do become intermingled with everything else. It should be in a >>> separate layer. Devices would then talk to this layer, and not to the >>> gluing by themselves as they have to now. >>> >>> Or maybe memory.c will be layered on top of QOM, and then it can take >>> care of everything. I really don't know QOM well enough to say. >>> Copying Anthony for more insight. >> >> >> QOM fixes all problems, but I don't think this has anything to do with >> QOM :-) >> >> We fundamentally should do save/restore using a rigorous, >> automatically generated mechanism where every field is save/restored[1]. > > "fundamentally", "rigrous", "automatically", "generated", "mechanism", > "every", "field", and even "a" are all words that I associate with QOM. > Am I misunderstanding anything? There's no code generation in QOM :-) This just comes down to how we do save/restore. We white list things we care about. We should move to a model where we save/restore everything (preferably via code generation), and then black list/transform state before it goes over the wire. Mike Roth's migration Visitor series is a first step in this direction. The reason I bring this up in this context though is that using that mind set makes the answer about what to do here obvious. If it's a member of a device's state, it should be save/restored. MemoryRegion is a member of the device's state, so it should be save/restored with the device. >> That means we should have a VMSTATE_MEMORY_REGION(). >> >> VMSTATE_MEMORY_REGION should save off the state of the memory region, >> and restore it appropriately. VMSTATE_MEMORY_REGION's implementation >> does not need to live in memory.c. It can certainly live in savevm.c >> or somewhere else more appropriate. > > What state is that? Some devices have fixed size, offset, parent, and > enable/disable state (is there a word for that?), so there is no state > that needs to be transferred. For other devices this is all dynamic. Any mutable state should be save/restored. Immutable state doesn't need to be saved as it's created as part of the device model. If the question is, how do we restore the immutable state, that should be happening as part of device creation, no? > The way I see it, we create a link between some device state (a > register) and a memory API field (like the offset). This way, when one > changes, so does the other. In complicated devices we'll have to write > a callback. In devices where we dynamically change the offset (it's mutable), we should save the offset and restore it. Since offset is sometimes mutable and sometimes immutable, we should always save/restore it. In the cases where it's really immutable, since the value isn't changing, there's no harm in doing save/restore. Yes, we could save just the device register, and use a callback to regenerate the offset. But that adds complexity and leads to more save/restore bugs. We shouldn't be reluctant to save/restore derived state. Whether we send it over the wire is a different story. We should start by saving as much state as we need to, and then sit down and start removing state and adding callbacks as we need to. That way, we start with a strong statement of correctness as opposed to starting from a position of weak correctness. >> Where the device is mapped within the address space is no longer a >> property of the device, so restoring the mapping really should happen >> at whatever owns the address space (in this case sysbus). >> >> In this case, integratorcp is the one mapping things into its own >> address space so flash_mapped ought to be saved by integratorcp. > > flash_mapped always reflects a bit in a real register. We shouldn't > duplicate state. Why? The only thing that removing it does is create additional complexity for save/restore. You may argue that sending minimal state improves migration compatibility but I think the current state of save/restore is an existence proof that this line of reasoning is incorrect. Regards, Anthony Liguori > >> [1] Deciding that a field doesn't need to be saved should be an >> exception. > > It should indicate that the field doesn't need to exist. >
On 11/08/2011 05:04 PM, Anthony Liguori wrote: > > There's no code generation in QOM :-) > > This just comes down to how we do save/restore. We white list things > we care about. We should move to a model where we save/restore > everything (preferably via code generation), and then black > list/transform state before it goes over the wire. > > Mike Roth's migration Visitor series is a first step in this > direction. The reason I bring this up in this context though is that > using that mind set makes the answer about what to do here obvious. > If it's a member of a device's state, it should be save/restored. Ok. > > MemoryRegion is a member of the device's state, so it should be > save/restored with the device. Not all MemoryRegion fields are state. In some instantiations, none of them are. > >>> That means we should have a VMSTATE_MEMORY_REGION(). >>> >>> VMSTATE_MEMORY_REGION should save off the state of the memory region, >>> and restore it appropriately. VMSTATE_MEMORY_REGION's implementation >>> does not need to live in memory.c. It can certainly live in savevm.c >>> or somewhere else more appropriate. >> >> What state is that? Some devices have fixed size, offset, parent, and >> enable/disable state (is there a word for that?), so there is no state >> that needs to be transferred. For other devices this is all dynamic. > > Any mutable state should be save/restored. Immutable state doesn't > need to be saved as it's created as part of the device model. The memory API doesn't know which fields are mutable and which are not. > > If the question is, how do we restore the immutable state, that should > be happening as part of device creation, no? > >> The way I see it, we create a link between some device state (a >> register) and a memory API field (like the offset). This way, when one >> changes, so does the other. In complicated devices we'll have to write >> a callback. > > In devices where we dynamically change the offset (it's mutable), we > should save the offset and restore it. Since offset is sometimes > mutable and sometimes immutable, we should always save/restore it. In > the cases where it's really immutable, since the value isn't changing, > there's no harm in doing save/restore. There is, you're taking an implementation detail and making it into an ABI. Change the implementation and migration breaks. You can have a real region modeled as a set of nested regions, or as one big region (with a more complicated switch () statement in the callback). This shouldn't be reflected in the save/restore ABI. > > Yes, we could save just the device register, and use a callback to > regenerate the offset. But that adds complexity and leads to more > save/restore bugs. > > We shouldn't be reluctant to save/restore derived state. Whether we > send it over the wire is a different story. We should start by saving > as much state as we need to, and then sit down and start removing > state and adding callbacks as we need to. "saving state without sending it over the wire" is another way of saying "not saving state". > That way, we start with a strong statement of correctness as opposed > to starting from a position of weak correctness. We also start from a position of fragility wrt. implementation details. >> flash_mapped always reflects a bit in a real register. We shouldn't >> duplicate state. > > > Why? The only thing that removing it does is create additional > complexity for save/restore. You may argue that sending minimal state > improves migration compatibility but I think the current state of > save/restore is an existence proof that this line of reasoning is > incorrect. It doesn't create additional complexity for save restore, and I don't think that the current state of save/restore proves anything except that it needs a lot more work.
On 11/08/2011 09:15 AM, Avi Kivity wrote: > On 11/08/2011 05:04 PM, Anthony Liguori wrote: >>> What state is that? Some devices have fixed size, offset, parent, and >>> enable/disable state (is there a word for that?), so there is no state >>> that needs to be transferred. For other devices this is all dynamic. >> >> Any mutable state should be save/restored. Immutable state doesn't >> need to be saved as it's created as part of the device model. > > The memory API doesn't know which fields are mutable and which are not. Right, but sending immutable fields is just wasteful, it's not functionally incorrect. >> >> If the question is, how do we restore the immutable state, that should >> be happening as part of device creation, no? >> >>> The way I see it, we create a link between some device state (a >>> register) and a memory API field (like the offset). This way, when one >>> changes, so does the other. In complicated devices we'll have to write >>> a callback. >> >> In devices where we dynamically change the offset (it's mutable), we >> should save the offset and restore it. Since offset is sometimes >> mutable and sometimes immutable, we should always save/restore it. In >> the cases where it's really immutable, since the value isn't changing, >> there's no harm in doing save/restore. > > There is, you're taking an implementation detail and making it into an > ABI. Change the implementation and migration breaks. Yes, that's a feature, not a bug. If we send too little state today in version X, then discover this while working on version X + 1, we have no recourse. We have to black list version X. Discovering this is hard because we have to find a symptom of broken migration. This is often subtle like, "if you migrate while a floppy request is in flight, the request is lost resulting in a timeout in the guest kernel". If we send too much state (internal implementation that is derived from something else) in version X, then discover this while working on version X + 1, we can filter the incoming state in X + 1 to just ignore the extra state and derive the correct internal state from the other stable registers. Discovering cases like this is easy because migration fails directly--not indirectly through a functional regression. That means this is something we can very easily catch in regression testing. I actually think this is the way to do it too. Save/restore everything by default and then as we develop and discover migration breaks, add filtering in the new versions to ignore and not send internal state. I don't think there's a tremendous amount of value is proactively filtering internal state. A lot of internal state never changes over a long period of time. >> Yes, we could save just the device register, and use a callback to >> regenerate the offset. But that adds complexity and leads to more >> save/restore bugs. >> >> We shouldn't be reluctant to save/restore derived state. Whether we >> send it over the wire is a different story. We should start by saving >> as much state as we need to, and then sit down and start removing >> state and adding callbacks as we need to. > > "saving state without sending it over the wire" is another way of saying > "not saving state". Or filtering it on the receiving end. That's the fundamental difference. >> Why? The only thing that removing it does is create additional >> complexity for save/restore. You may argue that sending minimal state >> improves migration compatibility but I think the current state of >> save/restore is an existence proof that this line of reasoning is >> incorrect. > > It doesn't create additional complexity for save restore, and I don't > think that the current state of save/restore proves anything except that > it needs a lot more work. It's very hard to do the style of save/restore that we do correctly. Regards, Anthony Liguori
On 11/08/2011 05:32 PM, Anthony Liguori wrote: > >>> >>> If the question is, how do we restore the immutable state, that should >>> be happening as part of device creation, no? >>> >>>> The way I see it, we create a link between some device state (a >>>> register) and a memory API field (like the offset). This way, when >>>> one >>>> changes, so does the other. In complicated devices we'll have to >>>> write >>>> a callback. >>> >>> In devices where we dynamically change the offset (it's mutable), we >>> should save the offset and restore it. Since offset is sometimes >>> mutable and sometimes immutable, we should always save/restore it. In >>> the cases where it's really immutable, since the value isn't changing, >>> there's no harm in doing save/restore. >> >> There is, you're taking an implementation detail and making it into an >> ABI. Change the implementation and migration breaks. > > Yes, that's a feature, not a bug. If we send too little state today > in version X, then discover this while working on version X + 1, we > have no recourse. We have to black list version X. > > Discovering this is hard because we have to find a symptom of broken > migration. This is often subtle like, "if you migrate while a floppy > request is in flight, the request is lost resulting in a timeout in > the guest kernel". > > If we send too much state (internal implementation that is derived > from something else) in version X, then discover this while working on > version X + 1, we can filter the incoming state in X + 1 to just > ignore the extra state and derive the correct internal state from the > other stable registers. > > Discovering cases like this is easy because migration fails > directly--not indirectly through a functional regression. That means > this is something we can very easily catch in regression testing. > > I actually think this is the way to do it too. Save/restore > everything by default and then as we develop and discover migration > breaks, add filtering in the new versions to ignore and not send > internal state. I don't think there's a tremendous amount of value is > proactively filtering internal state. A lot of internal state never > changes over a long period of time. I might agree if a significant fraction of the memory API's state needed to be saved. But that's not the case -- indeed I expect it to be zero. Take this patch for example, the only field that is mutable is the enabled/disabled state, which mirrors some bit in a register. PIIX's PAM, PCI's BARs are the same. I doubt there is *any* case where the memory API is the sole source of this information. The way we do this now is to call device_update_mappings() whenever a register that contains mapping information changes, whether it is in a device_write() callback or in device_post_load(). All that you'd save with automatic memory API state migration is the latter call. > >>> Yes, we could save just the device register, and use a callback to >>> regenerate the offset. But that adds complexity and leads to more >>> save/restore bugs. >>> >>> We shouldn't be reluctant to save/restore derived state. Whether we >>> send it over the wire is a different story. We should start by saving >>> as much state as we need to, and then sit down and start removing >>> state and adding callbacks as we need to. >> >> "saving state without sending it over the wire" is another way of saying >> "not saving state". > > Or filtering it on the receiving end. That's the fundamental difference. I might agree if I thought there is anything worthwhile in the memory API's state. > >>> Why? The only thing that removing it does is create additional >>> complexity for save/restore. You may argue that sending minimal state >>> improves migration compatibility but I think the current state of >>> save/restore is an existence proof that this line of reasoning is >>> incorrect. >> >> It doesn't create additional complexity for save restore, and I don't >> think that the current state of save/restore proves anything except that >> it needs a lot more work. > > It's very hard to do the style of save/restore that we do correctly. If we had a Register class, that would take care of device registers automatically. As to in flight transactions or hidden state, I don't think there are any shortcuts.
On 11/08/2011 11:19 AM, Avi Kivity wrote: > On 11/08/2011 05:32 PM, Anthony Liguori wrote: >> >>>> >>>> If the question is, how do we restore the immutable state, that should >>>> be happening as part of device creation, no? >>>> >>>>> The way I see it, we create a link between some device state (a >>>>> register) and a memory API field (like the offset). This way, when >>>>> one >>>>> changes, so does the other. In complicated devices we'll have to >>>>> write >>>>> a callback. >>>> >>>> In devices where we dynamically change the offset (it's mutable), we >>>> should save the offset and restore it. Since offset is sometimes >>>> mutable and sometimes immutable, we should always save/restore it. In >>>> the cases where it's really immutable, since the value isn't changing, >>>> there's no harm in doing save/restore. >>> >>> There is, you're taking an implementation detail and making it into an >>> ABI. Change the implementation and migration breaks. >> >> Yes, that's a feature, not a bug. If we send too little state today >> in version X, then discover this while working on version X + 1, we >> have no recourse. We have to black list version X. >> >> Discovering this is hard because we have to find a symptom of broken >> migration. This is often subtle like, "if you migrate while a floppy >> request is in flight, the request is lost resulting in a timeout in >> the guest kernel". >> >> If we send too much state (internal implementation that is derived >> from something else) in version X, then discover this while working on >> version X + 1, we can filter the incoming state in X + 1 to just >> ignore the extra state and derive the correct internal state from the >> other stable registers. >> >> Discovering cases like this is easy because migration fails >> directly--not indirectly through a functional regression. That means >> this is something we can very easily catch in regression testing. >> >> I actually think this is the way to do it too. Save/restore >> everything by default and then as we develop and discover migration >> breaks, add filtering in the new versions to ignore and not send >> internal state. I don't think there's a tremendous amount of value is >> proactively filtering internal state. A lot of internal state never >> changes over a long period of time. > > I might agree if a significant fraction of the memory API's state needed > to be saved. But that's not the case -- indeed I expect it to be zero. > > Take this patch for example, the only field that is mutable is the > enabled/disabled state, which mirrors some bit in a register. PIIX's > PAM, PCI's BARs are the same. I doubt there is *any* case where the > memory API is the sole source of this information. > > The way we do this now is to call device_update_mappings() whenever a > register that contains mapping information changes, whether it is in a > device_write() callback or in device_post_load(). All that you'd save > with automatic memory API state migration is the latter call. So we have: typedef struct { SysBusDevice busdev; uint32_t memsz; MemoryRegion flash; bool flash_mapped; uint32_t cm_osc; uint32_t cm_ctrl; uint32_t cm_lock; uint32_t cm_auxosc; uint32_t cm_sdram; uint32_t cm_init; uint32_t cm_flags; uint32_t cm_nvflags; uint32_t int_level; uint32_t irq_enabled; uint32_t fiq_enabled; } integratorcm_state; What I'm saying is let's do: VMSTATE_SYSBUS(integratorcm_state, busdev), VMSTATE_UINT32(integratorcm, memsz), VMSTATE_MEMORY_REGION(integratorcm, flash), VMSTATE_BOOL(integratorcm, flash_mapped), VMSTATE_UINT32(integratorcm, cm_osc), VMSTATE_UINT32(integratorcm, cm_ctrl), VMSTATE_UINT32(integratorcm, cm_lock), VMSTATE_UINT32(integratorcm, cm_auxosc), VMSTATE_UINT32(integratorcm, cm_sdram), VMSTATE_UINT32(integratorcm, cm_init), VMSTATE_UINT32(integratorcm, cm_flags), VMSTATE_UINT32(integratorcm, cm_nvflags), VMSTATE_UINT32(integratorcm, int_level), VMSTATE_UINT32(integratorcm, irq_enabled), VMSTATE_UINT32(integratorcm, fiq_enabled), As there's a 1-1 mapping here. You can agree, that this is functionally correct. But flash_mapped is derived state and the contents of flash are almost entirely immutable so we don't strictly need to send it. Okay, then let's add something to vmstate to suppress fields. It could be as simple as: struct VMStateDescription { + const char *derived_fields[]; const char *name; This gives us a few things. First, it means we're describing how to marshal everything which I really believe is the direction we need to go. Second, it makes writing VMState descriptions easier to review. Every field should be in the VMState description. Any field that is in the derived_fields array should have its value set in the post_load function. You could also have an immutable_fields array to indicate which fields are immutable. Since VMSTATE_MEMORY_REGION is probably just going to point to a substructure, you could mark all of the fields of the memory region as immutable except for enabled and mark that derived. This would also let us to do things like systematically make sure that when we're listing derived fields, we validate that we have a post_load function. >>>> Yes, we could save just the device register, and use a callback to >>>> regenerate the offset. But that adds complexity and leads to more >>>> save/restore bugs. >>>> >>>> We shouldn't be reluctant to save/restore derived state. Whether we >>>> send it over the wire is a different story. We should start by saving >>>> as much state as we need to, and then sit down and start removing >>>> state and adding callbacks as we need to. >>> >>> "saving state without sending it over the wire" is another way of saying >>> "not saving state". >> >> Or filtering it on the receiving end. That's the fundamental difference. > > I might agree if I thought there is anything worthwhile in the memory > API's state. > >> >>>> Why? The only thing that removing it does is create additional >>>> complexity for save/restore. You may argue that sending minimal state >>>> improves migration compatibility but I think the current state of >>>> save/restore is an existence proof that this line of reasoning is >>>> incorrect. >>> >>> It doesn't create additional complexity for save restore, and I don't >>> think that the current state of save/restore proves anything except that >>> it needs a lot more work. >> >> It's very hard to do the style of save/restore that we do correctly. > > If we had a Register class, that would take care of device registers > automatically. As to in flight transactions or hidden state, I don't > think there are any shortcuts. BTW, I've thought about this in the past but never came up with anything that really made sense. Have you thought about what what a Register class would do? Regards, Anthony Liguori
On 11/09/2011 04:40 PM, Anthony Liguori wrote: > > typedef struct { > SysBusDevice busdev; > uint32_t memsz; > MemoryRegion flash; > bool flash_mapped; Both flash.has_parent and flash_mapped are slaved to a bit of one of the registers below. > uint32_t cm_osc; > uint32_t cm_ctrl; > uint32_t cm_lock; > uint32_t cm_auxosc; > uint32_t cm_sdram; > uint32_t cm_init; > uint32_t cm_flags; > uint32_t cm_nvflags; > uint32_t int_level; > uint32_t irq_enabled; > uint32_t fiq_enabled; > } integratorcm_state; > > What I'm saying is let's do: > > VMSTATE_SYSBUS(integratorcm_state, busdev), > VMSTATE_UINT32(integratorcm, memsz), > VMSTATE_MEMORY_REGION(integratorcm, flash), Therefore this line is 100% redundant. > VMSTATE_BOOL(integratorcm, flash_mapped), > VMSTATE_UINT32(integratorcm, cm_osc), > VMSTATE_UINT32(integratorcm, cm_ctrl), > VMSTATE_UINT32(integratorcm, cm_lock), > VMSTATE_UINT32(integratorcm, cm_auxosc), > VMSTATE_UINT32(integratorcm, cm_sdram), > VMSTATE_UINT32(integratorcm, cm_init), > VMSTATE_UINT32(integratorcm, cm_flags), > VMSTATE_UINT32(integratorcm, cm_nvflags), > VMSTATE_UINT32(integratorcm, int_level), > VMSTATE_UINT32(integratorcm, irq_enabled), > VMSTATE_UINT32(integratorcm, fiq_enabled), > > As there's a 1-1 mapping here. > > You can agree, that this is functionally correct. But flash_mapped is > derived state and the contents of flash are almost entirely immutable > so we don't strictly need to send it. > > Okay, then let's add something to vmstate to suppress fields. It > could be as simple as: > > struct VMStateDescription { > + const char *derived_fields[]; > const char *name; > > This gives us a few things. First, it means we're describing how to > marshal everything which I really believe is the direction we need to > go. Second, it makes writing VMState descriptions easier to review. > Every field should be in the VMState description. Any field that is > in the derived_fields array should have its value set in the post_load > function. You could also have an immutable_fields array to indicate > which fields are immutable. 100% of the memory API's fields are either immutable or derived. > > Since VMSTATE_MEMORY_REGION is probably just going to point to a > substructure, you could mark all of the fields of the memory region as > immutable except for enabled and mark that derived. This would also > let us to do things like systematically make sure that when we're > listing derived fields, we validate that we have a post_load function. > If a post_load is missing, it's just a bug, not missing state, so it's not a catastrophic bug. The issues are save-side. >> If we had a Register class, that would take care of device registers >> automatically. As to in flight transactions or hidden state, I don't >> think there are any shortcuts. > > BTW, I've thought about this in the past but never came up with > anything that really made sense. Have you thought about what what a > Register class would do? > name (for the monitor) size ptr to storage (in device state) writeable bits mask clear-on-read mask read function (if computed on demand; otherwise satisfied from storage) write function (if have side effects)
On 9 November 2011 15:05, Avi Kivity <avi@redhat.com> wrote: > On 11/09/2011 04:40 PM, Anthony Liguori wrote: >> >> typedef struct { >> SysBusDevice busdev; >> uint32_t memsz; >> MemoryRegion flash; >> bool flash_mapped; > > Both flash.has_parent and flash_mapped are slaved to a bit of one of the > registers below. Yes. The only reason for having a separate flash_mapped would be if it was *not* saved/loaded and we needed it to track whether the memory region API thought the region was mapped or not (ie whether calling memory_region_add_subregion_overlap() from the post-load-hook would assert, which is what it does in master currently.) This is why I'm pushing back on Benoit's patch here -- I don't want us to carry around this extra flash_mapped flag unless we really need it. From a really quick glance at Avi's memory/mutators branch I think the long term plan would be that rather than mapping/unmapping the region we just call memory_region_set_enabled() which would cope happily with being asked to enable an already-enabled region. Is that right? -- PMM
On 11/09/2011 05:20 PM, Peter Maydell wrote: > From a really quick glance at Avi's memory/mutators branch I think > the long term plan would be that rather than mapping/unmapping the > region we just call memory_region_set_enabled() which would cope > happily with being asked to enable an already-enabled region. > Is that right? It is. Call it from both the _write() callback that sets the register, and from post_load(), and you're done.
On 11/09/2011 09:05 AM, Avi Kivity wrote: > On 11/09/2011 04:40 PM, Anthony Liguori wrote: >> >> typedef struct { >> SysBusDevice busdev; >> uint32_t memsz; >> MemoryRegion flash; >> bool flash_mapped; > > Both flash.has_parent and flash_mapped are slaved to a bit of one of the > registers below. > >> uint32_t cm_osc; >> uint32_t cm_ctrl; >> uint32_t cm_lock; >> uint32_t cm_auxosc; >> uint32_t cm_sdram; >> uint32_t cm_init; >> uint32_t cm_flags; >> uint32_t cm_nvflags; >> uint32_t int_level; >> uint32_t irq_enabled; >> uint32_t fiq_enabled; >> } integratorcm_state; >> >> What I'm saying is let's do: >> >> VMSTATE_SYSBUS(integratorcm_state, busdev), >> VMSTATE_UINT32(integratorcm, memsz), >> VMSTATE_MEMORY_REGION(integratorcm, flash), > > Therefore this line is 100% redundant. Yes, but the problem is that it's not obvious *why*. That's what I'm trying to get at here. If you have a VMSTATE_MEMORY_REGION() that has all of it's fields marked immutable and one field marked derived, now it becomes obvious *why* we don't save these fields. Just not having it in the vmstate description makes it very non-obvious. Is it a bug? Is there some field in memory region that I'm responsible for setting in a post load hook? >> This gives us a few things. First, it means we're describing how to >> marshal everything which I really believe is the direction we need to >> go. Second, it makes writing VMState descriptions easier to review. >> Every field should be in the VMState description. Any field that is >> in the derived_fields array should have its value set in the post_load >> function. You could also have an immutable_fields array to indicate >> which fields are immutable. > > 100% of the memory API's fields are either immutable or derived. Ok, let's at least make the code make it obvious that that is the case. >> BTW, I've thought about this in the past but never came up with >> anything that really made sense. Have you thought about what what a >> Register class would do? >> > > name (for the monitor) > size > ptr to storage (in device state) > writeable bits mask > clear-on-read mask Really? Is that all that common outside of PCI config? > read function (if computed on demand; otherwise satisfied from storage) > write function (if have side effects) I tried something like this in Python at one point and the code ended up very big to write a device model. It's hard to beat the conciseness of the dispatch functions with a switch() statement. Regards, Anthony Liguori
On 11/09/2011 05:49 PM, Anthony Liguori wrote: > >>> VMSTATE_MEMORY_REGION(integratorcm, flash), >> >> Therefore this line is 100% redundant. > > > Yes, but the problem is that it's not obvious *why*. That's what I'm > trying to get at here. If you have a VMSTATE_MEMORY_REGION() that has > all of it's fields marked immutable and one field marked derived, now > it becomes obvious *why* we don't save these fields. Every MemoryRegion field in qemu today is either immutable or slaved to another register. We could have a system to annotate every field, but it's pointless. If we had a device that set the region offset to some value it computes at runtime that is not derived from state (say, offset = count of writes to some register) then there would be some point in it. But we don't, so there isn't. > Just not having it in the vmstate description makes it very > non-obvious. Is it a bug? Is there some field in memory region that > I'm responsible for setting in a post load hook? Missing post-load hook bugs are not destructive. Of course we should try to avoid them, but a markup system that we know ends up doing nothing is excessive. > >>> This gives us a few things. First, it means we're describing how to >>> marshal everything which I really believe is the direction we need to >>> go. Second, it makes writing VMState descriptions easier to review. >>> Every field should be in the VMState description. Any field that is >>> in the derived_fields array should have its value set in the post_load >>> function. You could also have an immutable_fields array to indicate >>> which fields are immutable. >> >> 100% of the memory API's fields are either immutable or derived. > > Ok, let's at least make the code make it obvious that that is the case. The memory/mutators branch simplifies it by eliminating pseudo state like flash_mapped. > >>> BTW, I've thought about this in the past but never came up with >>> anything that really made sense. Have you thought about what what a >>> Register class would do? >>> >> >> name (for the monitor) >> size >> ptr to storage (in device state) >> writeable bits mask >> clear-on-read mask > > Really? Is that all that common outside of PCI config? Yes, ISR fields often have it (like virtio). > >> read function (if computed on demand; otherwise satisfied from storage) >> write function (if have side effects) > > I tried something like this in Python at one point and the code ended > up very big to write a device model. It's hard to beat the > conciseness of the dispatch functions with a switch() statement. This style of code really wants lambdas. Without them, we have 4-5 lines of boilerplate for each callback. Even then, it's worthwhile IMO (and many callbacks can be avoided, both read and write, or merged into a device_update_mapping or device_update_irq read-all-state style functions).
On 9 November 2011 15:56, Avi Kivity <avi@redhat.com> wrote: > On 11/09/2011 05:49 PM, Anthony Liguori wrote: >> Avi wrote: >>> clear-on-read mask >> >> Really? Is that all that common outside of PCI config? > > Yes, ISR fields often have it (like virtio). Write-one-to-clear is pretty popular too. -- PMM
On 11/09/2011 09:56 AM, Avi Kivity wrote: > On 11/09/2011 05:49 PM, Anthony Liguori wrote: >> >>>> VMSTATE_MEMORY_REGION(integratorcm, flash), >>> >>> Therefore this line is 100% redundant. >> >> >> Yes, but the problem is that it's not obvious *why*. That's what I'm >> trying to get at here. If you have a VMSTATE_MEMORY_REGION() that has >> all of it's fields marked immutable and one field marked derived, now >> it becomes obvious *why* we don't save these fields. > > Every MemoryRegion field in qemu today is either immutable or slaved to > another register. We could have a system to annotate every field, but > it's pointless. If I'm writing a device and doing save/restore and I happen to use a MemoryRegion, how do I determine that every field is either immutable or slaved? > If we had a device that set the region offset to some value it computes > at runtime that is not derived from state (say, offset = count of writes > to some register) then there would be some point in it. But we don't, > so there isn't. > >> Just not having it in the vmstate description makes it very >> non-obvious. Is it a bug? Is there some field in memory region that >> I'm responsible for setting in a post load hook? > > Missing post-load hook bugs are not destructive. Of course we should > try to avoid them, but a markup system that we know ends up doing > nothing is excessive. > >> >>>> This gives us a few things. First, it means we're describing how to >>>> marshal everything which I really believe is the direction we need to >>>> go. Second, it makes writing VMState descriptions easier to review. >>>> Every field should be in the VMState description. Any field that is >>>> in the derived_fields array should have its value set in the post_load >>>> function. You could also have an immutable_fields array to indicate >>>> which fields are immutable. >>> >>> 100% of the memory API's fields are either immutable or derived. >> >> Ok, let's at least make the code make it obvious that that is the case. > > The memory/mutators branch simplifies it by eliminating pseudo state > like flash_mapped. They just moved the derived state into the MemoryRegion, no? >>>> BTW, I've thought about this in the past but never came up with >>>> anything that really made sense. Have you thought about what what a >>>> Register class would do? >>>> >>> >>> name (for the monitor) >>> size >>> ptr to storage (in device state) >>> writeable bits mask >>> clear-on-read mask >> >> Really? Is that all that common outside of PCI config? > > Yes, ISR fields often have it (like virtio). Yes, but virtio-pci was a very special case to avoid taking an extra exit. Do you know of any other than virtio-pci? All the ones I can think of (RTC, Serial, etc.) are cleared with a write. >>> read function (if computed on demand; otherwise satisfied from storage) >>> write function (if have side effects) >> >> I tried something like this in Python at one point and the code ended >> up very big to write a device model. It's hard to beat the >> conciseness of the dispatch functions with a switch() statement. > > This style of code really wants lambdas. Without them, we have 4-5 > lines of boilerplate for each callback. Even then, it's worthwhile IMO > (and many callbacks can be avoided, both read and write, or merged into > a device_update_mapping or device_update_irq read-all-state style > functions). Yeah, I looked at this but wasn't happy with the results. In practice, many devices end up implementing non-trivial logic when register values change. What I was really interested in was coming up with a way to get really high quality tracing of device register accesses. Regards, Anthony Liguori
On 11/09/2011 07:43 PM, Anthony Liguori wrote: >> Every MemoryRegion field in qemu today is either immutable or slaved to >> another register. We could have a system to annotate every field, but >> it's pointless. > > > If I'm writing a device and doing save/restore and I happen to use a > MemoryRegion, how do I determine that every field is either immutable > or slaved? In general 'return true' should work. I have a hard time imagining a device where this doesn't hold. If all memory API functions are called with parameters that are functions of the state and only the state, you're good. >> The memory/mutators branch simplifies it by eliminating pseudo state >> like flash_mapped. > > > They just moved the derived state into the MemoryRegion, no? > They do not. We had this state in three places. memory/mutators folds ->flash_mapped and the MemoryRegion equivalent; they both still mirror the real device register. If we had an Observable interface for Registers, then we could make any write to the Register automatically update the MemoryRegion; as it is, we have to call device_update_mapping() after every write. >> Yes, ISR fields often have it (like virtio). > > Yes, but virtio-pci was a very special case to avoid taking an extra > exit. > > Do you know of any other than virtio-pci? All the ones I can think of > (RTC, Serial, etc.) are cleared with a write. Can't think of any offhand, but see http://verificationguild.com/modules.php?name=Forums&file=viewtopic&p=5724. Anyway, if something turns out not to be useful, we don't have to keep it in the core. >> This style of code really wants lambdas. Without them, we have 4-5 >> lines of boilerplate for each callback. Even then, it's worthwhile IMO >> (and many callbacks can be avoided, both read and write, or merged into >> a device_update_mapping or device_update_irq read-all-state style >> functions). > > > Yeah, I looked at this but wasn't happy with the results. In > practice, many devices end up implementing non-trivial logic when > register values change. > > What I was really interested in was coming up with a way to get really > high quality tracing of device register accesses. A Register can still dispatch to a common dispatch function. Another thing I'm thinking of is wrapping addr/size/value in a Transaction object, to keep the signatures trim.
diff --git a/hw/integratorcp.c b/hw/integratorcp.c index 9a289b4..e8d8d67 100644 --- a/hw/integratorcp.c +++ b/hw/integratorcp.c @@ -34,6 +34,29 @@ typedef struct { uint32_t fiq_enabled; } integratorcm_state; +static const VMStateDescription vmstate_integratorcm = { + .name = "integratorcm", + .version_id = 1, + .minimum_version_id = 1, + .minimum_version_id_old = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT32(memsz, integratorcm_state), + VMSTATE_BOOL(flash_mapped, integratorcm_state), + VMSTATE_UINT32(cm_osc, integratorcm_state), + VMSTATE_UINT32(cm_ctrl, integratorcm_state), + VMSTATE_UINT32(cm_lock, integratorcm_state), + VMSTATE_UINT32(cm_auxosc, integratorcm_state), + VMSTATE_UINT32(cm_sdram, integratorcm_state), + VMSTATE_UINT32(cm_init, integratorcm_state), + VMSTATE_UINT32(cm_flags, integratorcm_state), + VMSTATE_UINT32(cm_nvflags, integratorcm_state), + VMSTATE_UINT32(int_level, integratorcm_state), + VMSTATE_UINT32(irq_enabled, integratorcm_state), + VMSTATE_UINT32(fiq_enabled, integratorcm_state), + VMSTATE_END_OF_LIST() + } +}; + static uint8_t integrator_spd[128] = { 128, 8, 4, 11, 9, 1, 64, 0, 2, 0xa0, 0xa0, 0, 0, 8, 0, 1, 0xe, 4, 0x1c, 1, 2, 0x20, 0xc0, 0, 0, 0, 0, 0x30, 0x28, 0x30, 0x28, 0x40 @@ -547,6 +570,7 @@ static SysBusDeviceInfo core_info = { .init = integratorcm_init, .qdev.name = "integrator_core", .qdev.size = sizeof(integratorcm_state), + .qdev.vmsd = &vmstate_integratorcm, .qdev.props = (Property[]) { DEFINE_PROP_UINT32("memsz", integratorcm_state, memsz, 0), DEFINE_PROP_END_OF_LIST(),
Signed-off-by: Benoit Canet <benoit.canet@gmail.com> --- hw/integratorcp.c | 24 ++++++++++++++++++++++++ 1 files changed, 24 insertions(+), 0 deletions(-)