[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

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

[Michael S. Tsirkin]

On Tue, Oct 13, 2015 at 05:31:27PM +0300, Marcel Apfelbaum wrote:
> 
> 
> On Monday, October 12, 2015, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
>     On Mon, Oct 12, 2015 at 06:17:54PM +0300, Marcel Apfelbaum wrote:
>     > The virtio devices are converted to PCI-Express
>     > if they are plugged into a PCI-Express bus and
>     > the 'modern' protocol is enabled.
>     >
>     > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>     > ---
>     >
>     > This is an RFC because all it does it adds the PCIe capability and
>     nothing more.
> 
>     Express capability is easy.
>     But if you go over express space you will see that a bunch of
>     other capabilities are required, such as PM capability etc.
>     These might need more work.
> 
> 
> Sure, I'll look on the PCIe spec for the minimum requirements.
>  
> 
> 
>     > I post it early so I can get feedbacks on what is the best way to
>     continue it.
>     >
>     > Any comments would be appreciated,
>     > Thanks,
>     > Marcel
>     >
>     >  hw/virtio/virtio-pci.c | 11 +++++++++++
>     >  1 file changed, 11 insertions(+)
>     >
>     > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>     > index 6703806..f7c93d9 100644
>     > --- a/hw/virtio/virtio-pci.c
>     > +++ b/hw/virtio/virtio-pci.c
>     > @@ -1591,6 +1591,17 @@ static void virtio_pci_realize(PCIDevice *pci_dev,
>     Error **errp)
>     >
>     >      address_space_init(&proxy->modern_as, &proxy->modern_cfg,
>     "virtio-pci-cfg-as");
>     >
>     > +    if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN)
>     > +        && pci_bus_is_express(pci_dev->bus)) {
> 
>     One point: we probably want to avoid doing this for integrated
>     devices on root bus. Does pci_bus_is_express return true there?
> 
> 
> Hmm, I'll check, but I think it does. Is this a must? 

It's probably a smart thing to do because you can't e.g.
hot-unplug them. So supporting PCI E capability for
integrated devices is probably more work than just making
the PCI devices (which is not spec compliant but very popular
so guests support this).

> 
> 
>     > +        int pos = pci_add_capability(pci_dev, PCI_CAP_ID_EXP, 0,
>     > +                                     PCI_EXP_VER2_SIZEOF);
>     > +
>     > +        if (pos > 0) {
> 
>     We probably want to assert on pos < 0 instead.
>     That implies a code bug.
> 
> 
> 
> I was wondering what to do here, thanks for the idea!
> 
> Thanks,
> Marcel
> 
>  
> 
> 
>     > +            pci_dev->exp.exp_cap = pos;
>     > +            pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
>     > +        }
>     > +    }
>     > +
>     >      virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy);
>     >      if (k->realize) {
>     >          k->realize(proxy, errp);
>     > --
>     > 2.1.0
> 
> 

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

[Marcel Apfelbaum]

[-- Attachment #1: Type: text/plain, Size: 2240 bytes --]

On Monday, October 12, 2015, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Mon, Oct 12, 2015 at 06:17:54PM +0300, Marcel Apfelbaum wrote:
> > The virtio devices are converted to PCI-Express
> > if they are plugged into a PCI-Express bus and
> > the 'modern' protocol is enabled.
> >
> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com <javascript:;>>
> > ---
> >
> > This is an RFC because all it does it adds the PCIe capability and
> nothing more.
>
> Express capability is easy.
> But if you go over express space you will see that a bunch of
> other capabilities are required, such as PM capability etc.
> These might need more work.


Sure, I'll look on the PCIe spec for the minimum requirements.


>
> > I post it early so I can get feedbacks on what is the best way to
> continue it.
> >
> > Any comments would be appreciated,
> > Thanks,
> > Marcel
> >
> >  hw/virtio/virtio-pci.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index 6703806..f7c93d9 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -1591,6 +1591,17 @@ static void virtio_pci_realize(PCIDevice
> *pci_dev, Error **errp)
> >
> >      address_space_init(&proxy->modern_as, &proxy->modern_cfg,
> "virtio-pci-cfg-as");
> >
> > +    if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN)
> > +        && pci_bus_is_express(pci_dev->bus)) {
>
> One point: we probably want to avoid doing this for integrated
> devices on root bus. Does pci_bus_is_express return true there?


Hmm, I'll check, but I think it does. Is this a must?


>
> > +        int pos = pci_add_capability(pci_dev, PCI_CAP_ID_EXP, 0,
> > +                                     PCI_EXP_VER2_SIZEOF);
> > +
> > +        if (pos > 0) {
>
> We probably want to assert on pos < 0 instead.
> That implies a code bug.
>
>
I was wondering what to do here, thanks for the idea!

Thanks,
Marcel



>
> > +            pci_dev->exp.exp_cap = pos;
> > +            pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> > +        }
> > +    }
> > +
> >      virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy);
> >      if (k->realize) {
> >          k->realize(proxy, errp);
> > --
> > 2.1.0
>
>

[-- Attachment #2: Type: text/html, Size: 3429 bytes --]

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

[Michael S. Tsirkin]

On Tue, Oct 13, 2015 at 10:13:37AM +0200, Gerd Hoffmann wrote:
> On Mo, 2015-10-12 at 18:42 +0300, Michael S. Tsirkin wrote:
> > On Mon, Oct 12, 2015 at 06:17:54PM +0300, Marcel Apfelbaum wrote:
> > > The virtio devices are converted to PCI-Express
> > > if they are plugged into a PCI-Express bus and
> > > the 'modern' protocol is enabled.
> > > 
> > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> > > ---
> > > 
> > > This is an RFC because all it does it adds the PCIe capability and nothing more.
> > 
> > Express capability is easy.
> > But if you go over express space you will see that a bunch of
> > other capabilities are required, such as PM capability etc.
> > These might need more work.
> 
> Also what about the legacy io bar?  I guess we'd better avoid that for
> express devices.

This needs some thought.  We definitely still want a way to enable it I
think, e.g. for old guests.

> Maybe it makes sense to add virtio-*-pcie devices (virtio-1.0 only, with
> pcie caps)?
> 
> cheers,
>   Gerd

Personally I think it's ugly enough to remember the need to specify -pci.
OTOH the need to specify upstream and downstream ports is even uglier.
Any idea how to address both issues at the same time?

-- 
MST

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

[Gerd Hoffmann]

On Mo, 2015-10-12 at 18:42 +0300, Michael S. Tsirkin wrote:
> On Mon, Oct 12, 2015 at 06:17:54PM +0300, Marcel Apfelbaum wrote:
> > The virtio devices are converted to PCI-Express
> > if they are plugged into a PCI-Express bus and
> > the 'modern' protocol is enabled.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> > ---
> > 
> > This is an RFC because all it does it adds the PCIe capability and nothing more.
> 
> Express capability is easy.
> But if you go over express space you will see that a bunch of
> other capabilities are required, such as PM capability etc.
> These might need more work.

Also what about the legacy io bar?  I guess we'd better avoid that for
express devices.

Maybe it makes sense to add virtio-*-pcie devices (virtio-1.0 only, with
pcie caps)?

cheers,
  Gerd

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

[Michael S. Tsirkin]

On Mon, Oct 12, 2015 at 06:17:54PM +0300, Marcel Apfelbaum wrote:
> The virtio devices are converted to PCI-Express
> if they are plugged into a PCI-Express bus and
> the 'modern' protocol is enabled.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
> 
> This is an RFC because all it does it adds the PCIe capability and nothing more.

Express capability is easy.
But if you go over express space you will see that a bunch of
other capabilities are required, such as PM capability etc.
These might need more work.

> I post it early so I can get feedbacks on what is the best way to continue it.
> 
> Any comments would be appreciated,
> Thanks,
> Marcel
> 
>  hw/virtio/virtio-pci.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 6703806..f7c93d9 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1591,6 +1591,17 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
>  
>      address_space_init(&proxy->modern_as, &proxy->modern_cfg, "virtio-pci-cfg-as");
>  
> +    if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN)
> +        && pci_bus_is_express(pci_dev->bus)) {

One point: we probably want to avoid doing this for integrated
devices on root bus. Does pci_bus_is_express return true there?

> +        int pos = pci_add_capability(pci_dev, PCI_CAP_ID_EXP, 0,
> +                                     PCI_EXP_VER2_SIZEOF);
> +
> +        if (pos > 0) {

We probably want to assert on pos < 0 instead.
That implies a code bug.


> +            pci_dev->exp.exp_cap = pos;
> +            pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> +        }
> +    }
> +
>      virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy);
>      if (k->realize) {
>          k->realize(proxy, errp);
> -- 
> 2.1.0

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

[Marcel Apfelbaum]

The virtio devices are converted to PCI-Express
if they are plugged into a PCI-Express bus and
the 'modern' protocol is enabled.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---

This is an RFC because all it does it adds the PCIe capability and nothing more.
I post it early so I can get feedbacks on what is the best way to continue it.

Any comments would be appreciated,
Thanks,
Marcel

 hw/virtio/virtio-pci.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 6703806..f7c93d9 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1591,6 +1591,17 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
 
     address_space_init(&proxy->modern_as, &proxy->modern_cfg, "virtio-pci-cfg-as");
 
+    if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN)
+        && pci_bus_is_express(pci_dev->bus)) {
+        int pos = pci_add_capability(pci_dev, PCI_CAP_ID_EXP, 0,
+                                     PCI_EXP_VER2_SIZEOF);
+
+        if (pos > 0) {
+            pci_dev->exp.exp_cap = pos;
+            pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
+        }
+    }
+
     virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy);
     if (k->realize) {
         k->realize(proxy, errp);
-- 
2.1.0