From mboxrd@z Thu Jan  1 00:00:00 1970
From: =?ISO-8859-1?Q?Andreas_F=E4rber?= <afaerber@suse.de>
Subject: Re: [PATCH v6 03/16] target-i386: Add cpu object access routines
 for Hypervisor level.
Date: Wed, 10 Oct 2012 17:40:02 +0200
Message-ID: <507596D2.2000505@suse.de>
References: <1348497138-2516-1-git-send-email-Don@CloudSwitch.com> <1348497138-2516-4-git-send-email-Don@CloudSwitch.com> <20121009162520.GB12330@amt.cnet> <5074773F.6070503@CloudSwitch.Com> <507592C0.6030107@CloudSwitch.Com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: QUOTED-PRINTABLE
Cc: Marcelo Tosatti <mtosatti@redhat.com>, qemu-devel@nongnu.org,
	ehabkost@redhat.com, avi@redhat.com, peter.maydell@linaro.org,
	kvm@vger.kernel.org, anthony@codemonkey.ws
To: Don Slutz <Don@CloudSwitch.Com>,
	Igor Mammedov <imammedo@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from cantor2.suse.de ([195.135.220.15]:45060 "EHLO mx2.suse.de"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753983Ab2JJPkI (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 10 Oct 2012 11:40:08 -0400
In-Reply-To: <507592C0.6030107@CloudSwitch.Com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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 =3D X86_CPU(obj);
>>>> +    uint32_t value;
>>>> +
>>>> +    visit_type_uint32(v, &value, name, errp);
>>>> +    if (error_is_set(errp)) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (value !=3D 0 && value < 0x40000000) {
>>>> +        value +=3D 0x40000000;
>>>> +    }
>>> Whats the purpose of this? Adds ambiguity.
[...]
> This is direct copy with adjustment from x86_cpuid_set_xlevel():
>=20
>     if (value < 0x80000000) {
>         value +=3D 0x80000000;
>     }
>=20
> (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
>=20
> This does mean that just like xlevel=3D1 and xlevel=3D0x80000001 are =
the
> same; hypervisor-level=3D1 and hypervisor-level=3D0x4000001 are the s=
ame.=20
> 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

--=20
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrn=
berg