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

[Cornelia Huck <cohuck@redhat.com>]

On Mon, 14 Jan 2019 10:23:58 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 14.01.19 10:18, Pierre Morel wrote:
> > On 09/01/2019 12:43, David Hildenbrand wrote:  
> >> On 09.01.19 12:29, David Hildenbrand wrote:  
> >>> On 08.01.19 18:37, Pierre Morel wrote:  
> >>>> From: Yi Min Zhao <zyimin@linux.ibm.com>
> >>>>
> >>>> 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 <zyimin@linux.ibm.com>
> >>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> >>>> ---
> >>>>   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 still think this is the wrong place. it has nothing to do with
> >>> hotplug/unplug. This belongs into unrealize/finalize.
> >>>
> >>>  
> >>
> >> ... but I see the issue. It boils down to the bad design of zPCI. The
> >> PCI device has no clue about the pbdev. So this has to stay here because
> >> in unrealize of pbdev it would be wrong and into unrealize of pci_dev,
> >> we can't move it.
> >>  
> > 
> > OK, thanks.
> > So is it a ack from you?  
> 
> Acked-by: David Hildenbrand <david@redhat.com>
> 
> :)

Great!

Still waiting for an ack from Collin (nudge, nudge :) before applying
this.