[PATCH] qemu-traditional: Bump piix4acpi save-record version_id

[PATCH] qemu-traditional: Bump piix4acpi save-record version_id

[Andrew Cooper]

Sadly, because of a buggy attempt to revert c/s
ce3b7ce68426ea6249bb411f26b376d459c45450 (piix4acpi, xen: change in ACPI to
match the change in the BIOS) "for debugging purposes" which has remained
present in XenServer for several releases, an incompatibility in the Qemu save
record went unnoticed until now when I tried to clean up the patch queue.

The result is that save-records for XenServer 6.0 to 6.2 advertise a piix4acpi
record of version 2, but with the content of version 1 record (also corrupting
everything later in the stream, but as this is the last record and qemu doesn't
care about junk in the end of the record, this went completely unnoticed).

While this can of-course be fixed by us locally, reserving version_id 3
upstream is the only way to prevent further incompatibilities if/when the
piix4acpi object gets further development/bugfixes which require a change to
the save-record.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 hw/piix4acpi.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/piix4acpi.c b/hw/piix4acpi.c
index ddbe8e0..1e356f0 100644
--- a/hw/piix4acpi.c
+++ b/hw/piix4acpi.c
@@ -157,7 +157,7 @@ static int piix4acpi_load(QEMUFile *f, void *opaque, int version_id)
     int ret;
     uint32_t pm1a_evt_address_assigned;
 
-    if (version_id > 2)
+    if (version_id > 3)
         return -EINVAL;
     ret = pci_device_load(&s->dev, f);
     if (ret < 0)
@@ -778,7 +778,7 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
 #endif
     register_ioport_write(ACPI_DBG_IO_ADDR, 4, 1, acpi_dbg_writeb, d);
 
-    register_savevm("piix4acpi", 0, 2, piix4acpi_save, piix4acpi_load, d);
+    register_savevm("piix4acpi", 0, 3, piix4acpi_save, piix4acpi_load, d);
 
     return NULL;
 }
-- 
1.7.10.4

Re: [PATCH] qemu-traditional: Bump piix4acpi save-record version_id

[Ian Jackson]

Andrew Cooper writes ("[PATCH] qemu-traditional: Bump piix4acpi save-record version_id"):
> Sadly, because of a buggy attempt to revert c/s
> ce3b7ce68426ea6249bb411f26b376d459c45450 (piix4acpi, xen: change in
> ACPI to match the change in the BIOS) "for debugging purposes" which
> has remained present in XenServer for several releases, an
> incompatibility in the Qemu save record went unnoticed until now
> when I tried to clean up the patch queue.
> 
> The result is that save-records for XenServer 6.0 to 6.2 advertise a
> piix4acpi record of version 2, but with the content of version 1
> record (also corrupting everything later in the stream, but as this
> is the last record and qemu doesn't care about junk in the end of
> the record, this went completely unnoticed).
> 
> While this can of-course be fixed by us locally, reserving version_id 3
> upstream is the only way to prevent further incompatibilities if/when the
> piix4acpi object gets further development/bugfixes which require a change to
> the save-record.

Just to be clear, what you are saying is this: there is no bug in
upstream.  However, some versions of XenServer have a bug which means
that they generate broken savefiles with declared version 2 which are
not compatible with upstream's.  You intend to fix this in XenServer
by allocating a new savefile version 3 for your users.

You would like Xen.org's qemu-xen-traditional to also bump the
savefile version to 3 because that would avoid your continuing
divergence on this point from Xen.org's qemu-xen-traditional.

This is in principle allowable because we guarantee forward but not
backward migration compatibility.  However, it does have a cost:
without this change, it is possible that in practice reverse migration
or save/restore from 4.4 to 4.3 would work.  I haven't thought through
whether this is in fact possible at the moment.

It seems to me that your proposal is only useful if as a result (at
least) XenServer's "version 3" savefiles are compatible with Xen.org's
"version 3" qemu-xen-traditional savefiles.  What steps have you taken
to verify that this is the case ?  What do you think the usage
scenario of this compatibility would be - some kind of migration to or
from XenServer ?

An aside: I haven't ever tried a migration (or save/restore) from
qemu-xen-traditional to qemu-xen-upstream but I can't see it working
because the set of devices will be different.  So I think
savefile compatibility between qemu-xen-traditional and
qemu-xen-traditional is not a consideration.

Ian.

Re: [PATCH] qemu-traditional: Bump piix4acpi save-record version_id

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 4336 bytes --]

On 16/12/2013 14:47, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH] qemu-traditional: Bump piix4acpi save-record version_id"):
>> Sadly, because of a buggy attempt to revert c/s
>> ce3b7ce68426ea6249bb411f26b376d459c45450 (piix4acpi, xen: change in
>> ACPI to match the change in the BIOS) "for debugging purposes" which
>> has remained present in XenServer for several releases, an
>> incompatibility in the Qemu save record went unnoticed until now
>> when I tried to clean up the patch queue.
>>
>> The result is that save-records for XenServer 6.0 to 6.2 advertise a
>> piix4acpi record of version 2, but with the content of version 1
>> record (also corrupting everything later in the stream, but as this
>> is the last record and qemu doesn't care about junk in the end of
>> the record, this went completely unnoticed).
>>
>> While this can of-course be fixed by us locally, reserving version_id 3
>> upstream is the only way to prevent further incompatibilities if/when the
>> piix4acpi object gets further development/bugfixes which require a change to
>> the save-record.
> Just to be clear, what you are saying is this: there is no bug in
> upstream.

Correct

>   However, some versions of XenServer have a bug which means
> that they generate broken savefiles with declared version 2 which are
> not compatible with upstream's.

Correct

>   You intend to fix this in XenServer
> by allocating a new savefile version 3 for your users.

This is only the version of the piix4acpi object inside the stream, not
of the whole stream itself.

Upstream can consider version 2 and version 3 identical.  XenServer will
further have to declare version 2 to be version 1 for the purposes of
interpreting the stream.

As this is a pipe, and the difference in the stream is the presence/lack
of a single integer, I see no practical way to try and evaluate whether
it is a buggy v2 record without trusting the version_id.

If anyone has a clever solution that could prevent the need to bump the
version then I would love to hear it.  I just cant see a practical
alternative at the moment.

>
> You would like Xen.org's qemu-xen-traditional to also bump the
> savefile version to 3 because that would avoid your continuing
> divergence on this point from Xen.org's qemu-xen-traditional.

Correct

>
> This is in principle allowable because we guarantee forward but not
> backward migration compatibility.  However, it does have a cost:
> without this change, it is possible that in practice reverse migration
> or save/restore from 4.4 to 4.3 would work.  I haven't thought through
> whether this is in fact possible at the moment.

I hadn't considered this point.  The version compatibility statement
declares "Migrating down a version (e.g. from release /N+1/ down to
release /N/) is not normally supported."  I cant remember whether there
have been any change in Xen itself which would prevent this from working.

>
> It seems to me that your proposal is only useful if as a result (at
> least) XenServer's "version 3" savefiles are compatible with Xen.org's
> "version 3" qemu-xen-traditional savefiles.  What steps have you taken
> to verify that this is the case ?  What do you think the usage
> scenario of this compatibility would be - some kind of migration to or
> from XenServer ?

The main purpose of this is so someone doesn't come along with a new
improvement/bugfix and bumps the piix4acpi version from 2 to 3 upstream,
and break the hack I have to make XenServer compatible with its buggy
past self.

As far as validating, I have confirmed that we have no patches touching
piix4acpi_save, so it is the same as upstream.

I honestly don't know how well a VM would do being migrated between
XenServer and another qemu-traditional based toolstack.  I suspect we
have some incompatibilities elsewhere, but I am still wading through the
years and years detritus which has accumulated in our patch queue.

I will be doing my upmost to ensure that this doesn't occur when we move
to qemu-upstream.

~Andrew

>
> An aside: I haven't ever tried a migration (or save/restore) from
> qemu-xen-traditional to qemu-xen-upstream but I can't see it working
> because the set of devices will be different.  So I think
> savefile compatibility between qemu-xen-traditional and
> qemu-xen-traditional is not a consideration.
>
> Ian.


[-- Attachment #1.2: Type: text/html, Size: 5952 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] qemu-traditional: Bump piix4acpi save-record version_id

[Ian Jackson]

Andrew Cooper writes ("Re: [PATCH] qemu-traditional: Bump piix4acpi save-record version_id"):
> On 16/12/2013 14:47, Ian Jackson wrote:
>
>       You intend to fix this in XenServer
>     by allocating a new savefile version 3 for your users.
> 
> This is only the version of the piix4acpi object inside the stream,
> not of the whole stream itself.

Yes, that's what I meant; sorry for not being clear.

>     It seems to me that your proposal is only useful if as a result (at
>     least) XenServer's "version 3" savefiles are compatible with Xen.org's
>     "version 3" qemu-xen-traditional savefiles.  What steps have you taken
>     to verify that this is the case ?  What do you think the usage
>     scenario of this compatibility would be - some kind of migration to or
>     from XenServer ?
> 
> The main purpose of this is so someone doesn't come along with a new
> improvement/bugfix and bumps the piix4acpi version from 2 to 3 upstream, and
> break the hack I have to make XenServer compatible with its buggy past self.

I'm not sure I understand.  You're worried that if you apply this
version bump only to the XenServer version of piix4acpi, and we
(Xen.org) in the future change qemu-xen-traditional to assign version
3, the change that we make would "break" your change somehow when you
merged your change with ours ?

But surely when that occurred you'd get a textual merge conflict on
this piece of code, so you would notice.  Obviously you'd have to
decide what to do about it but the obvious answer would be to allocate
yourself save format 4 in the private numbering space which was (de
facto) created by the original XenServer bug.

And in practice I don't foresee us wanting to bump our own format any
time soon.  qemu-xen-traditional is in the deep freeze
maintenance-wise.

> I honestly don't know how well a VM would do being migrated between
> XenServer and another qemu-traditional based toolstack.  I suspect
> we have some incompatibilities elsewhere, but I am still wading
> through the years and years detritus which has accumulated in our
> patch queue.

If it is not intended to support migration of savefiles between
XenServer and Xen.org's qemu-xen-traditional, then I don't see why it
matters that XenServer has a private incompatible savefile format.

If you want to make this migration work then excellent and I'd be
happy to bump savefile version numbers for that.  But surely we'd want
to know that we weren't going to have to do it again (whether in this
driver or another part of qemu), which would involve doing some kind
of research or testing which AIUI you aren't anywhere near completing
yet.  Otherwise we'll find ourselves incurring the savefile version
bump compatibility cost on multiple occasions, for no benefit.

> I will be doing my upmost to ensure that this doesn't occur when we move to
> qemu-upstream.

That would be good :-).  Three of these incompatible savefile formats
is certainy enough ...

Thanks,
Ian.

Re: [PATCH] qemu-traditional: Bump piix4acpi save-record version_id

[Andrew Cooper]

On 16/12/2013 16:23, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH] qemu-traditional: Bump piix4acpi save-record version_id"):
>> On 16/12/2013 14:47, Ian Jackson wrote:
>>
>>       You intend to fix this in XenServer
>>     by allocating a new savefile version 3 for your users.
>>
>> This is only the version of the piix4acpi object inside the stream,
>> not of the whole stream itself.
> Yes, that's what I meant; sorry for not being clear.
>
>>     It seems to me that your proposal is only useful if as a result (at
>>     least) XenServer's "version 3" savefiles are compatible with Xen.org's
>>     "version 3" qemu-xen-traditional savefiles.  What steps have you taken
>>     to verify that this is the case ?  What do you think the usage
>>     scenario of this compatibility would be - some kind of migration to or
>>     from XenServer ?
>>
>> The main purpose of this is so someone doesn't come along with a new
>> improvement/bugfix and bumps the piix4acpi version from 2 to 3 upstream, and
>> break the hack I have to make XenServer compatible with its buggy past self.
> I'm not sure I understand.  You're worried that if you apply this
> version bump only to the XenServer version of piix4acpi, and we
> (Xen.org) in the future change qemu-xen-traditional to assign version
> 3, the change that we make would "break" your change somehow when you
> merged your change with ours ?
>
> But surely when that occurred you'd get a textual merge conflict on
> this piece of code, so you would notice.  Obviously you'd have to
> decide what to do about it but the obvious answer would be to allocate
> yourself save format 4 in the private numbering space which was (de
> facto) created by the original XenServer bug.
>
> And in practice I don't foresee us wanting to bump our own format any
> time soon.  qemu-xen-traditional is in the deep freeze
> maintenance-wise.

By "break", I mean "require some more cunning solution to the problem to
be found".  It will certainly not be subtle in terms of merge conflicts.

If upstream were to introduce a new v3 record with a different format,
XenServer's fix would have to automatically bump up to v4, and extend
the detection logic.  The amount of hacking would have to increase every
time upstream changed.

>
>> I honestly don't know how well a VM would do being migrated between
>> XenServer and another qemu-traditional based toolstack.  I suspect
>> we have some incompatibilities elsewhere, but I am still wading
>> through the years and years detritus which has accumulated in our
>> patch queue.
> If it is not intended to support migration of savefiles between
> XenServer and Xen.org's qemu-xen-traditional, then I don't see why it
> matters that XenServer has a private incompatible savefile format.

It matters, because of the maintenance effort of taking incremental
improvements from upstream.

I refer you to
https://github.com/xenserver/xen-4.3.pg/blob/master/xen-legacy-win-xenmapspace-quirks.patch
which shows the amount of hackary required when it is not possible to
take a preemtive step like this to prevent it breaking in the future.


I guess this all comes down to how likely the piix4acpi object version
id is to change in the future.  It is certainly possible for me to fix
this in XenServer only, and might be the better way if it is fairly
certain that none of this is going to change in qemu-traditional going
forward.

~Andrew

Re: [PATCH] qemu-traditional: Bump piix4acpi save-record version_id

[Ian Jackson]

Andrew Cooper writes ("Re: [PATCH] qemu-traditional: Bump piix4acpi save-record version_id"):
> On 16/12/2013 16:23, Ian Jackson wrote:
> > And in practice I don't foresee us wanting to bump our own format any
> > time soon.  qemu-xen-traditional is in the deep freeze
> > maintenance-wise.
> 
> By "break", I mean "require some more cunning solution to the problem to
> be found".  It will certainly not be subtle in terms of merge conflicts.
> 
> If upstream were to introduce a new v3 record with a different format,
> XenServer's fix would have to automatically bump up to v4, and extend
> the detection logic.  The amount of hacking would have to increase every
> time upstream changed.

I think it very unlikely that qemu-xen-traditional will grow an
entirely new format at this stage of its life.

But if it did, I think all of this would be a SMOP.  And not even a
particularly difficult one: the difference would be just in the
version numbers.

> I refer you to
> https://github.com/xenserver/xen-4.3.pg/blob/master/xen-legacy-win-xenmapspace-quirks.patch
> which shows the amount of hackary required when it is not possible to
> take a preemtive step like this to prevent it breaking in the future.

Firstly, a situation like that isn't going to arise I think.

Secondly: it is right that the XenServer project, not other users of
Xen, bears the cost of the technical debt incurred by previous
XenServer maintainers.

> I guess this all comes down to how likely the piix4acpi object version
> id is to change in the future.  It is certainly possible for me to fix
> this in XenServer only, and might be the better way if it is fairly
> certain that none of this is going to change in qemu-traditional going
> forward.

Frankly I think it unlikely that this code is going to be touched
again ever.  If it does it will be as part of an attempt to make it
compatible with XenServer.

Ian.