Patchwork [v6,05/16] target-i386: Use Hypervisor level in -machine pc, accel=kvm.

login
register
mail settings
Submitter Don Slutz
Date Sept. 24, 2012, 2:32 p.m.
Message ID <1348497138-2516-6-git-send-email-Don@CloudSwitch.com>
Download mbox | patch
Permalink /patch/186441/
State New
Headers show

Comments

Don Slutz - Sept. 24, 2012, 2:32 p.m.
Also known as Paravirtualization level.

This change is based on:

Microsoft Hypervisor CPUID Leaves:
  http://msdn.microsoft.com/en-us/library/windows/hardware/ff542428%28v=vs.85%29.aspx

Linux kernel change starts with:
  http://fixunix.com/kernel/538707-use-cpuid-communicate-hypervisor.html
Also:
  http://lkml.indiana.edu/hypermail/linux/kernel/1205.0/00100.html

VMware documention on CPUIDs (Mechanisms to determine if software is
running in a VMware virtual machine):
  http://kb.vmware.com/selfservice/microsites/search.do?language=en_US&cmd=displayKC&externalId=1009458

QEMU knows this is KVM_CPUID_SIGNATURE (0x40000000).

Signed-off-by: Don Slutz <Don@CloudSwitch.com>
---
 target-i386/kvm.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Marcelo Tosatti - Oct. 9, 2012, 5:18 p.m.
On Mon, Sep 24, 2012 at 10:32:07AM -0400, Don Slutz wrote:
> Also known as Paravirtualization level.
> 
> This change is based on:
> 
> Microsoft Hypervisor CPUID Leaves:
>   http://msdn.microsoft.com/en-us/library/windows/hardware/ff542428%28v=vs.85%29.aspx
> 
> Linux kernel change starts with:
>   http://fixunix.com/kernel/538707-use-cpuid-communicate-hypervisor.html
> Also:
>   http://lkml.indiana.edu/hypermail/linux/kernel/1205.0/00100.html
> 
> VMware documention on CPUIDs (Mechanisms to determine if software is
> running in a VMware virtual machine):
>   http://kb.vmware.com/selfservice/microsites/search.do?language=en_US&cmd=displayKC&externalId=1009458
> 
> QEMU knows this is KVM_CPUID_SIGNATURE (0x40000000).
> 
> Signed-off-by: Don Slutz <Don@CloudSwitch.com>
> ---
>  target-i386/kvm.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 895d848..8462c75 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -389,12 +389,12 @@ int kvm_arch_init_vcpu(CPUX86State *env)
>      c = &cpuid_data.entries[cpuid_i++];
>      memset(c, 0, sizeof(*c));
>      c->function = KVM_CPUID_SIGNATURE
> -    if (!hyperv_enabled()) {
> +    if (!env->cpuid_hv_level_set) {
>          memcpy(signature, "KVMKVMKVM\0\0\0", 12);
>          c->eax = 0;
>      } else {
>          memcpy(signature, "Microsoft Hv", 12);
> -        c->eax = HYPERV_CPUID_MIN;
> +        c->eax = env->cpuid_hv_level;

This breaks hyperv_enabled() checks. 

Don, are you certain it is worthwhile to make this configurable? 
Can you explain why, under your scenario, it is worthwhile?

Because these are separate problems:

- "Fake" VMWare hypervisor  (which seems to be your main goal).
- Make CPUID HV leafs configurable via command line.

My point is that the CPUIDs must be carefully constructed, 
that i miss the point why making them configurable is 
desired.
Marcelo Tosatti - Oct. 9, 2012, 6:27 p.m.
On Tue, Oct 09, 2012 at 02:18:05PM -0300, Marcelo Tosatti wrote:
> On Mon, Sep 24, 2012 at 10:32:07AM -0400, Don Slutz wrote:
> > Also known as Paravirtualization level.
> > 
> > This change is based on:
> > 
> > Microsoft Hypervisor CPUID Leaves:
> >   http://msdn.microsoft.com/en-us/library/windows/hardware/ff542428%28v=vs.85%29.aspx
> > 
> > Linux kernel change starts with:
> >   http://fixunix.com/kernel/538707-use-cpuid-communicate-hypervisor.html
> > Also:
> >   http://lkml.indiana.edu/hypermail/linux/kernel/1205.0/00100.html
> > 
> > VMware documention on CPUIDs (Mechanisms to determine if software is
> > running in a VMware virtual machine):
> >   http://kb.vmware.com/selfservice/microsites/search.do?language=en_US&cmd=displayKC&externalId=1009458
> > 
> > QEMU knows this is KVM_CPUID_SIGNATURE (0x40000000).
> > 
> > Signed-off-by: Don Slutz <Don@CloudSwitch.com>
> > ---
> >  target-i386/kvm.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 895d848..8462c75 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -389,12 +389,12 @@ int kvm_arch_init_vcpu(CPUX86State *env)
> >      c = &cpuid_data.entries[cpuid_i++];
> >      memset(c, 0, sizeof(*c));
> >      c->function = KVM_CPUID_SIGNATURE
> > -    if (!hyperv_enabled()) {
> > +    if (!env->cpuid_hv_level_set) {
> >          memcpy(signature, "KVMKVMKVM\0\0\0", 12);
> >          c->eax = 0;
> >      } else {
> >          memcpy(signature, "Microsoft Hv", 12);
> > -        c->eax = HYPERV_CPUID_MIN;
> > +        c->eax = env->cpuid_hv_level;
> 
> This breaks hyperv_enabled() checks. 
> 
> Don, are you certain it is worthwhile to make this configurable? 
> Can you explain why, under your scenario, it is worthwhile?
> 
> Because these are separate problems:
> 
> - "Fake" VMWare hypervisor  (which seems to be your main goal).
> - Make CPUID HV leafs configurable via command line.

Err, meant via properties. Point is, why have VMWare CPUID
configuration as data, if there are reasons to believe code 
is a better fit (code as in current Hyper-V implementation).

> My point is that the CPUIDs must be carefully constructed, 
> that i miss the point why making them configurable is 
> desired.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marcelo Tosatti - Oct. 9, 2012, 6:47 p.m.
On Tue, Oct 09, 2012 at 03:27:26PM -0300, Marcelo Tosatti wrote:
> On Tue, Oct 09, 2012 at 02:18:05PM -0300, Marcelo Tosatti wrote:
> > On Mon, Sep 24, 2012 at 10:32:07AM -0400, Don Slutz wrote:
> > > Also known as Paravirtualization level.
> > > 
> > > This change is based on:
> > > 
> > > Microsoft Hypervisor CPUID Leaves:
> > >   http://msdn.microsoft.com/en-us/library/windows/hardware/ff542428%28v=vs.85%29.aspx
> > > 
> > > Linux kernel change starts with:
> > >   http://fixunix.com/kernel/538707-use-cpuid-communicate-hypervisor.html
> > > Also:
> > >   http://lkml.indiana.edu/hypermail/linux/kernel/1205.0/00100.html
> > > 
> > > VMware documention on CPUIDs (Mechanisms to determine if software is
> > > running in a VMware virtual machine):
> > >   http://kb.vmware.com/selfservice/microsites/search.do?language=en_US&cmd=displayKC&externalId=1009458
> > > 
> > > QEMU knows this is KVM_CPUID_SIGNATURE (0x40000000).
> > > 
> > > Signed-off-by: Don Slutz <Don@CloudSwitch.com>
> > > ---
> > >  target-i386/kvm.c |    4 ++--
> > >  1 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > > index 895d848..8462c75 100644
> > > --- a/target-i386/kvm.c
> > > +++ b/target-i386/kvm.c
> > > @@ -389,12 +389,12 @@ int kvm_arch_init_vcpu(CPUX86State *env)
> > >      c = &cpuid_data.entries[cpuid_i++];
> > >      memset(c, 0, sizeof(*c));
> > >      c->function = KVM_CPUID_SIGNATURE
> > > -    if (!hyperv_enabled()) {
> > > +    if (!env->cpuid_hv_level_set) {
> > >          memcpy(signature, "KVMKVMKVM\0\0\0", 12);
> > >          c->eax = 0;
> > >      } else {
> > >          memcpy(signature, "Microsoft Hv", 12);
> > > -        c->eax = HYPERV_CPUID_MIN;
> > > +        c->eax = env->cpuid_hv_level;
> > 
> > This breaks hyperv_enabled() checks. 
> > 
> > Don, are you certain it is worthwhile to make this configurable? 
> > Can you explain why, under your scenario, it is worthwhile?
> > 
> > Because these are separate problems:
> > 
> > - "Fake" VMWare hypervisor  (which seems to be your main goal).
> > - Make CPUID HV leafs configurable via command line.
> 
> Err, meant via properties. Point is, why have VMWare CPUID
> configuration as data, if there are reasons to believe code 
> is a better fit (code as in current Hyper-V implementation).

Nevermind, its the right thing to do. Just separate the patchset
please:

1) Create object properties.
2) Export VMWare CPUID via properties.
3) Convert Hyper-V. 

Be careful to make sure Hyper-V's current options are functional 
in 3).
Don Slutz - Oct. 9, 2012, 7:09 p.m.
On 10/09/12 14:47, Marcelo Tosatti wrote:
> On Tue, Oct 09, 2012 at 03:27:26PM -0300, Marcelo Tosatti wrote:
>> On Tue, Oct 09, 2012 at 02:18:05PM -0300, Marcelo Tosatti wrote:
>>> On Mon, Sep 24, 2012 at 10:32:07AM -0400, Don Slutz wrote:
>>>> Also known as Paravirtualization level.
>>>>
>>>> This change is based on:
>>>>
>>>> Microsoft Hypervisor CPUID Leaves:
>>>>    http://msdn.microsoft.com/en-us/library/windows/hardware/ff542428%28v=vs.85%29.aspx
>>>>
>>>> Linux kernel change starts with:
>>>>    http://fixunix.com/kernel/538707-use-cpuid-communicate-hypervisor.html
>>>> Also:
>>>>    http://lkml.indiana.edu/hypermail/linux/kernel/1205.0/00100.html
>>>>
>>>> VMware documention on CPUIDs (Mechanisms to determine if software is
>>>> running in a VMware virtual machine):
>>>>    http://kb.vmware.com/selfservice/microsites/search.do?language=en_US&cmd=displayKC&externalId=1009458
>>>>
>>>> QEMU knows this is KVM_CPUID_SIGNATURE (0x40000000).
>>>>
>>>> Signed-off-by: Don Slutz <Don@CloudSwitch.com>
>>>> ---
>>>>   target-i386/kvm.c |    4 ++--
>>>>   1 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>>>> index 895d848..8462c75 100644
>>>> --- a/target-i386/kvm.c
>>>> +++ b/target-i386/kvm.c
>>>> @@ -389,12 +389,12 @@ int kvm_arch_init_vcpu(CPUX86State *env)
>>>>       c = &cpuid_data.entries[cpuid_i++];
>>>>       memset(c, 0, sizeof(*c));
>>>>       c->function = KVM_CPUID_SIGNATURE
>>>> -    if (!hyperv_enabled()) {
>>>> +    if (!env->cpuid_hv_level_set) {
>>>>           memcpy(signature, "KVMKVMKVM\0\0\0", 12);
>>>>           c->eax = 0;
>>>>       } else {
>>>>           memcpy(signature, "Microsoft Hv", 12);
>>>> -        c->eax = HYPERV_CPUID_MIN;
>>>> +        c->eax = env->cpuid_hv_level;
>>> This breaks hyperv_enabled() checks.
>>>
>>> Don, are you certain it is worthwhile to make this configurable?
>>> Can you explain why, under your scenario, it is worthwhile?
>>>
>>> Because these are separate problems:
>>>
>>> - "Fake" VMWare hypervisor  (which seems to be your main goal).
>>> - Make CPUID HV leafs configurable via command line.
>> Err, meant via properties. Point is, why have VMWare CPUID
>> configuration as data, if there are reasons to believe code
>> is a better fit (code as in current Hyper-V implementation).
> Nevermind, its the right thing to do. Just separate the patchset
> please:
>
> 1) Create object properties.
> 2) Export VMWare CPUID via properties.
> 3) Convert Hyper-V.
>
> Be careful to make sure Hyper-V's current options are functional
> in 3).
>
Did you mean 3 patch sets (or more)? Or just a different order?
    -Don Slutz
Marcelo Tosatti - Oct. 9, 2012, 7:11 p.m.
On Tue, Oct 09, 2012 at 03:09:17PM -0400, Don Slutz wrote:
> On 10/09/12 14:47, Marcelo Tosatti wrote:
> >On Tue, Oct 09, 2012 at 03:27:26PM -0300, Marcelo Tosatti wrote:
> >>On Tue, Oct 09, 2012 at 02:18:05PM -0300, Marcelo Tosatti wrote:
> >>>On Mon, Sep 24, 2012 at 10:32:07AM -0400, Don Slutz wrote:
> >>>>Also known as Paravirtualization level.
> >>>>
> >>>>This change is based on:
> >>>>
> >>>>Microsoft Hypervisor CPUID Leaves:
> >>>>   http://msdn.microsoft.com/en-us/library/windows/hardware/ff542428%28v=vs.85%29.aspx
> >>>>
> >>>>Linux kernel change starts with:
> >>>>   http://fixunix.com/kernel/538707-use-cpuid-communicate-hypervisor.html
> >>>>Also:
> >>>>   http://lkml.indiana.edu/hypermail/linux/kernel/1205.0/00100.html
> >>>>
> >>>>VMware documention on CPUIDs (Mechanisms to determine if software is
> >>>>running in a VMware virtual machine):
> >>>>   http://kb.vmware.com/selfservice/microsites/search.do?language=en_US&cmd=displayKC&externalId=1009458
> >>>>
> >>>>QEMU knows this is KVM_CPUID_SIGNATURE (0x40000000).
> >>>>
> >>>>Signed-off-by: Don Slutz <Don@CloudSwitch.com>
> >>>>---
> >>>>  target-i386/kvm.c |    4 ++--
> >>>>  1 files changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>>diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> >>>>index 895d848..8462c75 100644
> >>>>--- a/target-i386/kvm.c
> >>>>+++ b/target-i386/kvm.c
> >>>>@@ -389,12 +389,12 @@ int kvm_arch_init_vcpu(CPUX86State *env)
> >>>>      c = &cpuid_data.entries[cpuid_i++];
> >>>>      memset(c, 0, sizeof(*c));
> >>>>      c->function = KVM_CPUID_SIGNATURE
> >>>>-    if (!hyperv_enabled()) {
> >>>>+    if (!env->cpuid_hv_level_set) {
> >>>>          memcpy(signature, "KVMKVMKVM\0\0\0", 12);
> >>>>          c->eax = 0;
> >>>>      } else {
> >>>>          memcpy(signature, "Microsoft Hv", 12);
> >>>>-        c->eax = HYPERV_CPUID_MIN;
> >>>>+        c->eax = env->cpuid_hv_level;
> >>>This breaks hyperv_enabled() checks.
> >>>
> >>>Don, are you certain it is worthwhile to make this configurable?
> >>>Can you explain why, under your scenario, it is worthwhile?
> >>>
> >>>Because these are separate problems:
> >>>
> >>>- "Fake" VMWare hypervisor  (which seems to be your main goal).
> >>>- Make CPUID HV leafs configurable via command line.
> >>Err, meant via properties. Point is, why have VMWare CPUID
> >>configuration as data, if there are reasons to believe code
> >>is a better fit (code as in current Hyper-V implementation).
> >Nevermind, its the right thing to do. Just separate the patchset
> >please:
> >
> >1) Create object properties.
> >2) Export VMWare CPUID via properties.
> >3) Convert Hyper-V.
> >
> >Be careful to make sure Hyper-V's current options are functional
> >in 3).
> >
> Did you mean 3 patch sets (or more)? Or just a different order?
>    -Don Slutz

Different order. Patches should be logically related (think of what
information the reviewer needs). Please write changelogs for
every patch.
Don Slutz - Oct. 10, 2012, 1:03 p.m.
On 10/09/12 15:11, Marcelo Tosatti wrote:
> On Tue, Oct 09, 2012 at 03:09:17PM -0400, Don Slutz wrote:
>> On 10/09/12 14:47, Marcelo Tosatti wrote:
>>> On Tue, Oct 09, 2012 at 03:27:26PM -0300, Marcelo Tosatti wrote:
>>>> On Tue, Oct 09, 2012 at 02:18:05PM -0300, Marcelo Tosatti wrote:
>>>>> On Mon, Sep 24, 2012 at 10:32:07AM -0400, Don Slutz wrote:
>>>>>> Also known as Paravirtualization level.
>>>>>>
>>>>>> This change is based on:
>>>>>>
>>>>>> Microsoft Hypervisor CPUID Leaves:
>>>>>>    http://msdn.microsoft.com/en-us/library/windows/hardware/ff542428%28v=vs.85%29.aspx
>>>>>>
>>>>>> Linux kernel change starts with:
>>>>>>    http://fixunix.com/kernel/538707-use-cpuid-communicate-hypervisor.html
>>>>>> Also:
>>>>>>    http://lkml.indiana.edu/hypermail/linux/kernel/1205.0/00100.html
>>>>>>
>>>>>> VMware documention on CPUIDs (Mechanisms to determine if software is
>>>>>> running in a VMware virtual machine):
>>>>>>    http://kb.vmware.com/selfservice/microsites/search.do?language=en_US&cmd=displayKC&externalId=1009458
>>>>>>
>>>>>> QEMU knows this is KVM_CPUID_SIGNATURE (0x40000000).
>>>>>>
>>>>>> Signed-off-by: Don Slutz <Don@CloudSwitch.com>
>>>>>> ---
>>>>>>   target-i386/kvm.c |    4 ++--
>>>>>>   1 files changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>>>>>> index 895d848..8462c75 100644
>>>>>> --- a/target-i386/kvm.c
>>>>>> +++ b/target-i386/kvm.c
>>>>>> @@ -389,12 +389,12 @@ int kvm_arch_init_vcpu(CPUX86State *env)
>>>>>>       c = &cpuid_data.entries[cpuid_i++];
>>>>>>       memset(c, 0, sizeof(*c));
>>>>>>       c->function = KVM_CPUID_SIGNATURE
>>>>>> -    if (!hyperv_enabled()) {
>>>>>> +    if (!env->cpuid_hv_level_set) {
>>>>>>           memcpy(signature, "KVMKVMKVM\0\0\0", 12);
>>>>>>           c->eax = 0;
>>>>>>       } else {
>>>>>>           memcpy(signature, "Microsoft Hv", 12);
>>>>>> -        c->eax = HYPERV_CPUID_MIN;
>>>>>> +        c->eax = env->cpuid_hv_level;
>>>>> This breaks hyperv_enabled() checks.
>>>>>
>>>>> Don, are you certain it is worthwhile to make this configurable?
>>>>> Can you explain why, under your scenario, it is worthwhile?
>>>>>
>>>>> Because these are separate problems:
>>>>>
>>>>> - "Fake" VMWare hypervisor  (which seems to be your main goal).
>>>>> - Make CPUID HV leafs configurable via command line.
>>>> Err, meant via properties. Point is, why have VMWare CPUID
>>>> configuration as data, if there are reasons to believe code
>>>> is a better fit (code as in current Hyper-V implementation).
>>> Nevermind, its the right thing to do. Just separate the patchset
>>> please:
>>>
>>> 1) Create object properties.
>>> 2) Export VMWare CPUID via properties.
>>> 3) Convert Hyper-V.
>>>
>>> Be careful to make sure Hyper-V's current options are functional
>>> in 3).
>>>
>> Did you mean 3 patch sets (or more)? Or just a different order?
>>     -Don Slutz
> Different order. Patches should be logically related (think of what
> information the reviewer needs). Please write changelogs for
> every patch.
>
>
Using this order causes support for Hyper-V to stop working in the 
middle of the patch set.  How about this order:
1) Create object properties.
2) Convert Hyper-V to set the new properties.
3) Change kvm.c to use the new properties.
4) Export VMWare CPUID via properties.

5) Change accel=tcg to use the new properties.

    -Don Slutz
Marcelo Tosatti - Oct. 10, 2012, 2:08 p.m.
On Wed, Oct 10, 2012 at 09:03:20AM -0400, Don Slutz wrote:
> >>Did you mean 3 patch sets (or more)? Or just a different order?
> >>    -Don Slutz
> >Different order. Patches should be logically related (think of what
> >information the reviewer needs). Please write changelogs for
> >every patch.
> >
> >
> Using this order causes support for Hyper-V to stop working in the
> middle of the patch set.  How about this order:
> 1) Create object properties.
> 2) Convert Hyper-V to set the new properties.
> 3) Change kvm.c to use the new properties.
> 4) Export VMWare CPUID via properties.
> 
> 5) Change accel=tcg to use the new properties.
> 
>    -Don Slutz

Fine, as long as change from item A) does not leak to item B).

Patch

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 895d848..8462c75 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -389,12 +389,12 @@  int kvm_arch_init_vcpu(CPUX86State *env)
     c = &cpuid_data.entries[cpuid_i++];
     memset(c, 0, sizeof(*c));
     c->function = KVM_CPUID_SIGNATURE;
-    if (!hyperv_enabled()) {
+    if (!env->cpuid_hv_level_set) {
         memcpy(signature, "KVMKVMKVM\0\0\0", 12);
         c->eax = 0;
     } else {
         memcpy(signature, "Microsoft Hv", 12);
-        c->eax = HYPERV_CPUID_MIN;
+        c->eax = env->cpuid_hv_level;
     }
     c->ebx = signature[0];
     c->ecx = signature[1];