[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH 5/5] Port apic to new VMState design
From: |
Juan Quintela |
Subject: |
[Qemu-devel] Re: [PATCH 5/5] Port apic to new VMState design |
Date: |
Tue, 18 Aug 2009 17:38:57 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) |
Reply-to: address@hidden
Reimar Döffinger <address@hidden> wrote:
> On Tue, Aug 18, 2009 at 04:41:42PM +0200, Juan Quintela wrote:
>> Reimar Döffinger <address@hidden> 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 handle
>> > "calculated" fields.
>>
>> 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).
>>
>> 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.
Re: [Qemu-devel] [PATCH RFC 0/5] New VMState table based load/save infrastructure, Gerd Hoffmann, 2009/08/19