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

[Qemu-devel] [PATCH 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.

Cao jin (2):
  PCI-e device multi-function hot-add support
  PCI-e device multi-function hot-add support

 hw/pci/pcie.c | 48 +++++++++++++++++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 13 deletions(-)

-- 
2.1.0

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

[Cao jin]

Enable PCI-e device multifunction hot, just ensure the function 0
added last, then driver will got the notification to scan all the
function in the slot.

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

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 6e28985..61ebefd 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -249,16 +249,20 @@ 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_dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) &&
+            PCI_FUNC(pci_dev->devfn) > 0) {
+        error_setg(errp, "single function device, function number must be 0,"
+                "but got %d", PCI_FUNC(pci_dev->devfn));
+    } else 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

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

[Alex Williamson]

On Thu, 2015-09-10 at 20:12 +0800, Cao jin wrote:
> Enable PCI-e device multifunction hot, just ensure the function 0
> added last, then driver will got the notification to scan all the
> function in the slot.
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/pci/pcie.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 6e28985..61ebefd 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -249,16 +249,20 @@ 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_dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) &&
> +            PCI_FUNC(pci_dev->devfn) > 0) {

The PCI spec is actually not entirely clear about whether each function
in a multifunction device needs to have the multifunction bit set or if
only function 0 needs to set it.  From a discovery perspective, a kernel
should only scan for function 0 and only scan non-zero funcions if
function 0 reports the multifunction bit set.  Therefore, it doesn't
particularly matter what non-zero functions report for the multifunction
bit.  QEMU allows either interpretation (see comment in
pci_init_multifunction), so this appears to be a new requirement that
breaks assumptions elsewhere.  Thanks,

Alex

> +        error_setg(errp, "single function device, function number must be 0,"
> +                "but got %d", PCI_FUNC(pci_dev->devfn));
> +    } else 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,

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

[Cao jin]

Hi, Alex,
     Thanks very much for your quick review and I am sorry for my late 
response.

On 09/10/2015 11:29 PM, Alex Williamson wrote:
> On Thu, 2015-09-10 at 20:12 +0800, Cao jin wrote:
>> Enable PCI-e device multifunction hot, just ensure the function 0
>> added last, then driver will got the notification to scan all the
>> function in the slot.
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/pci/pcie.c | 22 +++++++++++++---------
>>   1 file changed, 13 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index 6e28985..61ebefd 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -249,16 +249,20 @@ 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_dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) &&
>> +            PCI_FUNC(pci_dev->devfn) > 0) {
>
> The PCI spec is actually not entirely clear about whether each function
> in a multifunction device needs to have the multifunction bit set or if
> only function 0 needs to set it.  From a discovery perspective, a kernel
> should only scan for function 0 and only scan non-zero funcions if
> function 0 reports the multifunction bit set.  Therefore, it doesn't
> particularly matter what non-zero functions report for the multifunction
> bit.  QEMU allows either interpretation (see comment in
> pci_init_multifunction), so this appears to be a new requirement that
> breaks assumptions elsewhere.  Thanks,
>
> Alex
>
Yes, PCI Spec does not describe it clearly. The if() here, is based on 
the condition like following:

   (qemu) device_add e1000,bus=pb1,id=net1,addr=00.1
   (qemu) device_add e1000,bus=pb1,id=net0,addr=00.0
   PCI: 0.0 indicates single function, but 0.1 is already populated.

Now, we can see the net1 via "info pci" while can`t see net1 in guest 
via "lspci -t", so I think maybe it is not good condition, but no big 
deal, so I use the if() to prevent the condition.

will remove the if() in next version.

>> +        error_setg(errp, "single function device, function number must be 0,"
>> +                "but got %d", PCI_FUNC(pci_dev->devfn));
>> +    } else 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,
>
>
>
> .
>

-- 
Yours Sincerely,

Cao Jin

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

[Cao jin]

In case user regret when hot-add multi-function, we should roll back,
device_del the function added but still not worked.

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

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 61ebefd..b83a244 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -265,10 +265,33 @@ 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;
+
+    /* handle the condition: user want to hot-add multi function, but regret
+     * before finish it, and want to delete the added but not worked function.
+     */
+    if (PCI_FUNC(pci_dev->devfn) > 0 &&
+        bus->devices[PCI_DEVFN(0,0)] == NULL) {
+        pci_for_each_device(bus, pci_bus_num(bus),
+                            pcie_unplug_device, NULL);
+
+        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
+                                     PCI_EXP_SLTSTA_PDS);
+        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
+                                   PCI_EXP_SLTSTA_PDC);
+
+        return;
+    }
 
     pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp);
 
@@ -382,11 +405,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 2/2] PCI-e device multi-function hot-add support

[Alex Williamson]

On Thu, 2015-09-10 at 20:12 +0800, Cao jin wrote:
> In case user regret when hot-add multi-function, we should roll back,
> device_del the function added but still not worked.
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/pci/pcie.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 61ebefd..b83a244 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -265,10 +265,33 @@ 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;
> +
> +    /* handle the condition: user want to hot-add multi function, but regret
> +     * before finish it, and want to delete the added but not worked function.
> +     */
> +    if (PCI_FUNC(pci_dev->devfn) > 0 &&
> +        bus->devices[PCI_DEVFN(0,0)] == NULL) {
> +        pci_for_each_device(bus, pci_bus_num(bus),
> +                            pcie_unplug_device, NULL);
> +
> +        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
> +                                     PCI_EXP_SLTSTA_PDS);
> +        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> +                                   PCI_EXP_SLTSTA_PDC);
> +
> +        return;
> +    }

How can we know that the guest hasn't discovered the device and made use
of it?  The device is fully accessible to the guest even is the
notification and standard discovery mechanism are not in place.  A
gratuitous full PCI bus scan in the guest could find the device.
Thanks,

Alex

>  
>      pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp);
>  
> @@ -382,11 +405,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 2/2] PCI-e device multi-function hot-add support

[Igor Mammedov]

On Thu, 10 Sep 2015 20:12:23 +0800
Cao jin <caoj.fnst@cn.fujitsu.com> wrote:

> In case user regret when hot-add multi-function, we should roll back,
> device_del the function added but still not worked.
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/pci/pcie.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 61ebefd..b83a244 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -265,10 +265,33 @@ 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;
> +
> +    /* handle the condition: user want to hot-add multi function, but regret
> +     * before finish it, and want to delete the added but not worked function.
> +     */
sorry, I couldn't parse this comment and commit message as well. Please rephrase it.

> +    if (PCI_FUNC(pci_dev->devfn) > 0 &&
> +        bus->devices[PCI_DEVFN(0,0)] == NULL) {
> +        pci_for_each_device(bus, pci_bus_num(bus),
> +                            pcie_unplug_device, NULL);
*_unplug_request_cb is a way to communicate to guest that it should free
and eject device. So you are not allowed to destroy device from this path,
it's upto guest to decide what do on this request.

Look where pcie_unplug_device() is actually used, that's the place where
guest voluntary ejects device.

> +
> +        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
> +                                     PCI_EXP_SLTSTA_PDS);
> +        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> +                                   PCI_EXP_SLTSTA_PDC);
> +
> +        return;
> +    }
>  
>      pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp);
>  
> @@ -382,11 +405,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 2/2] PCI-e device multi-function hot-add support

[Cao jin]

Hi Igor,
     sorry I missed you mail.

On 09/11/2015 10:35 PM, Igor Mammedov wrote:
> On Thu, 10 Sep 2015 20:12:23 +0800
> Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
>
>> In case user regret when hot-add multi-function, we should roll back,
>> device_del the function added but still not worked.
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/pci/pcie.c | 28 +++++++++++++++++++++++-----
>>   1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index 61ebefd..b83a244 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -265,10 +265,33 @@ 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;
>> +
>> +    /* handle the condition: user want to hot-add multi function, but regret
>> +     * before finish it, and want to delete the added but not worked function.
>> +     */
> sorry, I couldn't parse this comment and commit message as well. Please rephrase it.
>

Ok, I send v2 before saw your mail, sorry. Maybe I can rephrase it in v3.

>> +    if (PCI_FUNC(pci_dev->devfn) > 0 &&
>> +        bus->devices[PCI_DEVFN(0,0)] == NULL) {
>> +        pci_for_each_device(bus, pci_bus_num(bus),
>> +                            pcie_unplug_device, NULL);
> *_unplug_request_cb is a way to communicate to guest that it should free
> and eject device. So you are not allowed to destroy device from this path,
> it's upto guest to decide what do on this request.
>
> Look where pcie_unplug_device() is actually used, that's the place where
> guest voluntary ejects device.
>

Agree. In v2 patch, I changed the delete process, fake the condition, 
let the Guest voluntary ejects device

>> +
>> +        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
>> +                                     PCI_EXP_SLTSTA_PDS);
>> +        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
>> +                                   PCI_EXP_SLTSTA_PDC);
>> +
>> +        return;
>> +    }
>>
>>       pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp);
>>
>> @@ -382,11 +405,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