[Qemu-devel] [PATCH v6 0/2] PCI-e device multi-function hot-add support

[Qemu-devel] [PATCH v6 0/2] PCI-e device multi-function hot-add support

[Cao jin]

Support multi-function hot-add of PCIe device via device_add, just ensure
function 0 is added last. While allow user to cancel the hot-add operation
via device_del.

changelog v6:
1. reimplement pci_get_function_0() using mst`s suggestion code
2. modify pcie_cap_slot_hot_unplug_request_cb(), because PCIe device only
   have 1 slot.

changelog v5:
1. change pci_is_function_0() to pci_get_function_0(),
   and use it according to v4 comments.
2. reimplement the content of pci_get_function_0()

changelog v4:
1. take ari into account, add api: pci_is_function_0()
2. reorder the patch
3. other minor fix according to the v3 comment

changelog v3:
1. Flag device as unexposed when func 0 doesn`t exist, via return 0xFF
   in case of gratuitous pci bus scan.
2. Since device is unexposed to guest, can remove function individually,
   without interaction with the guest.

Cao jin (2):
  remove function during multi-function hot-add
  enable multi-function hot-add

 hw/pci/pci.c         | 40 +++++++++++++++++++++++++++++++++++++++-
 hw/pci/pci_host.c    | 13 +++++++++++--
 hw/pci/pcie.c        | 39 ++++++++++++++++++++++++++-------------
 include/hw/pci/pci.h |  1 +
 4 files changed, 77 insertions(+), 16 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v6 1/2] remove function during multi-function hot-add

[Cao jin]

In case user want to cancel the hot-add operation, should roll back,
device_del the added function that still don`t work.

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/pci/pcie.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 6e28985..a72d516 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -261,13 +261,31 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                         PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
 }
 
+static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
+{
+    object_unparent(OBJECT(dev));
+}
+
 void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
                                          DeviceState *dev, Error **errp)
 {
     uint8_t *exp_cap;
+    PCIDevice *pci_dev = PCI_DEVICE(dev);
+    PCIBus *bus = pci_dev->bus;
 
     pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp);
 
+    /* In case user cancel the operation of multi-function hot-add,
+     * remove the function that is unexposed to guest individually,
+     * without interaction with guest.
+     */
+    if (pci_dev->devfn &&
+        !bus->devices[0]) {
+        pcie_unplug_device(bus, pci_dev, NULL);
+
+        return;
+    }
+
     pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
 }
 
@@ -378,11 +396,6 @@ void pcie_cap_slot_reset(PCIDevice *dev)
     hotplug_event_update_event_status(dev);
 }
 
-static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
-{
-    object_unparent(OBJECT(dev));
-}
-
 void pcie_cap_slot_write_config(PCIDevice *dev,
                                 uint32_t addr, uint32_t val, int len)
 {
-- 
2.1.0

[Qemu-devel] [PATCH v6 2/2] enable multi-function hot-add

[Cao jin]

Enable PCIe device multi-function hot-add, just ensure function 0 is added
last, then driver will got the notification to scan the slot.

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/pci/pci.c         | 40 +++++++++++++++++++++++++++++++++++++++-
 hw/pci/pci_host.c    | 13 +++++++++++--
 hw/pci/pcie.c        | 18 +++++++++---------
 include/hw/pci/pci.h |  1 +
 4 files changed, 60 insertions(+), 12 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b0bf540..168b9cc 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -847,6 +847,9 @@ 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);
+
+    pci_dev->bus = bus;
 
     if (devfn < 0) {
         for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
@@ -864,9 +867,17 @@ 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 &&
+               pci_get_function_0(pci_dev)) {
+        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;
     pci_dev->devfn = devfn;
     dma_as = pci_device_iommu_address_space(pci_dev);
 
@@ -2454,6 +2465,33 @@ void pci_bus_get_w64_range(PCIBus *bus, Range *range)
     pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
 }
 
+static bool pcie_has_upstream_port(PCIDevice *dev)
+{
+    PCIDevice *parent_dev = pci_bridge_get_device(dev->bus);
+
+    /* Device associated with an upstream port.
+     * As there are several types of these, it's easier to check the
+     * parent device: upstream ports are always connected to
+     * root or downstream ports.
+     */
+    return parent_dev &&
+        pci_is_express(parent_dev) &&
+        parent_dev->exp.exp_cap &&
+        (pcie_cap_get_type(parent_dev) == PCI_EXP_TYPE_ROOT_PORT ||
+         pcie_cap_get_type(parent_dev) == PCI_EXP_TYPE_DOWNSTREAM);
+}
+
+PCIDevice *pci_get_function_0(PCIDevice *pci_dev)
+{
+    if(pcie_has_upstream_port(pci_dev)) {
+        /* With an upstream PCIe port, we only support 1 device at slot 0 */
+        return pci_dev->bus->devices[0];
+    } else {
+        /* Other bus types might support multiple devices at slots 0-31 */
+        return pci_dev->bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)];
+    }
+}
+
 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..63d7d2f 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,11 @@ 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 ||
+        (pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev))) {
         return;
     }
 
@@ -91,7 +96,11 @@ 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 ||
+        (pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev))) {
         return ~0x0;
     }
 
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index a72d516..32c65c2 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. When function 0 is added, we set the sltsta and
+     * inform OS via event notification.
      */
-    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_get_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..379b6e1 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);
+PCIDevice *pci_get_function_0(PCIDevice *pci_dev);
 
 /* Use this wrapper when specific scan order is not required. */
 static inline
-- 
2.1.0

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

[Alex Williamson]

On Tue, 2015-10-27 at 20:51 +0800, Cao jin wrote:
> Enable PCIe device multi-function hot-add, just ensure function 0 is added
> last, then driver will got the notification to scan the slot.
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/pci/pci.c         | 40 +++++++++++++++++++++++++++++++++++++++-
>  hw/pci/pci_host.c    | 13 +++++++++++--
>  hw/pci/pcie.c        | 18 +++++++++---------
>  include/hw/pci/pci.h |  1 +
>  4 files changed, 60 insertions(+), 12 deletions(-)

> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index 3e26f92..63d7d2f 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,11 @@ 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 ||
> +        (pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev))) {
>          return;
>      }
>  
> @@ -91,7 +96,11 @@ 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 ||
> +        (pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev))) {
>          return ~0x0;
>      }


Don't we need to do this in pci_host_config_read_common() and
pci_host_config_write_common() instead?  Otherwise I don't think we
catch config access via mmcfg.  Thanks,

Alex

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

[Cao jin]

Hi Alex

On 10/27/2015 10:21 PM, Alex Williamson wrote:
> On Tue, 2015-10-27 at 20:51 +0800, Cao jin wrote:
>> Enable PCIe device multi-function hot-add, just ensure function 0 is added
>> last, then driver will got the notification to scan the slot.
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/pci/pci.c         | 40 +++++++++++++++++++++++++++++++++++++++-
>>   hw/pci/pci_host.c    | 13 +++++++++++--
>>   hw/pci/pcie.c        | 18 +++++++++---------
>>   include/hw/pci/pci.h |  1 +
>>   4 files changed, 60 insertions(+), 12 deletions(-)
>
>> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
>> index 3e26f92..63d7d2f 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,11 @@ 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 ||
>> +        (pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev))) {
>>           return;
>>       }
>>
>> @@ -91,7 +96,11 @@ 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 ||
>> +        (pci_dev->qdev.hotplugged && !pci_get_function_0(pci_dev))) {
>>           return ~0x0;
>>       }
>
>
> Don't we need to do this in pci_host_config_read_common() and
> pci_host_config_write_common() instead?  Otherwise I don't think we
> catch config access via mmcfg.  Thanks,
>

Yup...my carelessness, will fix it. Thanks for reminding:)

> Alex
>
> .
>

-- 
Yours Sincerely,

Cao Jin