From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:46337) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RO9ik-0007OR-Gp for qemu-devel@nongnu.org; Wed, 09 Nov 2011 10:05:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RO9ie-00006O-Ce for qemu-devel@nongnu.org; Wed, 09 Nov 2011 10:05:46 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47588) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RO9id-000068-V0 for qemu-devel@nongnu.org; Wed, 09 Nov 2011 10:05:40 -0500 Message-ID: <4EBA96BD.5050904@redhat.com> Date: Wed, 09 Nov 2011 17:05:33 +0200 From: Avi Kivity 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> In-Reply-To: <4EBA90D8.8040707@codemonkey.ws> Content-Type: text/plain; charset=UTF-8 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: Anthony Liguori Cc: Peter Maydell , =?UTF-8?B?QmVub8OudCBDYW5ldA==?= , qemu-devel@nongnu.org, quintela@redhat.com 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) -- error compiling committee.c: too many arguments to function