| Message ID | CAG4AFWaLLC-6tGyChAotKt25Kh4eUXwj=VQFBiN98Ue8F=tLBA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Jidong Xiao <jidong.xiao@gmail.com> writes: > On Wed, Jun 4, 2014 at 3:09 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> Il 04/06/2014 03:10, Jidong Xiao ha scritto: >> >>> diff --git a/qemu-2.0.0/target-i386/kvm.c.orig >>> b/qemu-2.0.0/target-i386/kvm.c >>> index 4389959..b8b282d 100644 >>> --- a/qemu-2.0.0/target-i386/kvm.c.orig >>> +++ b/qemu-2.0.0/target-i386/kvm.c >>> @@ -530,7 +530,7 @@ int kvm_arch_init_vcpu(CPUState *cs) >>> memcpy(signature, "KVMKVMKVM\0\0\0", 12); >>> c = &cpuid_data.entries[cpuid_i++]; >>> c->function = KVM_CPUID_SIGNATURE | kvm_base; >>> - c->eax = 0; >>> + c->eax = KVM_CPUID_FEATURES; >>> c->ebx = signature[0]; >>> c->ecx = signature[1]; >>> c->edx = signature[2]; >> >> >> This should actually be "KVM_CPUID_FEATURES | kvm_base", in case Hyper-V >> leaves are available too. But it is a good catch! >> >> Paolo >> > Thanks Paolo. I have just added that and resend the patch as following: > > -Jidong > > === > Signed-off-by: Jidong Xiao <jidong.xiao@gmail.com> Did you mean to send this as the new version of your patch ? Please send the revised version in a separate email and add "[PATCH v2]" to the subject. > --- > diff --git a/qemu-2.0.0/target-i386/kvm.c.orig b/qemu-2.0.0/target-i386/kvm.c > index 4389959..fe49a75 100644 > --- a/qemu-2.0.0/target-i386/kvm.c.orig > +++ b/qemu-2.0.0/target-i386/kvm.c > @@ -530,7 +530,7 @@ int kvm_arch_init_vcpu(CPUState *cs) > memcpy(signature, "KVMKVMKVM\0\0\0", 12); > c = &cpuid_data.entries[cpuid_i++]; > c->function = KVM_CPUID_SIGNATURE | kvm_base; > - c->eax = 0; > + c->eax = KVM_CPUID_FEATURES | kvm_base; Looks like this is not four spaces > c->ebx = signature[0]; > c->ecx = signature[1]; > c->edx = signature[2];
On Wed, Jun 4, 2014 at 3:08 PM, Bandan Das <bsd@redhat.com> wrote: > Jidong Xiao <jidong.xiao@gmail.com> writes: > >> On Wed, Jun 4, 2014 at 3:09 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> Il 04/06/2014 03:10, Jidong Xiao ha scritto: >>> >>>> diff --git a/qemu-2.0.0/target-i386/kvm.c.orig >>>> b/qemu-2.0.0/target-i386/kvm.c >>>> index 4389959..b8b282d 100644 >>>> --- a/qemu-2.0.0/target-i386/kvm.c.orig >>>> +++ b/qemu-2.0.0/target-i386/kvm.c >>>> @@ -530,7 +530,7 @@ int kvm_arch_init_vcpu(CPUState *cs) >>>> memcpy(signature, "KVMKVMKVM\0\0\0", 12); >>>> c = &cpuid_data.entries[cpuid_i++]; >>>> c->function = KVM_CPUID_SIGNATURE | kvm_base; >>>> - c->eax = 0; >>>> + c->eax = KVM_CPUID_FEATURES; >>>> c->ebx = signature[0]; >>>> c->ecx = signature[1]; >>>> c->edx = signature[2]; >>> >>> >>> This should actually be "KVM_CPUID_FEATURES | kvm_base", in case Hyper-V >>> leaves are available too. But it is a good catch! >>> >>> Paolo >>> >> Thanks Paolo. I have just added that and resend the patch as following: >> >> -Jidong >> >> === >> Signed-off-by: Jidong Xiao <jidong.xiao@gmail.com> > > Did you mean to send this as the new version of your patch ? > Please send the revised version in a separate email and add > "[PATCH v2]" to the subject. > Hi, Bandan, Thanks for the advice. I think Paolo has incorporated this patch into his patch sets, and he sent the revised version in a separate email earlier today with the subject "[PULL 11/11] kvm: Fix eax for cpuid leaf 0x40000000". -Jidong >> --- >> diff --git a/qemu-2.0.0/target-i386/kvm.c.orig b/qemu-2.0.0/target-i386/kvm.c >> index 4389959..fe49a75 100644 >> --- a/qemu-2.0.0/target-i386/kvm.c.orig >> +++ b/qemu-2.0.0/target-i386/kvm.c >> @@ -530,7 +530,7 @@ int kvm_arch_init_vcpu(CPUState *cs) >> memcpy(signature, "KVMKVMKVM\0\0\0", 12); >> c = &cpuid_data.entries[cpuid_i++]; >> c->function = KVM_CPUID_SIGNATURE | kvm_base; >> - c->eax = 0; >> + c->eax = KVM_CPUID_FEATURES | kvm_base; > > Looks like this is not four spaces > >> c->ebx = signature[0]; >> c->ecx = signature[1]; >> c->edx = signature[2];
Jidong Xiao <jidong.xiao@gmail.com> writes: .. >> > Hi, Bandan, > > Thanks for the advice. I think Paolo has incorporated this patch into > his patch sets, and he sent the revised version in a separate email > earlier today with the subject "[PULL 11/11] kvm: Fix eax for cpuid > leaf 0x40000000". Oops! Sorry, must have missed it. Thanks for the heads-up. > -Jidong > >>> --- >>> diff --git a/qemu-2.0.0/target-i386/kvm.c.orig b/qemu-2.0.0/target-i386/kvm.c >>> index 4389959..fe49a75 100644 >>> --- a/qemu-2.0.0/target-i386/kvm.c.orig >>> +++ b/qemu-2.0.0/target-i386/kvm.c >>> @@ -530,7 +530,7 @@ int kvm_arch_init_vcpu(CPUState *cs) >>> memcpy(signature, "KVMKVMKVM\0\0\0", 12); >>> c = &cpuid_data.entries[cpuid_i++]; >>> c->function = KVM_CPUID_SIGNATURE | kvm_base; >>> - c->eax = 0; >>> + c->eax = KVM_CPUID_FEATURES | kvm_base; >> >> Looks like this is not four spaces >> >>> c->ebx = signature[0]; >>> c->ecx = signature[1]; >>> c->edx = signature[2];
On Wed, Jun 04, 2014 at 03:17:56AM -0400, Jidong Xiao wrote: > On Wed, Jun 4, 2014 at 3:09 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > Il 04/06/2014 03:10, Jidong Xiao ha scritto: > > > >> diff --git a/qemu-2.0.0/target-i386/kvm.c.orig > >> b/qemu-2.0.0/target-i386/kvm.c > >> index 4389959..b8b282d 100644 > >> --- a/qemu-2.0.0/target-i386/kvm.c.orig > >> +++ b/qemu-2.0.0/target-i386/kvm.c > >> @@ -530,7 +530,7 @@ int kvm_arch_init_vcpu(CPUState *cs) > >> memcpy(signature, "KVMKVMKVM\0\0\0", 12); > >> c = &cpuid_data.entries[cpuid_i++]; > >> c->function = KVM_CPUID_SIGNATURE | kvm_base; > >> - c->eax = 0; > >> + c->eax = KVM_CPUID_FEATURES; > >> c->ebx = signature[0]; > >> c->ecx = signature[1]; > >> c->edx = signature[2]; > > > > > > This should actually be "KVM_CPUID_FEATURES | kvm_base", in case Hyper-V > > leaves are available too. But it is a good catch! > > > > Paolo > > > Thanks Paolo. I have just added that and resend the patch as following: > > -Jidong > > === > Signed-off-by: Jidong Xiao <jidong.xiao@gmail.com> > > --- > diff --git a/qemu-2.0.0/target-i386/kvm.c.orig b/qemu-2.0.0/target-i386/kvm.c > index 4389959..fe49a75 100644 > --- a/qemu-2.0.0/target-i386/kvm.c.orig > +++ b/qemu-2.0.0/target-i386/kvm.c > @@ -530,7 +530,7 @@ int kvm_arch_init_vcpu(CPUState *cs) > memcpy(signature, "KVMKVMKVM\0\0\0", 12); > c = &cpuid_data.entries[cpuid_i++]; > c->function = KVM_CPUID_SIGNATURE | kvm_base; > - c->eax = 0; > + c->eax = KVM_CPUID_FEATURES | kvm_base; This makes the CPUID data change under the guest's feet during live-migration. Adding compat code to ensure older machine-types keep the old behavior is necessary, but in this specific case it is mostly harmless because 0x0 is documented as being equivalent to 0x40000001. (But I don't know how guests are supposed to behave when they see CPUID[KVM_CPUID_SIGNATURE_NEXT].EAX==0.)
Il 12/08/2014 20:55, Eduardo Habkost ha scritto: > This makes the CPUID data change under the guest's feet during > live-migration. > > Adding compat code to ensure older machine-types keep the old behavior > is necessary, but in this specific case it is mostly harmless because > 0x0 is documented as being equivalent to 0x40000001. > > (But I don't know how guests are supposed to behave when they see > CPUID[KVM_CPUID_SIGNATURE_NEXT].EAX==0.) The only obvious thing to do would be to treat it as 0x40000101. Paolo
On Tue, Aug 12, 2014 at 09:12:00PM +0200, Paolo Bonzini wrote: > Il 12/08/2014 20:55, Eduardo Habkost ha scritto: > > This makes the CPUID data change under the guest's feet during > > live-migration. > > > > Adding compat code to ensure older machine-types keep the old behavior > > is necessary, but in this specific case it is mostly harmless because > > 0x0 is documented as being equivalent to 0x40000001. > > > > (But I don't know how guests are supposed to behave when they see > > CPUID[KVM_CPUID_SIGNATURE_NEXT].EAX==0.) > > The only obvious thing to do would be to treat it as 0x40000101. I just want to be sure the guests really do that. If we know guests won't do anything different with the CPUID change, I won't mind having no compat code for this.
Il 12/08/2014 21:29, Eduardo Habkost ha scritto: > On Tue, Aug 12, 2014 at 09:12:00PM +0200, Paolo Bonzini wrote: >> Il 12/08/2014 20:55, Eduardo Habkost ha scritto: >>> This makes the CPUID data change under the guest's feet during >>> live-migration. >>> >>> Adding compat code to ensure older machine-types keep the old behavior >>> is necessary, but in this specific case it is mostly harmless because >>> 0x0 is documented as being equivalent to 0x40000001. >>> >>> (But I don't know how guests are supposed to behave when they see >>> CPUID[KVM_CPUID_SIGNATURE_NEXT].EAX==0.) >> >> The only obvious thing to do would be to treat it as 0x40000101. > > I just want to be sure the guests really do that. If we know guests > won't do anything different with the CPUID change, I won't mind having > no compat code for this. > Considering that only two leaves are defined for KVM, and both are mandatory I don't think current guests have any reason to look at CPUID[KVM_CPUID_SIGNATURE | kvm_base].EAX at all. Paolo
=== Signed-off-by: Jidong Xiao <jidong.xiao@gmail.com> --- diff --git a/qemu-2.0.0/target-i386/kvm.c.orig b/qemu-2.0.0/target-i386/kvm.c index 4389959..fe49a75 100644 --- a/qemu-2.0.0/target-i386/kvm.c.orig +++ b/qemu-2.0.0/target-i386/kvm.c @@ -530,7 +530,7 @@ int kvm_arch_init_vcpu(CPUState *cs) memcpy(signature, "KVMKVMKVM\0\0\0", 12); c = &cpuid_data.entries[cpuid_i++]; c->function = KVM_CPUID_SIGNATURE | kvm_base; - c->eax = 0; + c->eax = KVM_CPUID_FEATURES | kvm_base; c->ebx = signature[0]; c->ecx = signature[1]; c->edx = signature[2];