From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38660) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4HYi-0001Kn-MH for qemu-devel@nongnu.org; Tue, 30 Jul 2013 17:34:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V4HYa-0002VU-8g for qemu-devel@nongnu.org; Tue, 30 Jul 2013 17:34:20 -0400 Received: from mail-ob0-x233.google.com ([2607:f8b0:4003:c01::233]:58468) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4HYa-0002VP-1p for qemu-devel@nongnu.org; Tue, 30 Jul 2013 17:34:12 -0400 Received: by mail-ob0-f179.google.com with SMTP id fb19so4218942obc.24 for ; Tue, 30 Jul 2013 14:34:11 -0700 (PDT) Sender: fluxion Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth 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> Message-ID: <20130730213407.14585.11429@loki> Date: Tue, 30 Jul 2013 16:34:07 -0500 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: =?utf-8?q?Andreas_F=C3=A4rber?= , "Michael S. Tsirkin" , Igor Mammedov Cc: qemu-devel@nongnu.org Quoting Andreas F=C3=A4rber (2013-07-28 05:17:47) > 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: > >>>> = > >>>> 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. It actually depends on the 'wire'/'serialized' encoding of the underlying visitor implementation. In this case we're 'serializing' into a QObject via QmpOutputVisitor, where all integers are actually stored as a QInt/int64 anyway, so this cast is unavoidable in this case regardless of what QAPI interface we use: we'll always end up storing as an int64, requiring us to re-cast to get back to original value. Best we can achieve is burying the cast deeper (or significantly re-working how QObject/QInt works) There is additional bounds checking performed prior to serialization if the serialized type is less than 64 bits though, so we'd probably want to add fixed-width accessors if we found ourselves in a situation where we needed to cast to a smaller datatype than the original. It's also worth noting that visiting uint64 types using int64 visitor interfaces isn't universally guaranteed to work: for certain visitor implementations the serialized encodings may not be compatible with one another. But in this case we should be good. > = > >> I guess it's copy-past error s/cpu_to_le64/cpu_to_le32/ > > = > > Not these are 64 bit values, but they need to be > > unsigned not signed. > > > >> but not need for extra API, with fixed property definition > >> i.e. s/UINT64/UNIT32/ property set code will take care about limits. > > = > > If you replace these with UINT32 you won't be able to > > specify values >4G. > > = > >>> 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"; > >>>> } > >>>> = > >>>> +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, > > = > > Or add DEFINE_PROP_UINT64_RO for this? > > = > >> 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. > > = > > So at the moment I do > > = > > qemu -device i440FX-pcihost,help > > = > > and this will get all properties. > > = > > 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 > = > -- = > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3= =BCrnberg