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

[Marcelo Tosatti]

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 <address@hidden>
> >>>>---
> >>>>  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.