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

[Alexander Graf]

On 25.11.14 11:11, Frank Blaschka wrote:
> 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.

Is this for overall CCW or only for PCI? Depending on the answer, you
can put the sei event list into the respective parent device.


Alex