Re: [Qemu-devel] [PATCH v1 2/4] s390x/zpci: use hotplug_dev instead of looking up the host bridge

[Christian Borntraeger]

On 11/05/2018 12:37 PM, David Hildenbrand wrote:
> On 05.11.18 12:21, Cornelia Huck wrote:
>> On Mon,  5 Nov 2018 12:03:11 +0100
>> David Hildenbrand <address@hidden> wrote:
>>
>>> We directly have it in our hands.
>>>
>>> Signed-off-by: David Hildenbrand <address@hidden>
>>> ---
>>>  hw/s390x/s390-pci-bus.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>> index 1eaae3aca6..68660eac74 100644
>>> --- a/hw/s390x/s390-pci-bus.c
>>> +++ b/hw/s390x/s390-pci-bus.c
>>> @@ -814,9 +814,9 @@ static bool s390_pci_alloc_idx(S390pciState *s, 
>>> S390PCIBusDevice *pbdev)
>>>  static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState 
>>> *dev,
>>>                                Error **errp)
>>>  {
>>> +    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
>>>      PCIDevice *pdev = NULL;
>>>      S390PCIBusDevice *pbdev = NULL;
>>> -    S390pciState *s = s390_get_phb();
>>>  
>>>      if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
>>>          BusState *bus;
>>> @@ -924,11 +924,11 @@ static void s390_pcihost_timer_cb(void *opaque)
>>>  static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState 
>>> *dev,
>>>                                  Error **errp)
>>>  {
>>> +    S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
>>>      PCIDevice *pci_dev = NULL;
>>>      PCIBus *bus;
>>>      int32_t devfn;
>>>      S390PCIBusDevice *pbdev = NULL;
>>> -    S390pciState *s = s390_get_phb();
>>>  
>>>      if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
>>>          error_setg(errp, "PCI bridge hot unplug currently not supported");
>>
>> Not sure whether that is an improvement (s390_get_phb() caches the
>> value, and is called from multiple other places as well.)
>>
> 
> Looking up a variable that is directly passed as an argument doesn't
> look clean to me.

I think there was a reason for this caching, namely that qom resolution can
be quite expensive. For the hotplug case this obviously does not matter but
for all the other cases it might. So do we really want to have different 
places use different methods?