From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:49048) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ROAOh-0007zc-9I for qemu-devel@nongnu.org; Wed, 09 Nov 2011 10:49:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ROAOf-00024f-PT for qemu-devel@nongnu.org; Wed, 09 Nov 2011 10:49:07 -0500 Received: from mail-gy0-f173.google.com ([209.85.160.173]:41999) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ROAOf-00024S-7r for qemu-devel@nongnu.org; Wed, 09 Nov 2011 10:49:05 -0500 Received: by gyb11 with SMTP id 11so2071049gyb.4 for ; Wed, 09 Nov 2011 07:49:03 -0800 (PST) Message-ID: <4EBAA0EC.6030409@codemonkey.ws> Date: Wed, 09 Nov 2011 09:49:00 -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> <4EB933BE.7080503@codemonkey.ws> <4EB93ED9.6060105@redhat.com> <4EB944EE.9090304@codemonkey.ws> <4EB9477D.5010804@redhat.com> <4EB94B9F.5040102@codemonkey.ws> <4EB964AC.6000605@redhat.com> <4EBA90D8.8040707@codemonkey.ws> <4EBA96BD.5050904@redhat.com> In-Reply-To: <4EBA96BD.5050904@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/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