Re: [Qemu-devel] [PATCH 1/3] s390: Add PCI bus support

[Frank Blaschka]

On Tue, Nov 18, 2014 at 06:00:40PM +0100, Alexander Graf wrote:
> 
> 
> On 18.11.14 13:50, Frank Blaschka wrote:
> > On Mon, Nov 10, 2014 at 04:14:16PM +0100, Alexander Graf wrote:
> >>
> >>
> >> On 10.11.14 15:20, Frank Blaschka wrote:
> >>> From: Frank Blaschka <address@hidden>
> >>>
> >>> This patch implements a pci bus for s390x together with infrastructure
> >>> to generate and handle hotplug events, to configure/unconfigure via
> >>> sclp instruction, to do iommu translations and provide s390 support for
> >>> MSI/MSI-X notification processing.
> >>>
> >>> Signed-off-by: Frank Blaschka <address@hidden>
> 
> [...]
> 
> >>> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> >>> new file mode 100644
> >>> index 0000000..f2fa6ba
> >>> --- /dev/null
> >>> +++ b/hw/s390x/s390-pci-bus.c
> >>> @@ -0,0 +1,485 @@
> >>> +/*
> >>> + * s390 PCI BUS
> >>> + *
> >>> + * Copyright 2014 IBM Corp.
> >>> + * Author(s): Frank Blaschka <address@hidden>
> >>> + *            Hong Bo Li <address@hidden>
> >>> + *            Yi Min Zhao <address@hidden>
> >>> + *
> >>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> >>> + * your option) any later version. See the COPYING file in the top-level
> >>> + * directory.
> >>> + */
> >>> +
> >>> +#include <hw/pci/pci.h>
> >>> +#include <hw/pci/pci_bus.h>
> >>> +#include <hw/s390x/css.h>
> >>> +#include <hw/s390x/sclp.h>
> >>> +#include <hw/pci/msi.h>
> >>> +#include "qemu/error-report.h"
> >>> +#include "s390-pci-bus.h"
> >>> +
> >>> +/* #define DEBUG_S390PCI_BUS */
> >>> +#ifdef DEBUG_S390PCI_BUS
> >>> +#define DPRINTF(fmt, ...) \
> >>> +    do { fprintf(stderr, "S390pci-bus: " fmt, ## __VA_ARGS__); } while 
> >>> (0)
> >>> +#else
> >>> +#define DPRINTF(fmt, ...) \
> >>> +    do { } while (0)
> >>> +#endif
> >>> +
> >>> +static const unsigned long be_to_le = BITS_PER_LONG - 1;
> >>> +static QTAILQ_HEAD(, SeiContainer) pending_sei =
> >>> +    QTAILQ_HEAD_INITIALIZER(pending_sei);
> >>> +static QTAILQ_HEAD(, S390PCIBusDevice) device_list =
> >>> +    QTAILQ_HEAD_INITIALIZER(device_list);
> >>
> >> Please get rid of all statics ;). All state has to live in objects.
> >>
> > 
> > be_to_le was misleading and unnecesary will remove this one but
> > static QTAILQ_HEAD seems to be a common practice for list anchors.
> > If you really want me to change this do you have any prefered way,
> > or can you point me to some code doing this?
> 
> For PCI devices, I don't think you need a list at all. Your PHB device
> should already have a proper qbus that knows about all its child devices.

OK

> 
> As for pending_sei, what is this about?
>

This is a queue to store events (StoreEventInformation) used for hotplug
support. In case a device is pluged/unpluged an event is stored to this queue
and the guest is notified. Then the guest pick up the event information via
chsc instruction.
 
> > 
> >>> +
> >>> +int chsc_sei_nt2_get_event(void *res)
> 
> [...]
> 
> >>> +
> >>> +int chsc_sei_nt2_get_event(void *res);
> >>> +int chsc_sei_nt2_have_event(void);
> >>> +void s390_pci_sclp_configure(int configure, SCCB *sccb);
> >>> +S390PCIBusDevice *s390_pci_find_dev_by_idx(uint32_t idx);
> >>> +S390PCIBusDevice *s390_pci_find_dev_by_fh(uint32_t fh);
> >>> +S390PCIBusDevice *s390_pci_find_dev_by_fid(uint32_t fid);
> >>
> >> I think it makes sense to pass the PHB device as parameter on these.
> >> Don't assume you only have one.
> > 
> > We need to lookup our device mainly in the instruction handlers and there
> > we do not have a PHB available.
> 
> Then have a way to find your PHB - either put a variable into the
> machine object, or find it by path via QOM tree lookups. Maybe we need
> multiple PHBs, identified by part of the ID? I know too little about the
> way PCI works on s390x to really tell.
> 
> Again, are there specs?
>

Yes there are, but unfortunately they are not public.
 
> > Also having one list for our S390PCIBusDevices
> > devices does not prevent us from supporting more PHBs.
> > 
> >>
> >>> +void s390_pci_bus_init(void);
> >>> +uint64_t s390_pci_get_table_origin(uint64_t iota);
> >>> +uint64_t s390_guest_io_table_walk(uint64_t guest_iota,
> >>> +                                  uint64_t guest_dma_address);
> >>
> >> Why are these exported?
> >>
> >>> +
> >>> +#endif
> >>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> >>> index bc4dc2a..2e25834 100644
> >>> --- a/hw/s390x/s390-virtio-ccw.c
> >>> +++ b/hw/s390x/s390-virtio-ccw.c
> >>> @@ -18,6 +18,7 @@
> >>>  #include "css.h"
> >>>  #include "virtio-ccw.h"
> >>>  #include "qemu/config-file.h"
> >>> +#include "s390-pci-bus.h"
> >>>  
> >>>  #define TYPE_S390_CCW_MACHINE               "s390-ccw-machine"
> >>>  
> >>> @@ -127,6 +128,8 @@ static void ccw_init(MachineState *machine)
> >>>                        machine->initrd_filename, "s390-ccw.img");
> >>>      s390_flic_init();
> >>>  
> >>> +    s390_pci_bus_init();
> >>
> >> Please just inline that function here.
> >>
> > 
> > What do you mean by "just inline"?
> 
> The contents of the s390_pci_bus_init() function should just be standing
> right here. There's no value in creating a public wrapper function for
> initialization. We only did this back in the old days before qdev was
> around, because initialization was difficult back then and some devices
> didn't make the jump to get rid of their public init functions.
> 
> > 
> >>> +
> >>>      /* register hypercalls */
> >>>      virtio_ccw_register_hcalls();
> >>>  
> >>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> >>> index a759da7..a969975 100644
> >>> --- a/hw/s390x/sclp.c
> >>> +++ b/hw/s390x/sclp.c
> >>> @@ -20,6 +20,7 @@
> >>>  #include "qemu/config-file.h"
> >>>  #include "hw/s390x/sclp.h"
> >>>  #include "hw/s390x/event-facility.h"
> >>> +#include "hw/s390x/s390-pci-bus.h"
> >>>  
> >>>  static inline SCLPEventFacility *get_event_facility(void)
> >>>  {
> >>> @@ -62,7 +63,8 @@ static void read_SCP_info(SCCB *sccb)
> >>>          read_info->entries[i].type = 0;
> >>>      }
> >>>  
> >>> -    read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO);
> >>> +    read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
> >>> +                                        SCLP_HAS_PCI_RECONFIG);
> 
> Can we make this conditional on the fact that there is PCI available? Or
> do you want PCI support to be the baseline? Keep in mind that going
> forward, we need to start thinking about machine backwards compatibility
> too, so the QEMU 2.2 versioned machine should not expose PCI (though for
> now we don't care yet IIRC, but still please keep this in mind to get
> used to the way things will be in the future).
>

Yes, It should be dependent on the machine generation. Michael Mueller is
working on this. I will add this the time the new CPU model code comes
available. Same for the PHB itselv, it should only be created if maschine
supports zPCI. 
 
> 
> Alex
> 

I think I have addressed all issues now and plan to post a new version
of the patch set later this week.

Thx, Frank