Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info

[Andreas Färber]

Am 28.07.2013 12:14, schrieb Michael S. Tsirkin:
> On Sun, Jul 28, 2013 at 11:38:17AM +0200, Andreas Färber wrote:
>> Am 28.07.2013 09:30, schrieb Michael S. Tsirkin:
>>> On Sun, Jul 28, 2013 at 02:12:49AM +0200, Andreas Färber wrote:
>>>> Am 25.07.2013 11:32, schrieb Michael S. Tsirkin:
>>>>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>>>>> index 3908860..daefdfb 100644
>>>>> --- a/hw/pci-host/piix.c
>>>>> +++ b/hw/pci-host/piix.c
>>>>> @@ -349,6 +349,14 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, 
>>>>> int *piix3_devfn,
>>>>>      return b;
>>>>>  }
>>>>>  
>>>>> +PCIBus *find_i440fx(void)
>>>>> +{
>>>>> +    PCIHostState *s = OBJECT_CHECK(PCIHostState,
>>>>> +                                   
>>>>> object_resolve_path("/machine/i440fx", NULL),
>>>>> +                                   TYPE_PCI_HOST_BRIDGE);
>>>>
>>>> Open-coded OBJECT_CHECK() - should use existing PCI_HOST_BRIDGE(...).
>>>>
>>>>> +    return s ? s->bus : NULL;
>>>>> +}
>>>>
>>>> Is this function really necessary? /machine/i440fx/pci.0 is a trivial
>>>> addition to the path that's already being used here. You can do:
>>>> PCIBus *bus = PCI_BUS(object_resolve_path("/machine/i440fx/pci.0"));
>>>> where you actually need to access it.
>>>
>>>
>>> I don't mind but I would like to avoid callers hard-coding
>>> paths, in this case "i440fx".
>>> Why the aversion to functions?
>>
>> Simply because QMP cannot call functions. It has to work with qom-list
>> and qom-get, so this is a test case showing what is missing and can IMO
>> easily be addressed for both parties.
> 
> Fine but if the function calls QOM things internally
> then where's the problem?

The grief with object_path_resolve_type() is that it's a "hack" Paolo
has added for his convenience (in audio code?) that QMP cannot use, so
we need name-based paths to be available.

And to clarify, I have been talking about two different time frames:
Small adjustments that you can make for 1.6 (e.g., casts, q35 property,
different QOM function/callsite used; if Anthony is okay with taking
ACPI at this late point) and post-1.6 cleanups to replace interim
constructs that involve refactorings outside your control (e.g.,
MemoryRegion QOM'ification, adding properties to random devices).

Andreas

>> The suggested cast to PCI_BUS() lets you use regular qdev functions btw
>> as a shortcut, QMP users would need to iterate children of that node.
>>
>> The suggested "pci.0" is considered stable for -device ...,bus=pci.0
>> according to review feedback the Xen people have received for libxl.

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg