From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:47253) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ROCBi-0004DJ-Ds for qemu-devel@nongnu.org; Wed, 09 Nov 2011 12:43:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ROCBh-000120-88 for qemu-devel@nongnu.org; Wed, 09 Nov 2011 12:43:50 -0500 Received: from mail-gy0-f173.google.com ([209.85.160.173]:58160) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ROCBh-00011w-5c for qemu-devel@nongnu.org; Wed, 09 Nov 2011 12:43:49 -0500 Received: by gyb11 with SMTP id 11so2209969gyb.4 for ; Wed, 09 Nov 2011 09:43:48 -0800 (PST) Message-ID: <4EBABBD1.3000206@codemonkey.ws> Date: Wed, 09 Nov 2011 11:43:45 -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> <4EBAA0EC.6030409@codemonkey.ws> <4EBAA2AE.5070003@redhat.com> In-Reply-To: <4EBAA2AE.5070003@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: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