Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Peter Maydell]

On 8 November 2011 06:33, Avi Kivity <address@hidden> wrote:
> On 11/08/2011 04:07 AM, Peter Maydell wrote:
>> 2011/10/26 Peter Maydell <address@hidden>:
>> > On 25 October 2011 12:09, Benoît Canet <address@hidden> 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