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

[Alexander Graf]

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.

As for pending_sei, what is this about?

> 
>>> +
>>> +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?

> 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).


Alex