From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:46756) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RNm4u-0002Ql-Ac for qemu-devel@nongnu.org; Tue, 08 Nov 2011 08:51:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RNm4p-0005I2-AY for qemu-devel@nongnu.org; Tue, 08 Nov 2011 08:51:04 -0500 Received: from mail-gy0-f173.google.com ([209.85.160.173]:43401) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RNm4p-0005Hn-7m for qemu-devel@nongnu.org; Tue, 08 Nov 2011 08:50:59 -0500 Received: by gyb11 with SMTP id 11so611435gyb.4 for ; Tue, 08 Nov 2011 05:50:58 -0800 (PST) Message-ID: <4EB933BE.7080503@codemonkey.ws> Date: Tue, 08 Nov 2011 07:50:54 -0600 From: Anthony Liguori MIME-Version: 1.0 References: <1319540983-4248-1-git-send-email-benoit.canet@gmail.com> <1319540983-4248-5-git-send-email-benoit.canet@gmail.com> <4EB8CD52.1000008@redhat.com> <4EB91EDE.7070909@redhat.com> <4EB922DD.6040309@redhat.com> In-Reply-To: <4EB922DD.6040309@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Avi Kivity Cc: Peter Maydell , =?UTF-8?B?QmVub8OudCBDYW5ldA==?= , qemu-devel@nongnu.org, quintela@redhat.com 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 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 >