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" <m...@redhat.com> 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.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
