From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51824) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V3WwK-0001hr-Lb for qemu-devel@nongnu.org; Sun, 28 Jul 2013 15:47:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V3WwF-0005Ll-MU for qemu-devel@nongnu.org; Sun, 28 Jul 2013 15:47:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:15425) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V3WwF-0005LZ-EO for qemu-devel@nongnu.org; Sun, 28 Jul 2013 15:47:31 -0400 Date: Sun, 28 Jul 2013 22:48:54 +0300 From: "Michael S. Tsirkin" Message-ID: <20130728194854.GA28008@redhat.com> 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> <20130728194032.457e52aa@thinkpad> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20130728194032.457e52aa@thinkpad> 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: Igor Mammedov Cc: Andreas =?iso-8859-1?Q?F=E4rber?= , qemu-devel@nongnu.org On Sun, Jul 28, 2013 at 07:40:32PM +0200, Igor Mammedov wrote: > On Sun, 28 Jul 2013 12:17:47 +0200 > Andreas F=E4rber wrote: >=20 > > 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.begi= n); > > >>>> - 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.begi= n); > > >>>> - 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_i= nfo, > > >>>> + "pci_hole_start", NULL)); > > >>>> + info->w32_max =3D cpu_to_le64(object_property_get_int(pci_i= nfo, > > >>>> + "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/propert= y. > > 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 limit= s. > > >=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_inf= o.w64.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.w3= 2.end, > > >>>> + 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 Prop= ertyInfo, > > >=20 > > > Or add DEFINE_PROP_UINT64_RO for this? > > >=20 > > >> user better not to touch these properties since they are mostly in= ternal API. > > >> but if we say it's internal properties then enforcing read-only mi= ght 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 argum= ent > > 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. I don't think we want user to control low level properties such and _start and _end. _size might be unavoidable but let's limit it to that. > > 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=FC= rnberg >=20 >=20 > --=20 > Regards, > Igor