From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60142) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V3UxV-0004Vs-Vi for qemu-devel@nongnu.org; Sun, 28 Jul 2013 13:40:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V3UxQ-0003Vi-Jb for qemu-devel@nongnu.org; Sun, 28 Jul 2013 13:40:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:32671) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V3UxQ-0003To-3b for qemu-devel@nongnu.org; Sun, 28 Jul 2013 13:40:36 -0400 Date: Sun, 28 Jul 2013 19:40:32 +0200 From: Igor Mammedov Message-ID: <20130728194032.457e52aa@thinkpad> In-Reply-To: <51F4EFCB.4050401@suse.de> 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> <51F4EFCB.4050401@suse.de> Mime-Version: 1.0 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: Andreas =?ISO-8859-1?B?RuRyYmVy?= Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" On Sun, 28 Jul 2013 12:17:47 +0200 Andreas F=E4rber wrote: > 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. >=20 > That's how QAPI works internally today for any uint64 visitor/property. > uint64_t is cast to int64_t and back in visitors. >=20 > So I'd hope something like > uint64_t val =3D (uint64_t) object_property_get_int() > would work equally well - CC'ing Michael. >=20 > >> 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] >=20 > Igor, you can simply use dynamic properties with NULL as setter argument > for object_property_add*() to achieve that effect. Yes, I can but it's more boiler plate code just for restricting single property. And if we have "pci_hole64_size"/default then user set "pci_hole64_end" would not have any effect, since "pci_hole64_size" would override it. > Andreas >=20 > --=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 --=20 Regards, Igor