Re: [Qemu-devel] [PATCH v6] s390x/pci: add common function measurement block

[David Hildenbrand]

On 08.01.19 18:29, Pierre Morel wrote:
> On 07/01/2019 12:30, David Hildenbrand wrote:
>> On 03.01.19 11:17, Pierre Morel wrote:
>>> From: Yi Min Zhao <address@hidden>
>>>
>>> Common function measurement block is used to report zPCI internal
>>> counters of successful pcilg/stg/stb and rpcit instructions to
>>> a memory location provided by the program.
>>>
>>> This patch introduces a new ZpciFmb structure and schedules a timer
>>> callback to copy the zPCI measures to the FMB in the guest memory
>>> at an interval time set to 4s.
>>>
>>> An error while attemping to update the FMB, would generate an error
>>> event to the guest.
>>>
>>> The pcilg/stg/stb and rpcit interception handlers increase the
>>> related counter on a successful call.
>>> The guest shall pass a null FMBA (FMB address) in the FIB (Function
>>> Information Block) when it issues a Modify PCI Function Control
>>> instruction to switch off FMB and stop the corresponding timer.
>>>
>>> Signed-off-by: Yi Min Zhao <address@hidden>
>>> Signed-off-by: Pierre Morel <address@hidden>
>>> ---
>>>   hw/s390x/s390-pci-bus.c  |   4 +-
>>>   hw/s390x/s390-pci-bus.h  |  29 +++++++++++
>>>   hw/s390x/s390-pci-inst.c | 133 
>>> +++++++++++++++++++++++++++++++++++++++++++++--
>>>   hw/s390x/s390-pci-inst.h |   1 +
>>>   4 files changed, 163 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
>>> index 060ff06..f0d34dd 100644
>>> --- a/hw/s390x/s390-pci-bus.c
>>> +++ b/hw/s390x/s390-pci-bus.c
>>> @@ -989,6 +989,7 @@ static void s390_pcihost_hot_unplug(HotplugHandler 
>>> *hotplug_dev,
>>>       bus = pci_get_bus(pci_dev);
>>>       devfn = pci_dev->devfn;
>>>       object_unparent(OBJECT(pci_dev));
>>> +    fmb_timer_free(pbdev);
>>
>> I wonder if this should go into the unrealize function instead.
>>
>>
>>
> 
> What make you think it should?
> All zPCI specific resources are free inside the hot_unplug callback.
> Why should the timer be handled differently?

Just because it is like that nowadays doesn't mean it is correct :)

> 
> The timer is not setup during realize, but later on a guest request.
> 
> AFAIK unrealize would be called after the unplug handler or is there a 
> case for which unrealize would be called and not the hotplug handler?

The unplug handler really only is there to unassign resources previously
assigned by the hotplug handler (and to eventually unrealize + free the
device). Using it for other purposes is usually logically wrong.
(unrealize/finalize is the right place for such things)

> 
> Regards,
> Pierre
> 
> 


-- 

Thanks,

David / dhildenb