On 05.11.18 12:40, Christian Borntraeger wrote: > > > 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 <da...@redhat.com> wrote: >>> >>>> We directly have it in our hands. >>>> >>>> Signed-off-by: David Hildenbrand <da...@redhat.com> >>>> --- >>>> 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? >
Caching resolution is fine (as that is expensive), caching a downcast is as far as I remember not necessary. Especially, as you said, for hotplug handlers. Anyhow, if there are strong feelings to this change, I can drop this patch. There are certainly more important things to do in zPCI hotplug code. -- Thanks, David / dhildenb