From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MdQoH-0007gn-Ca for qemu-devel@nongnu.org; Tue, 18 Aug 2009 11:41:17 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MdQoC-0007ay-LH for qemu-devel@nongnu.org; Tue, 18 Aug 2009 11:41:17 -0400 Received: from [199.232.76.173] (port=44470 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MdQoC-0007aj-GC for qemu-devel@nongnu.org; Tue, 18 Aug 2009 11:41:12 -0400 Received: from mx2.redhat.com ([66.187.237.31]:43431) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MdQoC-00078U-06 for qemu-devel@nongnu.org; Tue, 18 Aug 2009 11:41:12 -0400 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n7IFfBvY011417 for ; Tue, 18 Aug 2009 11:41:11 -0400 From: Juan Quintela In-Reply-To: <20090818152112.GA5483@1und1.de> ("Reimar =?utf-8?Q?D=C3=B6ff?= =?utf-8?Q?inger=22's?= message of "Tue, 18 Aug 2009 17:21:12 +0200") Date: Tue, 18 Aug 2009 17:38:57 +0200 Message-ID: References: <20090818142405.GA16563@1und1.de> <20090818152112.GA5483@1und1.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] Re: [PATCH 5/5] Port apic to new VMState design List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Reply-to: quintela@redhat.com Reimar D=C3=B6ffinger wrote: > On Tue, Aug 18, 2009 at 04:41:42PM +0200, Juan Quintela wrote: >> Reimar D=C3=B6ffinger wrote: >> > Hello, >> > sorry for replying in the middle of the thread, I was to fast and >> > deleted the other mails already. >> > And just in case I mention I am new around here, so feel free to ignore >> > me if you feel I am completely wrong. >> > One thing I don't like too much about it is that you can't really hand= le >> > "calculated" fields. >>=20 >> Calculated fields are not by definition part of the state :) >> The state are the other fields that are used to save the state. I >> haven't yet seen calculade fields (but I haven't looked throughfully >> yet). With the current design, basically you can only save things that >> are in one struct (the way it is stored is an offset against a base >> address). > > I am not sure we 100% understand each other, so I maybe tell the > specific example. > I made a change to the eepro100 driver to fix dumping the network > statistics. > The main problem is, depending on which device you emulate, the size of > the statistics struct changes. > Since it looked ugly etc. I decided not to calculate the size of these > statistics each time but instead save it in the device state. > But instead of adding it to save_vm and load_vm (which also would change > the version) I just set that statistics size again according to which > device is emulated. This also assures that the emulated device and > the statistics size always fit together, even if someone fiddles with > the saved state. > The "problem" with your approach if I understand it right is that I > couldn't do that since the device never knows when it would have to > re-fill these fields. > Basically what I am asking is if you couldn't just add an optional > callback so some additional stuff can be done after the "basic" state > has been loaded - or if that isn't desired at least a callback that > allows verifying the loaded values and aborting execution. Ah, ok. I guess we are going to need that callbacks, one before we load state, and another one after we load it. Are your changes on upstream hw/eepro100.c? I can't see anything there that can't be done in a table approach. >> It is already that way. This design don't change anything. And I am >> not sure how to fix it. We don't have a "is this value safe for this >> field", around yet. It is possible to add some support for it, but I >> would like to 1st have an use case. > > Well, I meant nowadays it is just possible to add a check in load_vm and > fix any values that are off. While it is quite a bit of work there is > nothing in the API stopping you from doing it, you even can return > -EINVAL and hopefully the core will print some somewhat useful message. I guess we are going to have an optional callback to be called before/after loading the state. You should be able to put your verify there. >> > If nothing else, I'd at least add support for a "verify" function that >> > gets a "const state *" and can abort loading the VM in case someone >> > tries something evil (or can print some useful hint instead of having >> > qemu crash silently on the user, possibly at some later time). >>=20 >> This is as different problem that is not solved in qemu either. I agree >> that it would be nice to have such a function, but I am not sure that I >> know how to do it. and what is worse. if you can modify the image, you >> can always change anything in the middle of the RAM. I don't really see >> too much point trying to get a verify function for devices, when we >> can't have it for RAM. > > That is completely different from what I meant. > Changing the RAM compromises the VM and only the VM, an exploit in a > device emulation might allow to compromise the _host_. Is it now clearer > what I meant? yes, I see where you are meaning now. But I guess that one is needed to be solved, not only for migration. Not sure what to do about this. Later, Juan.