Re: [Qemu-devel] [PATCH v4 2/2] enable multi-function hot-add

[Cao jin <caoj.fnst@cn.fujitsu.com>]

On 10/23/2015 09:41 PM, Michael S. Tsirkin wrote:
> On Fri, Oct 23, 2015 at 04:14:07PM +0800, Cao jin wrote:
>>
>>
>> On 10/23/2015 02:50 PM, Michael S. Tsirkin wrote:
>>> On Fri, Oct 23, 2015 at 12:13:53PM +0800, Cao jin wrote:
>>>> Hi Michael
>>>>
>>>> On 10/22/2015 10:37 PM, Michael S. Tsirkin wrote:
>>>>> On Thu, Oct 22, 2015 at 07:57:52PM +0800, Cao jin wrote:
>>>>>> Enable pcie device multifunction hot, just ensure the function 0
>>>>>> added last, then driver will got the notification to scan all the
>>>>>> function in the slot.
>>>>>>
>>>>>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>>>>>> ---
>>>>>>   hw/pci/pci.c         | 29 +++++++++++++++++++++++++++++
>>>>>>   hw/pci/pci_host.c    | 11 +++++++++--
>>>>>>   hw/pci/pcie.c        | 18 +++++++++---------
>>>>>>   include/hw/pci/pci.h |  1 +
>>>>>>   4 files changed, 48 insertions(+), 11 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>>>> index b0bf540..c068248 100644
>>>>>> --- a/hw/pci/pci.c
>>>>>> +++ b/hw/pci/pci.c
>>>>>> @@ -847,6 +847,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>>>>>>       PCIConfigWriteFunc *config_write = pc->config_write;
>>>>>>       Error *local_err = NULL;
>>>>>>       AddressSpace *dma_as;
>>>>>> +    DeviceState *dev = DEVICE(pci_dev);
>>>>>>
>>>>>>       if (devfn < 0) {
>>>>>>           for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
>>>>>> @@ -864,6 +865,16 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>>>>>>                      PCI_SLOT(devfn), PCI_FUNC(devfn), name,
>>>>>>                      bus->devices[devfn]->name);
>>>>>>           return NULL;
>>>>>> +    } else if (dev->hotplugged &&
>>>>>> +               ((!pc->is_express && bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]) ||
>>>>>> +                (pc->is_express && bus->devices[0]))) {
>>>>>
>>>>> So let's change pci_is_function_0 to pci_get_function_0 and reuse here?
>>>>
>>>> ok, will modify it and all the following comment.
>>>>
>>>>>
>>>>>> +        error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
>>>>>> +                   " new func %s cannot be exposed to guest.",
>>>>>> +                   PCI_SLOT(devfn),
>>>>>> +                   bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]->name,
>>>>>> +                   name);
>>>>>> +
>>>>>> +       return NULL;
>>>>>>       }
>>>>>>
>>>>>>       pci_dev->bus = bus;
>>>>>> @@ -2454,6 +2465,24 @@ void pci_bus_get_w64_range(PCIBus *bus, Range *range)
>>>>>>       pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>>>>>>   }
>>>>>>
>>>>>> +/* ARI device function number range is 0-255, means has only 1 function0;
>>>>>> + * while PCI device has 1 function0 in each slot(up to 32 slot) */
>>>>>> +bool pci_is_function_0(PCIDevice *pci_dev)
>>>>>> +{
>>>>>> +    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
>>>>>> +    bool is_func0 = false;
>>>>>> +
>>>>>> +    if (pc->is_express) {
>>>>>
>>>>>
>>>>> This is not what the spec says. It says:
>>>>> devices connected to an upstream port.
>>>>>
>>>> Yes, I am wrong here. I got confused about the ARI device definition in
>>>> spec:"a device associated with an Upstream Port". Because as I understand,
>>>> ARI device is a EP immediately below a ARI downstream port, just as Figure
>>>> 6-13(Example System Topology with ARI Devices) shows in the spec, but where
>>>> is upstream port in the definition come from, what the relationship between
>>>> upstream port and a ARI device?
>>>
>>> See Terms and Acronyms:
>>> 	The Port on a
>>> 	component that contains only Endpoint or Bridge Functions is an Upstream
>>> 	Port.
>>>
>> Thanks michael. if I understand right, ARI device has a upstream port on the
>> device, while on-ARI device doesn`t have?
>
> That's what it says, isn't it?
>

Yup, seems it does is. it redefined the concept of device in my mind, 
because never seen a figure shows that there is a port on a ARI device, 
like the figure of "ARI Device X" I draw below
-------------------------------
|       _______________       |
|      |_upstream port_|      |
|                             |
|                             |
|                             |
| F0  F1  F2 ... ..     F255  |
------------------------------|

Thanks for your time for this question, I will go on dealing with the 
other comments.

>> But in Figure 6-13(Example System Topology with ARI Devices) in the PCIe
>> spec, don`t see any notation of upstream port on ARI Device X & Y, so maybe
>> figure 6-13 is incomplete?
>
> You can see that they are connected to downstream or root ports.
>
>>>
>>>>>
>>>>>> +        if (!pci_dev->devfn)
>>>>>> +            is_func0 = true;
>>>>>
>>>>> Just do return !pci_dev->devfn here.
>>>>>
>>>>>> +    } else {
>>>>>> +        if(!PCI_FUNC(pci_dev->devfn))
>>>>>> +            is_func0 = true;
>>>>>> +    }
>>>>>> +
>>>>>> +    return is_func0;
>>>>>> +}
>>>>>> +
>>>>>>   static const TypeInfo pci_device_type_info = {
>>>>>>       .name = TYPE_PCI_DEVICE,
>>>>>>       .parent = TYPE_DEVICE,
>>>>>> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
>>>>>> index 3e26f92..40087c9 100644
>>>>>> --- a/hw/pci/pci_host.c
>>>>>> +++ b/hw/pci/pci_host.c
>>>>>> @@ -20,6 +20,7 @@
>>>>>>
>>>>>>   #include "hw/pci/pci.h"
>>>>>>   #include "hw/pci/pci_host.h"
>>>>>> +#include "hw/pci/pci_bus.h"
>>>>>>   #include "trace.h"
>>>>>>
>>>>>>   /* debug PCI */
>>>>>> @@ -75,7 +76,10 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
>>>>>>       PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
>>>>>>       uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>>>>>>
>>>>>> -    if (!pci_dev) {
>>>>>> +    /* non-zero functions are only exposed when function 0 is present,
>>>>>> +     * allowing direct removal of unexposed functions.
>>>>>> +     */
>>>>>> +    if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
>>>>>
>>>>> Reuse pci_get_function_0 here too?
>>>>>
>>>>>>           return;
>>>>>>       }
>>>>>>
>>>>>> @@ -91,7 +95,10 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
>>>>>>       uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>>>>>>       uint32_t val;
>>>>>>
>>>>>> -    if (!pci_dev) {
>>>>>> +    /* non-zero functions are only exposed when function 0 is present,
>>>>>> +     * allowing direct removal of unexposed functions.
>>>>>> +     */
>>>>>> +    if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
>>>>>
>>>>> And here?
>>>>>
>>>>>>           return ~0x0;
>>>>>>       }
>>>>>>
>>>>>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>>>>>> index b1adeaf..d0a8006 100644
>>>>>> --- a/hw/pci/pcie.c
>>>>>> +++ b/hw/pci/pcie.c
>>>>>> @@ -249,16 +249,16 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>>>>>>           return;
>>>>>>       }
>>>>>>
>>>>>> -    /* TODO: multifunction hot-plug.
>>>>>> -     * Right now, only a device of function = 0 is allowed to be
>>>>>> -     * hot plugged/unplugged.
>>>>>> +    /* To enable multifunction hot-plug, we just ensure the function
>>>>>> +     * 0 added last. Until function 0 added, we set the sltsta and
>>>>>> +     * inform OS via event notification.
>>>>>
>>>>> Should be "when function 0 is added".
>>>>>
>>>>>>        */
>>>>>> -    assert(PCI_FUNC(pci_dev->devfn) == 0);
>>>>>> -
>>>>>> -    pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
>>>>>> -                               PCI_EXP_SLTSTA_PDS);
>>>>>> -    pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
>>>>>> -                        PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
>>>>>> +    if (pci_is_function_0(pci_dev)) {
>>>>>> +        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
>>>>>> +                                   PCI_EXP_SLTSTA_PDS);
>>>>>> +        pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
>>>>>> +                            PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
>>>>>> +    }
>>>>>>   }
>>>>>>
>>>>>>   static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
>>>>>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>>>>>> index f5e7fd8..2820cfd 100644
>>>>>> --- a/include/hw/pci/pci.h
>>>>>> +++ b/include/hw/pci/pci.h
>>>>>> @@ -397,6 +397,7 @@ void pci_for_each_bus_depth_first(PCIBus *bus,
>>>>>>                                     void *(*begin)(PCIBus *bus, void *parent_state),
>>>>>>                                     void (*end)(PCIBus *bus, void *state),
>>>>>>                                     void *parent_state);
>>>>>> +bool pci_is_function_0(PCIDevice *pci_dev);
>>>>>>
>>>>>>   /* Use this wrapper when specific scan order is not required. */
>>>>>>   static inline
>>>>>> --
>>>>>> 2.1.0
>>>>> .
>>>>>
>>>>
>>>> --
>>>> Yours Sincerely,
>>>>
>>>> Cao Jin
>>> .
>>>
>>
>> --
>> Yours Sincerely,
>>
>> Cao Jin
> .
>

-- 
Yours Sincerely,

Cao Jin