[Qemu-devel] [PATCH for-2.0] target-i386: reorder fields in cpu/msr_hyperv_hypercall subsection

[Qemu-devel] [PATCH for-2.0] target-i386: reorder fields in cpu/msr_hyperv_hypercall subsection

[Paolo Bonzini]

The subsection already exists in one well-known enterprise Linux
distribution, but for some strange reason the fields were swapped
when forward-porting the patch to upstream.

Limit headaches for said enterprise Linux distributor when the
time will come to rebase their version of QEMU.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/machine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-i386/machine.c b/target-i386/machine.c
index 24bc373..168cab6 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -569,8 +569,8 @@ static const VMStateDescription vmstate_msr_hypercall_hypercall = {
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
     .fields      = (VMStateField []) {
-        VMSTATE_UINT64(env.msr_hv_hypercall, X86CPU),
         VMSTATE_UINT64(env.msr_hv_guest_os_id, X86CPU),
+        VMSTATE_UINT64(env.msr_hv_hypercall, X86CPU),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.9.0

Re: [Qemu-devel] [PATCH for-2.0] target-i386: reorder fields in cpu/msr_hyperv_hypercall subsection

[Michael S. Tsirkin]

On Wed, Apr 02, 2014 at 05:33:02PM +0200, Paolo Bonzini wrote:
> The subsection already exists in one well-known enterprise Linux
> distribution, but for some strange reason the fields were swapped
> when forward-porting the patch to upstream.
> 
> Limit headaches for said enterprise Linux distributor when the
> time will come to rebase their version of QEMU.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Peter could you pick this up for 2.0 please?

Otherwise we will need to maintain 2.0
compatibility code which is annoying ...

> ---
>  target-i386/machine.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index 24bc373..168cab6 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -569,8 +569,8 @@ static const VMStateDescription vmstate_msr_hypercall_hypercall = {
>      .minimum_version_id = 1,
>      .minimum_version_id_old = 1,
>      .fields      = (VMStateField []) {
> -        VMSTATE_UINT64(env.msr_hv_hypercall, X86CPU),
>          VMSTATE_UINT64(env.msr_hv_guest_os_id, X86CPU),
> +        VMSTATE_UINT64(env.msr_hv_hypercall, X86CPU),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> -- 
> 1.9.0

Re: [Qemu-devel] [PATCH for-2.0] target-i386: reorder fields in cpu/msr_hyperv_hypercall subsection

[Peter Maydell]

On 2 April 2014 16:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
> The subsection already exists in one well-known enterprise Linux
> distribution, but for some strange reason the fields were swapped
> when forward-porting the patch to upstream.
>
> Limit headaches for said enterprise Linux distributor when the
> time will come to rebase their version of QEMU.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target-i386/machine.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index 24bc373..168cab6 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -569,8 +569,8 @@ static const VMStateDescription vmstate_msr_hypercall_hypercall = {
>      .minimum_version_id = 1,
>      .minimum_version_id_old = 1,
>      .fields      = (VMStateField []) {
> -        VMSTATE_UINT64(env.msr_hv_hypercall, X86CPU),
>          VMSTATE_UINT64(env.msr_hv_guest_os_id, X86CPU),
> +        VMSTATE_UINT64(env.msr_hv_hypercall, X86CPU),
>          VMSTATE_END_OF_LIST()
>      }

Surely this is a migration compatibility break and you need to bump
the version fields here?

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.0] target-i386: reorder fields in cpu/msr_hyperv_hypercall subsection

[Paolo Bonzini]

Il 02/04/2014 17:42, Peter Maydell ha scritto:
> On 2 April 2014 16:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> The subsection already exists in one well-known enterprise Linux
>> distribution, but for some strange reason the fields were swapped
>> when forward-porting the patch to upstream.
>>
>> Limit headaches for said enterprise Linux distributor when the
>> time will come to rebase their version of QEMU.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  target-i386/machine.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target-i386/machine.c b/target-i386/machine.c
>> index 24bc373..168cab6 100644
>> --- a/target-i386/machine.c
>> +++ b/target-i386/machine.c
>> @@ -569,8 +569,8 @@ static const VMStateDescription vmstate_msr_hypercall_hypercall = {
>>      .minimum_version_id = 1,
>>      .minimum_version_id_old = 1,
>>      .fields      = (VMStateField []) {
>> -        VMSTATE_UINT64(env.msr_hv_hypercall, X86CPU),
>>          VMSTATE_UINT64(env.msr_hv_guest_os_id, X86CPU),
>> +        VMSTATE_UINT64(env.msr_hv_hypercall, X86CPU),
>>          VMSTATE_END_OF_LIST()
>>      }
>
> Surely this is a migration compatibility break and you need to bump
> the version fields here?

No, it was introduced in 2.0 so we're still in time.

Paolo

Re: [Qemu-devel] [PATCH for-2.0] target-i386: reorder fields in cpu/msr_hyperv_hypercall subsection

[Michael S. Tsirkin]

On Wed, Apr 02, 2014 at 04:42:08PM +0100, Peter Maydell wrote:
> On 2 April 2014 16:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > The subsection already exists in one well-known enterprise Linux
> > distribution, but for some strange reason the fields were swapped
> > when forward-porting the patch to upstream.
> >
> > Limit headaches for said enterprise Linux distributor when the
> > time will come to rebase their version of QEMU.
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  target-i386/machine.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target-i386/machine.c b/target-i386/machine.c
> > index 24bc373..168cab6 100644
> > --- a/target-i386/machine.c
> > +++ b/target-i386/machine.c
> > @@ -569,8 +569,8 @@ static const VMStateDescription vmstate_msr_hypercall_hypercall = {
> >      .minimum_version_id = 1,
> >      .minimum_version_id_old = 1,
> >      .fields      = (VMStateField []) {
> > -        VMSTATE_UINT64(env.msr_hv_hypercall, X86CPU),
> >          VMSTATE_UINT64(env.msr_hv_guest_os_id, X86CPU),
> > +        VMSTATE_UINT64(env.msr_hv_hypercall, X86CPU),
> >          VMSTATE_END_OF_LIST()
> >      }
> 
> Surely this is a migration compatibility break and you need to bump
> the version fields here?
> 
> thanks
> -- PMM

Not if we fix it before we put out 2.0.

Re: [Qemu-devel] [PATCH for-2.0] target-i386: reorder fields in cpu/msr_hyperv_hypercall subsection

[Andreas Färber]

Am 02.04.2014 17:56, schrieb Michael S. Tsirkin:
> On Wed, Apr 02, 2014 at 04:42:08PM +0100, Peter Maydell wrote:
>> On 2 April 2014 16:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> The subsection already exists in one well-known enterprise Linux
>>> distribution, but for some strange reason the fields were swapped
>>> when forward-porting the patch to upstream.
>>>
>>> Limit headaches for said enterprise Linux distributor when the
>>> time will come to rebase their version of QEMU.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>  target-i386/machine.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/target-i386/machine.c b/target-i386/machine.c
>>> index 24bc373..168cab6 100644
>>> --- a/target-i386/machine.c
>>> +++ b/target-i386/machine.c
>>> @@ -569,8 +569,8 @@ static const VMStateDescription vmstate_msr_hypercall_hypercall = {
>>>      .minimum_version_id = 1,
>>>      .minimum_version_id_old = 1,
>>>      .fields      = (VMStateField []) {
>>> -        VMSTATE_UINT64(env.msr_hv_hypercall, X86CPU),
>>>          VMSTATE_UINT64(env.msr_hv_guest_os_id, X86CPU),
>>> +        VMSTATE_UINT64(env.msr_hv_hypercall, X86CPU),
>>>          VMSTATE_END_OF_LIST()
>>>      }
>>
>> Surely this is a migration compatibility break and you need to bump
>> the version fields here?
>>
>> thanks
>> -- PMM
> 
> Not if we fix it before we put out 2.0.

Is the version_id important for that enterprise distribution?

We usually didn't make this depend on the release but on individual
changes, so PMM has a point. If someone did a savevm on master and after
this patch tries to loadvm it, maybe nothing bad happens in this case
but something we could easily prevent.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH for-2.0] target-i386: reorder fields in cpu/msr_hyperv_hypercall subsection

[Paolo Bonzini]

Il 02/04/2014 18:10, Andreas Färber ha scritto:
> Is the version_id important for that enterprise distribution?
>
> We usually didn't make this depend on the release but on individual
> changes, so PMM has a point. If someone did a savevm on master and after
> this patch tries to loadvm it, maybe nothing bad happens in this case
> but something we could easily prevent.

In this case most likely they don't have the subsection (it's only there 
if you use "-cpu foo,hv_something").

A more likely source of breakage if you do savevm on master is that the 
machine type changed between the time of the savevm and the time of the 
release.  We obviously do not guarantee that between releases.

Paolo

Re: [Qemu-devel] [PATCH for-2.0] target-i386: reorder fields in cpu/msr_hyperv_hypercall subsection

[Peter Maydell]

On 2 April 2014 16:38, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Apr 02, 2014 at 05:33:02PM +0200, Paolo Bonzini wrote:
>> The subsection already exists in one well-known enterprise Linux
>> distribution, but for some strange reason the fields were swapped
>> when forward-porting the patch to upstream.
>>
>> Limit headaches for said enterprise Linux distributor when the
>> time will come to rebase their version of QEMU.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>
> Peter could you pick this up for 2.0 please?
>
> Otherwise we will need to maintain 2.0
> compatibility code which is annoying ...

Applied to master, thanks.

-- PMM