From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51610) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V3O34-0004N5-V0 for qemu-devel@nongnu.org; Sun, 28 Jul 2013 06:18:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V3O2x-00080z-BQ for qemu-devel@nongnu.org; Sun, 28 Jul 2013 06:17:58 -0400 Received: from cantor2.suse.de ([195.135.220.15]:57475 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V3O2x-00080t-1t for qemu-devel@nongnu.org; Sun, 28 Jul 2013 06:17:51 -0400 Message-ID: <51F4EFCB.4050401@suse.de> Date: Sun, 28 Jul 2013 12:17:47 +0200 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1374996553-21828-1-git-send-email-imammedo@redhat.com> <1374996553-21828-7-git-send-email-imammedo@redhat.com> <20130728075712.GI12087@redhat.com> <20130728102156.12db54b9@thinkpad> <20130728091142.GA14505@redhat.com> In-Reply-To: <20130728091142.GA14505@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" , Igor Mammedov Cc: qemu-devel@nongnu.org Am 28.07.2013 11:11, schrieb Michael S. Tsirkin: > On Sun, Jul 28, 2013 at 10:21:56AM +0200, Igor Mammedov wrote: >> On Sun, 28 Jul 2013 10:57:12 +0300 >> "Michael S. Tsirkin" wrote: >> >>> On Sun, Jul 28, 2013 at 09:29:13AM +0200, Igor Mammedov wrote: >>>> =20 >>>> info =3D g_malloc(sizeof *info); >>>> - info->w32_min =3D cpu_to_le64(guest_info->pci_info.w32.begin); >>>> - info->w32_max =3D cpu_to_le64(guest_info->pci_info.w32.end); >>>> - info->w64_min =3D cpu_to_le64(guest_info->pci_info.w64.begin); >>>> - info->w64_max =3D cpu_to_le64(guest_info->pci_info.w64.end); >>>> + info->w32_min =3D cpu_to_le64(object_property_get_int(pci_info, >>>> + "pci_hole_start", NULL)); >>>> + info->w32_max =3D cpu_to_le64(object_property_get_int(pci_info, >>>> + "pci_hole_end", NULL)); >>> >>> Looks wrong. >>> object_property_get_int returns a signed int64. >>> w32 is unsigned. >>> Happens to work but I think we need an explicit API. That's how QAPI works internally today for any uint64 visitor/property. uint64_t is cast to int64_t and back in visitors. So I'd hope something like uint64_t val =3D (uint64_t) object_property_get_int() would work equally well - CC'ing Michael. >> I guess it's copy-past error s/cpu_to_le64/cpu_to_le32/ >=20 > Not these are 64 bit values, but they need to be > unsigned not signed. >=20 >> but not need for extra API, with fixed property definition >> i.e. s/UINT64/UNIT32/ property set code will take care about limits. >=20 > If you replace these with UINT32 you won't be able to > specify values >4G. >=20 >>> Property names are hard-coded string literals. >>> Please add macros to set and get them >>> so we can avoid duplicating code. >>> E.g. >> sure. >> >>> >>> #define PCI_HOST_PROPS... >>> static inline get_ [...] >>>> @@ -629,6 +648,15 @@ static const char *i440fx_pcihost_root_bus_path= (PCIHostState *host_bridge, >>>> return "0000"; >>>> } >>>> =20 >>>> +static Property i440fx_props[] =3D { >>>> + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState, pci_info.w6= 4.begin, 0), >>>> + DEFINE_PROP_UINT64("pci_hole64_end", I440FXState, pci_info.w64.= end, 0), >>>> + DEFINE_PROP_UINT64("pci_hole_start", I440FXState, pci_info.w32.= begin, 0), >>>> + DEFINE_PROP_UINT64("pci_hole_end", I440FXState, pci_info.w32.en= d, >>>> + IO_APIC_DEFAULT_ADDRESS), >>>> + DEFINE_PROP_END_OF_LIST(), >>>> +}; >>>> + >>> >>> So we have 4 properties. One of them pci_hole64_end >>> is supposed to be set to a value. >>> Others should not be touched under any circuimstances. >>> Of course if you query properties you have no way >>> to know which is which and what are the legal values. >>> Ouch. >> read-only properties are possible but we would have to drop >> usage DEFINE_PROP_UINT64 of and explicitly use only setter in Property= Info, >=20 > Or add DEFINE_PROP_UINT64_RO for this? >=20 >> user better not to touch these properties since they are mostly intern= al API. >> but if we say it's internal properties then enforcing read-only might = be >> overkill. >> For user friendly property "pci_hole64_size" would be nice to have. >=20 > So at the moment I do >=20 > qemu -device i440FX-pcihost,help >=20 > and this will get all properties. >=20 > If we add some properties that user can not set > they should not appear in this output. [snip] Igor, you can simply use dynamic properties with NULL as setter argument for object_property_add*() to achieve that effect. Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg