[Qemu-devel] Re: [PATCH 5/5] Port apic to new VMState design

[Juan Quintela <quintela@redhat.com>]

Reply-to: quintela@redhat.com
Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote:
> On Tue, Aug 18, 2009 at 04:41:42PM +0200, Juan Quintela wrote:
>> Reimar Döffinger <Reimar.Doeffinger@gmx.de> 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.