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

[Collin Walling]

On 1/14/19 4:44 AM, Cornelia Huck wrote:
> On Mon, 14 Jan 2019 10:23:58 +0100
> David Hildenbrand <address@hidden> 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 <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 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 <address@hidden>
> >
> > :)
>
> Great!
>
> Still waiting for an ack from Collin (nudge, nudge :) before applying
> this.

Whoops, thought I already acked this one -- sorry!

I give this my full Reviewed-by: Collin Walling <address@hidden>