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

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

[Cao jin]

Support PCI-e device hot-add multi-function via device_add, just ensure
add the function 0 is added last. While allow user to roll back in the
middle via device_del, in case user regret.

changelog:
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):
  enable multi-function hot-add
  remove function during multi-function hot-add

 hw/pci/pci.c      | 10 ++++++++++
 hw/pci/pci_host.c |  6 +++++-
 hw/pci/pcie.c     | 38 +++++++++++++++++++++++++-------------
 3 files changed, 40 insertions(+), 14 deletions(-)

-- 
2.1.0

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

[Cao jin]

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  | 10 ++++++++++
 hw/pci/pcie.c | 18 +++++++++---------
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index ccea628..15b53d9 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,15 @@ 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 &&
+               bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]) {
+        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;
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 6e28985..89bf61b 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.
      */
-    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_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);
+    }
 }
 
 void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
-- 
2.1.0

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

[Cao jin]

In case user regret when hot-adding multi-function, should roll back,
device_del the function added but not exposed to the guest.

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/pci/pci_host.c |  6 +++++-
 hw/pci/pcie.c     | 22 +++++++++++++++++-----
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index 3e26f92..35e5cf3 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 */
@@ -88,10 +89,13 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
 uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
 {
     PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
+    PCIDevice *f0 = NULL;
     uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
     uint32_t val;
+    uint8_t slot = (addr >> 11) & 0x1F;
 
-    if (!pci_dev) {
+    f0 = s->devices[PCI_DEVFN(slot, 0)];
+    if (!pci_dev || (!f0 && pci_dev)) {
         return ~0x0;
     }
 
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 89bf61b..58d2153 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -261,13 +261,30 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
     }
 }
 
+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 regret when hot-adding multi function, remove the function
+     * that is unexposed to guest individually, without interaction with guest.
+     */
+    if (PCI_FUNC(pci_dev->devfn) > 0 &&
+            bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)] == NULL) {
+        pcie_unplug_device(bus, pci_dev, NULL);
+
+        return;
+    }
+
     pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
 }
 
@@ -378,11 +395,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

Re: [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio devices

[Michael S. Tsirkin]

On Tue, Oct 13, 2015 at 04:41:35PM +0800, Cao jin wrote:
> In case user regret when hot-adding multi-function, should roll back,
> device_del the function added but not exposed to the guest.
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/pci/pci_host.c |  6 +++++-
>  hw/pci/pcie.c     | 22 +++++++++++++++++-----
>  2 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index 3e26f92..35e5cf3 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 */
> @@ -88,10 +89,13 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
>  uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
>  {
>      PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
> +    PCIDevice *f0 = NULL;
>      uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>      uint32_t val;
> +    uint8_t slot = (addr >> 11) & 0x1F;
>  
> -    if (!pci_dev) {
> +    f0 = s->devices[PCI_DEVFN(slot, 0)];
> +    if (!pci_dev || (!f0 && pci_dev)) {
>          return ~0x0;
>      }
>

This seems to belong to the previous patch?
  
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 89bf61b..58d2153 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -261,13 +261,30 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>      }
>  }
>  
> +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 regret when hot-adding multi function, remove the function
> +     * that is unexposed to guest individually, without interaction with guest.
> +     */
> +    if (PCI_FUNC(pci_dev->devfn) > 0 &&
> +            bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)] == NULL) {

Shorter: PCI_FUNC(pci_dev->devfn) &&
	!bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]


> +        pcie_unplug_device(bus, pci_dev, NULL);
> +
> +        return;
> +    }
> +
>      pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
>  }
>  
> @@ -378,11 +395,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

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

[Michael S. Tsirkin]

On Tue, Oct 13, 2015 at 04:41:33PM +0800, Cao jin wrote:
> Support PCI-e device hot-add multi-function via device_add, just ensure
> add the function 0 is added last. While allow user to roll back in the
> middle via device_del, in case user regret.

This patch doesn't seem to account of AIR though.

> changelog:
> 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):
>   enable multi-function hot-add
>   remove function during multi-function hot-add
> 
>  hw/pci/pci.c      | 10 ++++++++++
>  hw/pci/pci_host.c |  6 +++++-
>  hw/pci/pcie.c     | 38 +++++++++++++++++++++++++-------------
>  3 files changed, 40 insertions(+), 14 deletions(-)
> 
> -- 
> 2.1.0

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

[Michael S. Tsirkin]

On Tue, Oct 13, 2015 at 04:41:33PM +0800, Cao jin wrote:
> Support PCI-e device hot-add multi-function via device_add, just ensure
> add the function 0 is added last. While allow user to roll back in the
> middle via device_del, in case user regret.

s/regret/cancels the operation/


> changelog:
> 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):
>   enable multi-function hot-add
>   remove function during multi-function hot-add
> 
>  hw/pci/pci.c      | 10 ++++++++++
>  hw/pci/pci_host.c |  6 +++++-
>  hw/pci/pcie.c     | 38 +++++++++++++++++++++++++-------------
>  3 files changed, 40 insertions(+), 14 deletions(-)
> 
> -- 
> 2.1.0

Re: [Qemu-devel] [PATCH v3 2/2] remove function during multi-function hot-add

[Michael S. Tsirkin]

On Tue, Oct 13, 2015 at 04:41:35PM +0800, Cao jin wrote:
> In case user regret when hot-adding multi-function, should roll back,
> device_del the function added but not exposed to the guest.
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>

I think this patch should come first, before we enable the
functionality that depends on it.


> ---
>  hw/pci/pci_host.c |  6 +++++-
>  hw/pci/pcie.c     | 22 +++++++++++++++++-----
>  2 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index 3e26f92..35e5cf3 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 */
> @@ -88,10 +89,13 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
>  uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
>  {
>      PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
> +    PCIDevice *f0 = NULL;
>      uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>      uint32_t val;
> +    uint8_t slot = (addr >> 11) & 0x1F;
>  
> -    if (!pci_dev) {
> +    f0 = s->devices[PCI_DEVFN(slot, 0)];
> +    if (!pci_dev || (!f0 && pci_dev)) {
>          return ~0x0;
>      }
>  
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 89bf61b..58d2153 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -261,13 +261,30 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>      }
>  }
>  
> +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 regret when hot-adding multi function, remove the function
> +     * that is unexposed to guest individually, without interaction with guest.
> +     */
> +    if (PCI_FUNC(pci_dev->devfn) > 0 &&
> +            bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)] == NULL) {
> +        pcie_unplug_device(bus, pci_dev, NULL);
> +
> +        return;
> +    }
> +
>      pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
>  }
>  
> @@ -378,11 +395,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

Re: [Qemu-devel] [PATCH v3 2/2] remove function during multi-function hot-add

[Cao jin]

Hi Michael

On 10/13/2015 04:51 PM, Michael S. Tsirkin wrote:
> On Tue, Oct 13, 2015 at 04:41:35PM +0800, Cao jin wrote:
>> In case user regret when hot-adding multi-function, should roll back,
>> device_del the function added but not exposed to the guest.
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>
> I think this patch should come first, before we enable the
> functionality that depends on it.
>
Do you mean, the function should be removed individually in any 
condition? Because as you know, device_del pci_dev will remove all the 
functions in the slot that are fulled exposed to the guest, Alex also 
mentioned this limitation before.
>
>> ---
>>   hw/pci/pci_host.c |  6 +++++-
>>   hw/pci/pcie.c     | 22 +++++++++++++++++-----
>>   2 files changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
>> index 3e26f92..35e5cf3 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 */
>> @@ -88,10 +89,13 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
>>   uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
>>   {
>>       PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
>> +    PCIDevice *f0 = NULL;
>>       uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>>       uint32_t val;
>> +    uint8_t slot = (addr >> 11) & 0x1F;
>>
>> -    if (!pci_dev) {
>> +    f0 = s->devices[PCI_DEVFN(slot, 0)];
>> +    if (!pci_dev || (!f0 && pci_dev)) {
>>           return ~0x0;
>>       }
>>
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index 89bf61b..58d2153 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -261,13 +261,30 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>>       }
>>   }
>>
>> +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 regret when hot-adding multi function, remove the function
>> +     * that is unexposed to guest individually, without interaction with guest.
>> +     */
>> +    if (PCI_FUNC(pci_dev->devfn) > 0 &&
>> +            bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)] == NULL) {
>> +        pcie_unplug_device(bus, pci_dev, NULL);
>> +
>> +        return;
>> +    }
>> +
>>       pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
>>   }
>>
>> @@ -378,11 +395,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
> .
>

-- 
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH v3 2/2] remove function during multi-function hot-add

[Michael S. Tsirkin]

On Tue, Oct 13, 2015 at 05:54:34PM +0800, Cao jin wrote:
> Hi Michael
> 
> On 10/13/2015 04:51 PM, Michael S. Tsirkin wrote:
> >On Tue, Oct 13, 2015 at 04:41:35PM +0800, Cao jin wrote:
> >>In case user regret when hot-adding multi-function, should roll back,
> >>device_del the function added but not exposed to the guest.
> >>
> >>Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> >
> >I think this patch should come first, before we enable the
> >functionality that depends on it.
> >
> Do you mean, the function should be removed individually in any condition?

I just mean the patches in the series should be reordered.

> Because as you know, device_del pci_dev will remove all the functions in the
> slot that are fulled exposed to the guest, Alex also mentioned this
> limitation before.
> >
> >>---
> >>  hw/pci/pci_host.c |  6 +++++-
> >>  hw/pci/pcie.c     | 22 +++++++++++++++++-----
> >>  2 files changed, 22 insertions(+), 6 deletions(-)
> >>
> >>diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> >>index 3e26f92..35e5cf3 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 */
> >>@@ -88,10 +89,13 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
> >>  uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
> >>  {
> >>      PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
> >>+    PCIDevice *f0 = NULL;
> >>      uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
> >>      uint32_t val;
> >>+    uint8_t slot = (addr >> 11) & 0x1F;
> >>
> >>-    if (!pci_dev) {
> >>+    f0 = s->devices[PCI_DEVFN(slot, 0)];
> >>+    if (!pci_dev || (!f0 && pci_dev)) {
> >>          return ~0x0;
> >>      }
> >>
> >>diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> >>index 89bf61b..58d2153 100644
> >>--- a/hw/pci/pcie.c
> >>+++ b/hw/pci/pcie.c
> >>@@ -261,13 +261,30 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> >>      }
> >>  }
> >>
> >>+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 regret when hot-adding multi function, remove the function
> >>+     * that is unexposed to guest individually, without interaction with guest.
> >>+     */
> >>+    if (PCI_FUNC(pci_dev->devfn) > 0 &&
> >>+            bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)] == NULL) {
> >>+        pcie_unplug_device(bus, pci_dev, NULL);
> >>+
> >>+        return;
> >>+    }
> >>+
> >>      pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
> >>  }
> >>
> >>@@ -378,11 +395,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
> >.
> >
> 
> -- 
> Yours Sincerely,
> 
> Cao Jin

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

[Cao jin]

On 10/13/2015 04:49 PM, Michael S. Tsirkin wrote:
> On Tue, Oct 13, 2015 at 04:41:33PM +0800, Cao jin wrote:
>> Support PCI-e device hot-add multi-function via device_add, just ensure
>> add the function 0 is added last. While allow user to roll back in the
>> middle via device_del, in case user regret.
>
> This patch doesn't seem to account of AIR though.
>

Yes, but the AIR function seems never be used(nobody calls the function  
pcie_ari_init()), so I am a little confused about should it be consindered?

>> changelog:
>> 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):
>>    enable multi-function hot-add
>>    remove function during multi-function hot-add
>>
>>   hw/pci/pci.c      | 10 ++++++++++
>>   hw/pci/pci_host.c |  6 +++++-
>>   hw/pci/pcie.c     | 38 +++++++++++++++++++++++++-------------
>>   3 files changed, 40 insertions(+), 14 deletions(-)
>>
>> --
>> 2.1.0
> .
>

-- 
Yours Sincerely,

Cao Jin

Re: [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio devices

[Cao jin]

Hi, Michael
     Thanks for your quick response:)

On 10/13/2015 04:48 PM, Michael S. Tsirkin wrote:
> On Tue, Oct 13, 2015 at 04:41:35PM +0800, Cao jin wrote:
>> In case user regret when hot-adding multi-function, should roll back,
>> device_del the function added but not exposed to the guest.
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/pci/pci_host.c |  6 +++++-
>>   hw/pci/pcie.c     | 22 +++++++++++++++++-----
>>   2 files changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
>> index 3e26f92..35e5cf3 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 */
>> @@ -88,10 +89,13 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
>>   uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
>>   {
>>       PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
>> +    PCIDevice *f0 = NULL;
>>       uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>>       uint32_t val;
>> +    uint8_t slot = (addr >> 11) & 0x1F;
>>
>> -    if (!pci_dev) {
>> +    f0 = s->devices[PCI_DEVFN(slot, 0)];
>> +    if (!pci_dev || (!f0 && pci_dev)) {
>>           return ~0x0;
>>       }
>>
>
> This seems to belong to the previous patch?
>
Strictly speaking, yes, will move it in next version.

>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index 89bf61b..58d2153 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -261,13 +261,30 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>>       }
>>   }
>>
>> +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 regret when hot-adding multi function, remove the function
>> +     * that is unexposed to guest individually, without interaction with guest.
>> +     */
>> +    if (PCI_FUNC(pci_dev->devfn) > 0 &&
>> +            bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)] == NULL) {
>
> Shorter: PCI_FUNC(pci_dev->devfn) &&
> 	!bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]
>

Ok, will fix it in next vertion

>
>> +        pcie_unplug_device(bus, pci_dev, NULL);
>> +
>> +        return;
>> +    }
>> +
>>       pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
>>   }
>>
>> @@ -378,11 +395,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
> .
>

-- 
Yours Sincerely,

Cao Jin

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

[Michael S. Tsirkin]

On Tue, Oct 13, 2015 at 07:54:07PM +0800, Cao jin wrote:
> 
> 
> On 10/13/2015 04:49 PM, Michael S. Tsirkin wrote:
> >On Tue, Oct 13, 2015 at 04:41:33PM +0800, Cao jin wrote:
> >>Support PCI-e device hot-add multi-function via device_add, just ensure
> >>add the function 0 is added last. While allow user to roll back in the
> >>middle via device_del, in case user regret.
> >
> >This patch doesn't seem to account of AIR though.
> >
> 
> Yes, but the AIR function seems never be used(nobody calls the function
> pcie_ari_init()), so I am a little confused about should it be consindered?

Yes please - we'll likely use that in the future. Pls add an API
that takes ari into account.

> >>changelog:
> >>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):
> >>   enable multi-function hot-add
> >>   remove function during multi-function hot-add
> >>
> >>  hw/pci/pci.c      | 10 ++++++++++
> >>  hw/pci/pci_host.c |  6 +++++-
> >>  hw/pci/pcie.c     | 38 +++++++++++++++++++++++++-------------
> >>  3 files changed, 40 insertions(+), 14 deletions(-)
> >>
> >>--
> >>2.1.0
> >.
> >
> 
> -- 
> Yours Sincerely,
> 
> Cao Jin

Re: [Qemu-devel] [PATCH v3 2/2] remove function during multi-function hot-add

[Alex Williamson]

On Tue, 2015-10-13 at 16:41 +0800, Cao jin wrote:
> In case user regret when hot-adding multi-function, should roll back,
> device_del the function added but not exposed to the guest.

As Michael suggests, this patch should come first, before we actually
enable multi-function hot-add.

> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/pci/pci_host.c |  6 +++++-
>  hw/pci/pcie.c     | 22 +++++++++++++++++-----
>  2 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index 3e26f92..35e5cf3 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 */
> @@ -88,10 +89,13 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
>  uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
>  {
>      PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
> +    PCIDevice *f0 = NULL;
>      uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>      uint32_t val;
> +    uint8_t slot = (addr >> 11) & 0x1F;
>  
> -    if (!pci_dev) {
> +    f0 = s->devices[PCI_DEVFN(slot, 0)];
> +    if (!pci_dev || (!f0 && pci_dev)) {


This uses a lot more variables and operations than it needs to:

if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {

Shouldn't we do the same on pci_data_write()?  A well behaved guest
won't blindly write to config space, but not all guests are well
behaved.

Comments in the code would be nice here to explain that non-zero
functions are only exposed when function zero is present, allowing
direct removal of unexposed devices.

I imagine that due to qemu locking that we don't have a race here, but
note that devices[] is populated early in the core pci realize function,
prior to the device initialize function, and there are any number of
reasons that failure could still occur, which would create a window
where the function is accessible.  I doubt this is an issue, but simply
note it for completeness.

>          return ~0x0;
>      }
>  
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 89bf61b..58d2153 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -261,13 +261,30 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>      }
>  }
>  
> +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 regret when hot-adding multi function, remove the function
> +     * that is unexposed to guest individually, without interaction with guest.
> +     */
> +    if (PCI_FUNC(pci_dev->devfn) > 0 &&
> +            bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)] == NULL) {

Similarly,

if (PCI_FUNC(pci_dev->devfn) && !bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {

> +        pcie_unplug_device(bus, pci_dev, NULL);
> +
> +        return;
> +    }
> +
>      pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
>  }
>  
> @@ -378,11 +395,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)
>  {

Re: [Qemu-devel] [PATCH v3 2/2] remove function during multi-function hot-add

[Cao jin]

Hi, Alex

On 10/13/2015 11:27 PM, Alex Williamson wrote:
> On Tue, 2015-10-13 at 16:41 +0800, Cao jin wrote:
>> In case user regret when hot-adding multi-function, should roll back,
>> device_del the function added but not exposed to the guest.
>
> As Michael suggests, this patch should come first, before we actually
> enable multi-function hot-add.
>
OK.
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/pci/pci_host.c |  6 +++++-
>>   hw/pci/pcie.c     | 22 +++++++++++++++++-----
>>   2 files changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
>> index 3e26f92..35e5cf3 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 */
>> @@ -88,10 +89,13 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
>>   uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
>>   {
>>       PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
>> +    PCIDevice *f0 = NULL;
>>       uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>>       uint32_t val;
>> +    uint8_t slot = (addr >> 11) & 0x1F;
>>
>> -    if (!pci_dev) {
>> +    f0 = s->devices[PCI_DEVFN(slot, 0)];
>> +    if (!pci_dev || (!f0 && pci_dev)) {
>
>
> This uses a lot more variables and operations than it needs to:
>
> if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
>
Ok. variables is intended to make the line shorter.

> Shouldn't we do the same on pci_data_write()?  A well behaved guest
> won't blindly write to config space, but not all guests are well
> behaved.
>

Yup, agree. I missed the consideration of bad behavior. I thought anyone 
use the device should read the vendor ID first(good behavior), then do 
anything he/she want. Thanks for reminding

> Comments in the code would be nice here to explain that non-zero
> functions are only exposed when function zero is present, allowing
> direct removal of unexposed devices.
>
OK

> I imagine that due to qemu locking that we don't have a race here, but
> note that devices[] is populated early in the core pci realize function,
> prior to the device initialize function, and there are any number of
> reasons that failure could still occur, which would create a window
> where the function is accessible.  I doubt this is an issue, but simply
> note it for completeness.

Ok, will consider the "function access window" condition, to see what I 
can do with it

>>           return ~0x0;
>>       }
>>
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index 89bf61b..58d2153 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -261,13 +261,30 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>>       }
>>   }
>>
>> +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 regret when hot-adding multi function, remove the function
>> +     * that is unexposed to guest individually, without interaction with guest.
>> +     */
>> +    if (PCI_FUNC(pci_dev->devfn) > 0 &&
>> +            bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)] == NULL) {
>
> Similarly,
>
> if (PCI_FUNC(pci_dev->devfn) && !bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
>

Ok

>> +        pcie_unplug_device(bus, pci_dev, NULL);
>> +
>> +        return;
>> +    }
>> +
>>       pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
>>   }
>>
>> @@ -378,11 +395,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)
>>   {
>
>
>
> .
>

-- 
Yours Sincerely,

Cao Jin

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

[Cao jin]

On 10/13/2015 09:10 PM, Michael S. Tsirkin wrote:
> On Tue, Oct 13, 2015 at 07:54:07PM +0800, Cao jin wrote:
>>
>>
>> On 10/13/2015 04:49 PM, Michael S. Tsirkin wrote:
>>> On Tue, Oct 13, 2015 at 04:41:33PM +0800, Cao jin wrote:
>>>> Support PCI-e device hot-add multi-function via device_add, just ensure
>>>> add the function 0 is added last. While allow user to roll back in the
>>>> middle via device_del, in case user regret.
>>>
>>> This patch doesn't seem to account of AIR though.
>>>
>>
>> Yes, but the AIR function seems never be used(nobody calls the function
>> pcie_ari_init()), so I am a little confused about should it be consindered?
>
> Yes please - we'll likely use that in the future. Pls add an API
> that takes ari into account.
>
Ok, I am on it

>>>> changelog:
>>>> 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):
>>>>    enable multi-function hot-add
>>>>    remove function during multi-function hot-add
>>>>
>>>>   hw/pci/pci.c      | 10 ++++++++++
>>>>   hw/pci/pci_host.c |  6 +++++-
>>>>   hw/pci/pcie.c     | 38 +++++++++++++++++++++++++-------------
>>>>   3 files changed, 40 insertions(+), 14 deletions(-)
>>>>
>>>> --
>>>> 2.1.0
>>> .
>>>
>>
>> --
>> Yours Sincerely,
>>
>> Cao Jin
> .
>

-- 
Yours Sincerely,

Cao Jin

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

[Cao jin]

Hello Michael

On 10/13/2015 09:10 PM, Michael S. Tsirkin wrote:
> On Tue, Oct 13, 2015 at 07:54:07PM +0800, Cao jin wrote:
>>
>>
>> On 10/13/2015 04:49 PM, Michael S. Tsirkin wrote:
>>> On Tue, Oct 13, 2015 at 04:41:33PM +0800, Cao jin wrote:
>>>> Support PCI-e device hot-add multi-function via device_add, just ensure
>>>> add the function 0 is added last. While allow user to roll back in the
>>>> middle via device_del, in case user regret.
>>>
>>> This patch doesn't seem to account of AIR though.
>>>
>>
>> Yes, but the AIR function seems never be used(nobody calls the function
>> pcie_ari_init()), so I am a little confused about should it be consindered?
>
> Yes please - we'll likely use that in the future. Pls add an API
> that takes ari into account.
>
after analysed the code and read the spec, but still a little confused 
about "add an API that takes ari into account". what is new API for? 
Could you give a detailed description?

AFAICT now, taking ari into account will affect the "addr" property of 
PCI device(maybe also need a new prop of "ari=on" to enable ari).

>>>> changelog:
>>>> 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):
>>>>    enable multi-function hot-add
>>>>    remove function during multi-function hot-add
>>>>
>>>>   hw/pci/pci.c      | 10 ++++++++++
>>>>   hw/pci/pci_host.c |  6 +++++-
>>>>   hw/pci/pcie.c     | 38 +++++++++++++++++++++++++-------------
>>>>   3 files changed, 40 insertions(+), 14 deletions(-)
>>>>
>>>> --
>>>> 2.1.0
>>> .
>>>
>>
>> --
>> Yours Sincerely,
>>
>> Cao Jin
> .
>

-- 
Yours Sincerely,

Cao Jin

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

[Michael S. Tsirkin]

On Wed, Oct 21, 2015 at 04:32:17PM +0800, Cao jin wrote:
> Hello Michael
> 
> On 10/13/2015 09:10 PM, Michael S. Tsirkin wrote:
> >On Tue, Oct 13, 2015 at 07:54:07PM +0800, Cao jin wrote:
> >>
> >>
> >>On 10/13/2015 04:49 PM, Michael S. Tsirkin wrote:
> >>>On Tue, Oct 13, 2015 at 04:41:33PM +0800, Cao jin wrote:
> >>>>Support PCI-e device hot-add multi-function via device_add, just ensure
> >>>>add the function 0 is added last. While allow user to roll back in the
> >>>>middle via device_del, in case user regret.
> >>>
> >>>This patch doesn't seem to account of AIR though.
> >>>
> >>
> >>Yes, but the AIR function seems never be used(nobody calls the function
> >>pcie_ari_init()), so I am a little confused about should it be consindered?
> >
> >Yes please - we'll likely use that in the future. Pls add an API
> >that takes ari into account.
> >
> after analysed the code and read the spec, but still a little confused about
> "add an API that takes ari into account". what is new API for? Could you
> give a detailed description?

You are assuming PCI_FUNC == 0 means function 9.  But the real
rule is: for an express device associated with an upstream port, it's
devfn == 0. For any other device (non express or not connected
to an upstream port) - PCI_FUNC == 0.

I think a reasonable API could be e.g.
bool pci_is_function_0()
with a comment explaining the issues.


> AFAICT now, taking ari into account will affect the "addr" property of PCI
> device(maybe also need a new prop of "ari=on" to enable ari).
> 
> >>>>changelog:
> >>>>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):
> >>>>   enable multi-function hot-add
> >>>>   remove function during multi-function hot-add
> >>>>
> >>>>  hw/pci/pci.c      | 10 ++++++++++
> >>>>  hw/pci/pci_host.c |  6 +++++-
> >>>>  hw/pci/pcie.c     | 38 +++++++++++++++++++++++++-------------
> >>>>  3 files changed, 40 insertions(+), 14 deletions(-)
> >>>>
> >>>>--
> >>>>2.1.0
> >>>.
> >>>
> >>
> >>--
> >>Yours Sincerely,
> >>
> >>Cao Jin
> >.
> >
> 
> -- 
> Yours Sincerely,
> 
> Cao Jin