[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default |
Date: |
Sun, 28 Jul 2013 19:40:32 +0200 |
On Sun, 28 Jul 2013 12:17:47 +0200
Andreas Färber <address@hidden> 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" <address@hidden> wrote:
> >>
> >>> On Sun, Jul 28, 2013 at 09:29:13AM +0200, Igor Mammedov wrote:
> >>>>
> >>>> info = g_malloc(sizeof *info);
> >>>> - info->w32_min = cpu_to_le64(guest_info->pci_info.w32.begin);
> >>>> - info->w32_max = cpu_to_le64(guest_info->pci_info.w32.end);
> >>>> - info->w64_min = cpu_to_le64(guest_info->pci_info.w64.begin);
> >>>> - info->w64_max = cpu_to_le64(guest_info->pci_info.w64.end);
> >>>> + info->w32_min = cpu_to_le64(object_property_get_int(pci_info,
> >>>> + "pci_hole_start", NULL));
> >>>> + info->w32_max = 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 = (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/
> >
> > 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[] = {
> >>>> + DEFINE_PROP_UINT64("pci_hole64_start", I440FXState,
> >>>> pci_info.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.w32.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 PropertyInfo,
> >
> > Or add DEFINE_PROP_UINT64_RO for this?
> >
> >> user better not to touch these properties since they are mostly internal
> >> 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.
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
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
--
Regards,
Igor
- [Qemu-devel] [PATCH 3/6] utils: add range_size() wrapper, (continued)
- [Qemu-devel] [PATCH 3/6] utils: add range_size() wrapper, Igor Mammedov, 2013/07/28
- [Qemu-devel] [PATCH 4/6] pc: replace i440fx_common_init() with i440fx_init() as it isn't used by anywhere else, Igor Mammedov, 2013/07/28
- [Qemu-devel] [PATCH 5/6] pc: add Q35 to QOM composition tree under /machine, Igor Mammedov, 2013/07/28
- [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default, Igor Mammedov, 2013/07/28
- Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default, Igor Mammedov, 2013/07/28
- Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default, Michael S. Tsirkin, 2013/07/28
- Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default, Igor Mammedov, 2013/07/29
- Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default, Michael S. Tsirkin, 2013/07/29
- Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default, Michael S. Tsirkin, 2013/07/28
- Re: [Qemu-devel] [PATCH 6/6] pc: limit 64 bit hole to 2G by default, Igor Mammedov, 2013/07/28