Re: [PATCH v6 03/16] target-i386: Add cpu object access routines for Hypervisor level.

2012-10-11 Thread Don Slutz 

On 10/10/12 11:40, Andreas Färber wrote:

Am 10.10.2012 17:22, schrieb Don Slutz:

On 10/09/12 15:13, Don Slutz wrote:

On 10/09/12 12:25, Marcelo Tosatti wrote:

On Mon, Sep 24, 2012 at 10:32:05AM -0400, Don Slutz wrote:

+static void x86_cpuid_set_hv_level(Object *obj, Visitor *v, void
*opaque,
+const char *name, Error **errp)
+{
+X86CPU *cpu = X86_CPU(obj);
+uint32_t value;
+
+visit_type_uint32(v, value, name, errp);
+if (error_is_set(errp)) {
+return;
+}
+
+if (value != 0  value  0x4000) {
+value += 0x4000;
+}

Whats the purpose of this? Adds ambiguity.

[...]

This is direct copy with adjustment from x86_cpuid_set_xlevel():

 if (value  0x8000) {
 value += 0x8000;
 }

(Pending patch:
http://comments.gmane.org/gmane.comp.emulators.qemu/172703 adds this)

(Any pending patch can be changed ;))


The adjustment is that 0 is a legal value. See
http://lkml.indiana.edu/hypermail/linux/kernel/1205.0/00100.html

This does mean that just like xlevel=1 and xlevel=0x8001 are the
same; hypervisor-level=1 and hypervisor-level=0x401 are the same.
If this is not wanted, I have no issue with removing it.

I have no strong opinion either way, but if there's only one call site,
I'd prefer to apply these fixups to user input before setting the
property and to have the property setter error out on invalid values. I
consider that cleaner than silently fixing up values inside the setter.

Regards,
Andreas

I find more then one call site.  And one of them is converting the 
predefined x86 cpus (like 486).  So I am not planning on a change.


I have finished up the v7 changes except for this.  I will wait until 
some time tomorrow to send it in case there is more on this topic.

-Don Slutz


--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 03/16] target-i386: Add cpu object access routines for Hypervisor level.

2012-10-10 Thread Andreas Färber 
Am 10.10.2012 17:22, schrieb Don Slutz:
 On 10/09/12 15:13, Don Slutz wrote:
 On 10/09/12 12:25, Marcelo Tosatti wrote:
 On Mon, Sep 24, 2012 at 10:32:05AM -0400, Don Slutz wrote:
 +static void x86_cpuid_set_hv_level(Object *obj, Visitor *v, void
 *opaque,
 +const char *name, Error **errp)
 +{
 +X86CPU *cpu = X86_CPU(obj);
 +uint32_t value;
 +
 +visit_type_uint32(v, value, name, errp);
 +if (error_is_set(errp)) {
 +return;
 +}
 +
 +if (value != 0  value  0x4000) {
 +value += 0x4000;
 +}
 Whats the purpose of this? Adds ambiguity.
[...]
 This is direct copy with adjustment from x86_cpuid_set_xlevel():
 
 if (value  0x8000) {
 value += 0x8000;
 }
 
 (Pending patch:
 http://comments.gmane.org/gmane.comp.emulators.qemu/172703 adds this)

(Any pending patch can be changed ;))

 The adjustment is that 0 is a legal value. See
 http://lkml.indiana.edu/hypermail/linux/kernel/1205.0/00100.html
 
 This does mean that just like xlevel=1 and xlevel=0x8001 are the
 same; hypervisor-level=1 and hypervisor-level=0x401 are the same. 
 If this is not wanted, I have no issue with removing it.

I have no strong opinion either way, but if there's only one call site,
I'd prefer to apply these fixups to user input before setting the
property and to have the property setter error out on invalid values. I
consider that cleaner than silently fixing up values inside the setter.

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
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 03/16] target-i386: Add cpu object access routines for Hypervisor level.

2012-10-10 Thread Don Slutz 

On 10/09/12 15:13, Don Slutz wrote:

On 10/09/12 12:25, Marcelo Tosatti wrote:

On Mon, Sep 24, 2012 at 10:32:05AM -0400, Don Slutz wrote:

These are modeled after x86_cpuid_get_xlevel and x86_cpuid_set_xlevel.

Signed-off-by: Don Slutz d...@cloudswitch.com
---
  target-i386/cpu.c |   29 +
  1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 25ca986..451de12 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1166,6 +1166,32 @@ static void x86_cpuid_set_tsc_freq(Object 
*obj, Visitor *v, void *opaque,

  cpu-env.tsc_khz = value / 1000;
  }
  +static void x86_cpuid_get_hv_level(Object *obj, Visitor *v, void 
*opaque,

+const char *name, Error **errp)
+{
+X86CPU *cpu = X86_CPU(obj);
+
+visit_type_uint32(v, cpu-env.cpuid_hv_level, name, errp);
+}
+
+static void x86_cpuid_set_hv_level(Object *obj, Visitor *v, void 
*opaque,

+const char *name, Error **errp)
+{
+X86CPU *cpu = X86_CPU(obj);
+uint32_t value;
+
+visit_type_uint32(v, value, name, errp);
+if (error_is_set(errp)) {
+return;
+}
+
+if (value != 0  value  0x4000) {
+value += 0x4000;
+}

Whats the purpose of this? Adds ambiguity.

Will add more info in this commit message.
   -Don
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Not clear on how to add info in the commit message.

This is direct copy with adjustment from x86_cpuid_set_xlevel():

if (value  0x8000) {
value += 0x8000;
}

(Pending patch: 
http://comments.gmane.org/gmane.comp.emulators.qemu/172703 adds this)


The adjustment is that 0 is a legal value. See 
http://lkml.indiana.edu/hypermail/linux/kernel/1205.0/00100.html


This does mean that just like xlevel=1 and xlevel=0x8001 are the 
same; hypervisor-level=1 and hypervisor-level=0x401 are the same.  
If this is not wanted, I have no issue with removing it.


   -Don Slutz
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 03/16] target-i386: Add cpu object access routines for Hypervisor level.

2012-10-09 Thread Marcelo Tosatti 
On Mon, Sep 24, 2012 at 10:32:05AM -0400, Don Slutz wrote:
 These are modeled after x86_cpuid_get_xlevel and x86_cpuid_set_xlevel.
 
 Signed-off-by: Don Slutz d...@cloudswitch.com
 ---
  target-i386/cpu.c |   29 +
  1 files changed, 29 insertions(+), 0 deletions(-)
 
 diff --git a/target-i386/cpu.c b/target-i386/cpu.c
 index 25ca986..451de12 100644
 --- a/target-i386/cpu.c
 +++ b/target-i386/cpu.c
 @@ -1166,6 +1166,32 @@ static void x86_cpuid_set_tsc_freq(Object *obj, 
 Visitor *v, void *opaque,
  cpu-env.tsc_khz = value / 1000;
  }
  
 +static void x86_cpuid_get_hv_level(Object *obj, Visitor *v, void *opaque,
 +const char *name, Error **errp)
 +{
 +X86CPU *cpu = X86_CPU(obj);
 +
 +visit_type_uint32(v, cpu-env.cpuid_hv_level, name, errp);
 +}
 +
 +static void x86_cpuid_set_hv_level(Object *obj, Visitor *v, void *opaque,
 +const char *name, Error **errp)
 +{
 +X86CPU *cpu = X86_CPU(obj);
 +uint32_t value;
 +
 +visit_type_uint32(v, value, name, errp);
 +if (error_is_set(errp)) {
 +return;
 +}
 +
 +if (value != 0  value  0x4000) {
 +value += 0x4000;
 +}

Whats the purpose of this? Adds ambiguity.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 03/16] target-i386: Add cpu object access routines for Hypervisor level.

2012-10-09 Thread Don Slutz 

On 10/09/12 12:25, Marcelo Tosatti wrote:

On Mon, Sep 24, 2012 at 10:32:05AM -0400, Don Slutz wrote:

These are modeled after x86_cpuid_get_xlevel and x86_cpuid_set_xlevel.

Signed-off-by: Don Slutz d...@cloudswitch.com
---
  target-i386/cpu.c |   29 +
  1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 25ca986..451de12 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1166,6 +1166,32 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor 
*v, void *opaque,
  cpu-env.tsc_khz = value / 1000;
  }
  
+static void x86_cpuid_get_hv_level(Object *obj, Visitor *v, void *opaque,

+const char *name, Error **errp)
+{
+X86CPU *cpu = X86_CPU(obj);
+
+visit_type_uint32(v, cpu-env.cpuid_hv_level, name, errp);
+}
+
+static void x86_cpuid_set_hv_level(Object *obj, Visitor *v, void *opaque,
+const char *name, Error **errp)
+{
+X86CPU *cpu = X86_CPU(obj);
+uint32_t value;
+
+visit_type_uint32(v, value, name, errp);
+if (error_is_set(errp)) {
+return;
+}
+
+if (value != 0  value  0x4000) {
+value += 0x4000;
+}

Whats the purpose of this? Adds ambiguity.

Will add more info in this commit message.
   -Don
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 03/16] target-i386: Add cpu object access routines for Hypervisor level.

2012-09-24 Thread Don Slutz 
These are modeled after x86_cpuid_get_xlevel and x86_cpuid_set_xlevel.

Signed-off-by: Don Slutz d...@cloudswitch.com
---
 target-i386/cpu.c |   29 +
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 25ca986..451de12 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1166,6 +1166,32 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor 
*v, void *opaque,
 cpu-env.tsc_khz = value / 1000;
 }
 
+static void x86_cpuid_get_hv_level(Object *obj, Visitor *v, void *opaque,
+const char *name, Error **errp)
+{
+X86CPU *cpu = X86_CPU(obj);
+
+visit_type_uint32(v, cpu-env.cpuid_hv_level, name, errp);
+}
+
+static void x86_cpuid_set_hv_level(Object *obj, Visitor *v, void *opaque,
+const char *name, Error **errp)
+{
+X86CPU *cpu = X86_CPU(obj);
+uint32_t value;
+
+visit_type_uint32(v, value, name, errp);
+if (error_is_set(errp)) {
+return;
+}
+
+if (value != 0  value  0x4000) {
+value += 0x4000;
+}
+cpu-env.cpuid_hv_level = value;
+cpu-env.cpuid_hv_level_set = true;
+}
+
 #if !defined(CONFIG_USER_ONLY)
 static void x86_get_hv_spinlocks(Object *obj, Visitor *v, void *opaque,
  const char *name, Error **errp)
@@ -2061,6 +2087,9 @@ static void x86_cpu_initfn(Object *obj)
 object_property_add(obj, enforce, bool,
 x86_cpuid_get_enforce,
 x86_cpuid_set_enforce, NULL, NULL, NULL);
+object_property_add(obj, hypervisor-level, int,
+x86_cpuid_get_hv_level,
+x86_cpuid_set_hv_level, NULL, NULL, NULL);
 #if !defined(CONFIG_USER_ONLY)
 object_property_add(obj, hv_spinlocks, int,
 x86_get_hv_spinlocks,
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html