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

[Don Slutz <Don@CloudSwitch.Com>]

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