diff mbox

[v4,2/2] enable multi-function hot-add

Message ID 1445515072-7968-3-git-send-email-caoj.fnst@cn.fujitsu.com
State New
Headers show

Commit Message

Cao jin Oct. 22, 2015, 11:57 a.m. UTC
Enable pcie device multifunction hot, just ensure the function 0
added last, then driver will got the notification to scan all the
function in the slot.

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

Comments

Michael S. Tsirkin Oct. 22, 2015, 2:37 p.m. UTC | #1
On Thu, Oct 22, 2015 at 07:57:52PM +0800, Cao jin wrote:
> Enable pcie device multifunction hot, just ensure the function 0
> added last, then driver will got the notification to scan all the
> function in the slot.
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/pci/pci.c         | 29 +++++++++++++++++++++++++++++
>  hw/pci/pci_host.c    | 11 +++++++++--
>  hw/pci/pcie.c        | 18 +++++++++---------
>  include/hw/pci/pci.h |  1 +
>  4 files changed, 48 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index b0bf540..c068248 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -847,6 +847,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>      PCIConfigWriteFunc *config_write = pc->config_write;
>      Error *local_err = NULL;
>      AddressSpace *dma_as;
> +    DeviceState *dev = DEVICE(pci_dev);
>  
>      if (devfn < 0) {
>          for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
> @@ -864,6 +865,16 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>                     PCI_SLOT(devfn), PCI_FUNC(devfn), name,
>                     bus->devices[devfn]->name);
>          return NULL;
> +    } else if (dev->hotplugged &&
> +               ((!pc->is_express && bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]) ||
> +                (pc->is_express && bus->devices[0]))) {

So let's change pci_is_function_0 to pci_get_function_0 and reuse here?

> +        error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
> +                   " new func %s cannot be exposed to guest.",
> +                   PCI_SLOT(devfn),
> +                   bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]->name,
> +                   name);
> +
> +       return NULL;
>      }
>  
>      pci_dev->bus = bus;
> @@ -2454,6 +2465,24 @@ void pci_bus_get_w64_range(PCIBus *bus, Range *range)
>      pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>  }
>  
> +/* ARI device function number range is 0-255, means has only 1 function0;
> + * while PCI device has 1 function0 in each slot(up to 32 slot) */
> +bool pci_is_function_0(PCIDevice *pci_dev)
> +{
> +    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
> +    bool is_func0 = false;
> +
> +    if (pc->is_express) {


This is not what the spec says. It says:
devices connected to an upstream port.


> +        if (!pci_dev->devfn)
> +            is_func0 = true;

Just do return !pci_dev->devfn here.

> +    } else {
> +        if(!PCI_FUNC(pci_dev->devfn))
> +            is_func0 = true;
> +    }
> +
> +    return is_func0;
> +}
> +
>  static const TypeInfo pci_device_type_info = {
>      .name = TYPE_PCI_DEVICE,
>      .parent = TYPE_DEVICE,
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index 3e26f92..40087c9 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -20,6 +20,7 @@
>  
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pci_host.h"
> +#include "hw/pci/pci_bus.h"
>  #include "trace.h"
>  
>  /* debug PCI */
> @@ -75,7 +76,10 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
>      PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
>      uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>  
> -    if (!pci_dev) {
> +    /* non-zero functions are only exposed when function 0 is present,
> +     * allowing direct removal of unexposed functions.
> +     */
> +    if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {

Reuse pci_get_function_0 here too?

>          return;
>      }
>  
> @@ -91,7 +95,10 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
>      uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>      uint32_t val;
>  
> -    if (!pci_dev) {
> +    /* non-zero functions are only exposed when function 0 is present,
> +     * allowing direct removal of unexposed functions.
> +     */
> +    if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {

And here?

>          return ~0x0;
>      }
>  
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index b1adeaf..d0a8006 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -249,16 +249,16 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>          return;
>      }
>  
> -    /* TODO: multifunction hot-plug.
> -     * Right now, only a device of function = 0 is allowed to be
> -     * hot plugged/unplugged.
> +    /* To enable multifunction hot-plug, we just ensure the function
> +     * 0 added last. Until function 0 added, we set the sltsta and
> +     * inform OS via event notification.

Should be "when function 0 is added".

>       */
> -    assert(PCI_FUNC(pci_dev->devfn) == 0);
> -
> -    pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> -                               PCI_EXP_SLTSTA_PDS);
> -    pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
> -                        PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
> +    if (pci_is_function_0(pci_dev)) {
> +        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> +                                   PCI_EXP_SLTSTA_PDS);
> +        pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
> +                            PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
> +    }
>  }
>  
>  static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index f5e7fd8..2820cfd 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -397,6 +397,7 @@ void pci_for_each_bus_depth_first(PCIBus *bus,
>                                    void *(*begin)(PCIBus *bus, void *parent_state),
>                                    void (*end)(PCIBus *bus, void *state),
>                                    void *parent_state);
> +bool pci_is_function_0(PCIDevice *pci_dev);
>  
>  /* Use this wrapper when specific scan order is not required. */
>  static inline
> -- 
> 2.1.0
Cao jin Oct. 23, 2015, 4:13 a.m. UTC | #2
Hi Michael

On 10/22/2015 10:37 PM, Michael S. Tsirkin wrote:
> On Thu, Oct 22, 2015 at 07:57:52PM +0800, Cao jin wrote:
>> Enable pcie device multifunction hot, just ensure the function 0
>> added last, then driver will got the notification to scan all the
>> function in the slot.
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/pci/pci.c         | 29 +++++++++++++++++++++++++++++
>>   hw/pci/pci_host.c    | 11 +++++++++--
>>   hw/pci/pcie.c        | 18 +++++++++---------
>>   include/hw/pci/pci.h |  1 +
>>   4 files changed, 48 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index b0bf540..c068248 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -847,6 +847,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>>       PCIConfigWriteFunc *config_write = pc->config_write;
>>       Error *local_err = NULL;
>>       AddressSpace *dma_as;
>> +    DeviceState *dev = DEVICE(pci_dev);
>>
>>       if (devfn < 0) {
>>           for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
>> @@ -864,6 +865,16 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>>                      PCI_SLOT(devfn), PCI_FUNC(devfn), name,
>>                      bus->devices[devfn]->name);
>>           return NULL;
>> +    } else if (dev->hotplugged &&
>> +               ((!pc->is_express && bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]) ||
>> +                (pc->is_express && bus->devices[0]))) {
>
> So let's change pci_is_function_0 to pci_get_function_0 and reuse here?

ok, will modify it and all the following comment.

>
>> +        error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
>> +                   " new func %s cannot be exposed to guest.",
>> +                   PCI_SLOT(devfn),
>> +                   bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]->name,
>> +                   name);
>> +
>> +       return NULL;
>>       }
>>
>>       pci_dev->bus = bus;
>> @@ -2454,6 +2465,24 @@ void pci_bus_get_w64_range(PCIBus *bus, Range *range)
>>       pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>>   }
>>
>> +/* ARI device function number range is 0-255, means has only 1 function0;
>> + * while PCI device has 1 function0 in each slot(up to 32 slot) */
>> +bool pci_is_function_0(PCIDevice *pci_dev)
>> +{
>> +    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
>> +    bool is_func0 = false;
>> +
>> +    if (pc->is_express) {
>
>
> This is not what the spec says. It says:
> devices connected to an upstream port.
>
Yes, I am wrong here. I got confused about the ARI device definition in 
spec:"a device associated with an Upstream Port". Because as I 
understand, ARI device is a EP immediately below a ARI downstream port, 
just as Figure 6-13(Example System Topology with ARI Devices) shows in 
the spec, but where is upstream port in the definition come from, what 
the relationship between upstream port and a ARI device?
>
>> +        if (!pci_dev->devfn)
>> +            is_func0 = true;
>
> Just do return !pci_dev->devfn here.
>
>> +    } else {
>> +        if(!PCI_FUNC(pci_dev->devfn))
>> +            is_func0 = true;
>> +    }
>> +
>> +    return is_func0;
>> +}
>> +
>>   static const TypeInfo pci_device_type_info = {
>>       .name = TYPE_PCI_DEVICE,
>>       .parent = TYPE_DEVICE,
>> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
>> index 3e26f92..40087c9 100644
>> --- a/hw/pci/pci_host.c
>> +++ b/hw/pci/pci_host.c
>> @@ -20,6 +20,7 @@
>>
>>   #include "hw/pci/pci.h"
>>   #include "hw/pci/pci_host.h"
>> +#include "hw/pci/pci_bus.h"
>>   #include "trace.h"
>>
>>   /* debug PCI */
>> @@ -75,7 +76,10 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
>>       PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
>>       uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>>
>> -    if (!pci_dev) {
>> +    /* non-zero functions are only exposed when function 0 is present,
>> +     * allowing direct removal of unexposed functions.
>> +     */
>> +    if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
>
> Reuse pci_get_function_0 here too?
>
>>           return;
>>       }
>>
>> @@ -91,7 +95,10 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
>>       uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>>       uint32_t val;
>>
>> -    if (!pci_dev) {
>> +    /* non-zero functions are only exposed when function 0 is present,
>> +     * allowing direct removal of unexposed functions.
>> +     */
>> +    if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
>
> And here?
>
>>           return ~0x0;
>>       }
>>
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index b1adeaf..d0a8006 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -249,16 +249,16 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>>           return;
>>       }
>>
>> -    /* TODO: multifunction hot-plug.
>> -     * Right now, only a device of function = 0 is allowed to be
>> -     * hot plugged/unplugged.
>> +    /* To enable multifunction hot-plug, we just ensure the function
>> +     * 0 added last. Until function 0 added, we set the sltsta and
>> +     * inform OS via event notification.
>
> Should be "when function 0 is added".
>
>>        */
>> -    assert(PCI_FUNC(pci_dev->devfn) == 0);
>> -
>> -    pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
>> -                               PCI_EXP_SLTSTA_PDS);
>> -    pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
>> -                        PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
>> +    if (pci_is_function_0(pci_dev)) {
>> +        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
>> +                                   PCI_EXP_SLTSTA_PDS);
>> +        pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
>> +                            PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
>> +    }
>>   }
>>
>>   static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index f5e7fd8..2820cfd 100644
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -397,6 +397,7 @@ void pci_for_each_bus_depth_first(PCIBus *bus,
>>                                     void *(*begin)(PCIBus *bus, void *parent_state),
>>                                     void (*end)(PCIBus *bus, void *state),
>>                                     void *parent_state);
>> +bool pci_is_function_0(PCIDevice *pci_dev);
>>
>>   /* Use this wrapper when specific scan order is not required. */
>>   static inline
>> --
>> 2.1.0
> .
>
Michael S. Tsirkin Oct. 23, 2015, 6:50 a.m. UTC | #3
On Fri, Oct 23, 2015 at 12:13:53PM +0800, Cao jin wrote:
> Hi Michael
> 
> On 10/22/2015 10:37 PM, Michael S. Tsirkin wrote:
> >On Thu, Oct 22, 2015 at 07:57:52PM +0800, Cao jin wrote:
> >>Enable pcie device multifunction hot, just ensure the function 0
> >>added last, then driver will got the notification to scan all the
> >>function in the slot.
> >>
> >>Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> >>---
> >>  hw/pci/pci.c         | 29 +++++++++++++++++++++++++++++
> >>  hw/pci/pci_host.c    | 11 +++++++++--
> >>  hw/pci/pcie.c        | 18 +++++++++---------
> >>  include/hw/pci/pci.h |  1 +
> >>  4 files changed, 48 insertions(+), 11 deletions(-)
> >>
> >>diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >>index b0bf540..c068248 100644
> >>--- a/hw/pci/pci.c
> >>+++ b/hw/pci/pci.c
> >>@@ -847,6 +847,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> >>      PCIConfigWriteFunc *config_write = pc->config_write;
> >>      Error *local_err = NULL;
> >>      AddressSpace *dma_as;
> >>+    DeviceState *dev = DEVICE(pci_dev);
> >>
> >>      if (devfn < 0) {
> >>          for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
> >>@@ -864,6 +865,16 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> >>                     PCI_SLOT(devfn), PCI_FUNC(devfn), name,
> >>                     bus->devices[devfn]->name);
> >>          return NULL;
> >>+    } else if (dev->hotplugged &&
> >>+               ((!pc->is_express && bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]) ||
> >>+                (pc->is_express && bus->devices[0]))) {
> >
> >So let's change pci_is_function_0 to pci_get_function_0 and reuse here?
> 
> ok, will modify it and all the following comment.
> 
> >
> >>+        error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
> >>+                   " new func %s cannot be exposed to guest.",
> >>+                   PCI_SLOT(devfn),
> >>+                   bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]->name,
> >>+                   name);
> >>+
> >>+       return NULL;
> >>      }
> >>
> >>      pci_dev->bus = bus;
> >>@@ -2454,6 +2465,24 @@ void pci_bus_get_w64_range(PCIBus *bus, Range *range)
> >>      pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> >>  }
> >>
> >>+/* ARI device function number range is 0-255, means has only 1 function0;
> >>+ * while PCI device has 1 function0 in each slot(up to 32 slot) */
> >>+bool pci_is_function_0(PCIDevice *pci_dev)
> >>+{
> >>+    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
> >>+    bool is_func0 = false;
> >>+
> >>+    if (pc->is_express) {
> >
> >
> >This is not what the spec says. It says:
> >devices connected to an upstream port.
> >
> Yes, I am wrong here. I got confused about the ARI device definition in
> spec:"a device associated with an Upstream Port". Because as I understand,
> ARI device is a EP immediately below a ARI downstream port, just as Figure
> 6-13(Example System Topology with ARI Devices) shows in the spec, but where
> is upstream port in the definition come from, what the relationship between
> upstream port and a ARI device?

See Terms and Acronyms:
	The Port on a
	component that contains only Endpoint or Bridge Functions is an Upstream
	Port.


> >
> >>+        if (!pci_dev->devfn)
> >>+            is_func0 = true;
> >
> >Just do return !pci_dev->devfn here.
> >
> >>+    } else {
> >>+        if(!PCI_FUNC(pci_dev->devfn))
> >>+            is_func0 = true;
> >>+    }
> >>+
> >>+    return is_func0;
> >>+}
> >>+
> >>  static const TypeInfo pci_device_type_info = {
> >>      .name = TYPE_PCI_DEVICE,
> >>      .parent = TYPE_DEVICE,
> >>diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> >>index 3e26f92..40087c9 100644
> >>--- a/hw/pci/pci_host.c
> >>+++ b/hw/pci/pci_host.c
> >>@@ -20,6 +20,7 @@
> >>
> >>  #include "hw/pci/pci.h"
> >>  #include "hw/pci/pci_host.h"
> >>+#include "hw/pci/pci_bus.h"
> >>  #include "trace.h"
> >>
> >>  /* debug PCI */
> >>@@ -75,7 +76,10 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
> >>      PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
> >>      uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
> >>
> >>-    if (!pci_dev) {
> >>+    /* non-zero functions are only exposed when function 0 is present,
> >>+     * allowing direct removal of unexposed functions.
> >>+     */
> >>+    if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
> >
> >Reuse pci_get_function_0 here too?
> >
> >>          return;
> >>      }
> >>
> >>@@ -91,7 +95,10 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
> >>      uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
> >>      uint32_t val;
> >>
> >>-    if (!pci_dev) {
> >>+    /* non-zero functions are only exposed when function 0 is present,
> >>+     * allowing direct removal of unexposed functions.
> >>+     */
> >>+    if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
> >
> >And here?
> >
> >>          return ~0x0;
> >>      }
> >>
> >>diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> >>index b1adeaf..d0a8006 100644
> >>--- a/hw/pci/pcie.c
> >>+++ b/hw/pci/pcie.c
> >>@@ -249,16 +249,16 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> >>          return;
> >>      }
> >>
> >>-    /* TODO: multifunction hot-plug.
> >>-     * Right now, only a device of function = 0 is allowed to be
> >>-     * hot plugged/unplugged.
> >>+    /* To enable multifunction hot-plug, we just ensure the function
> >>+     * 0 added last. Until function 0 added, we set the sltsta and
> >>+     * inform OS via event notification.
> >
> >Should be "when function 0 is added".
> >
> >>       */
> >>-    assert(PCI_FUNC(pci_dev->devfn) == 0);
> >>-
> >>-    pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> >>-                               PCI_EXP_SLTSTA_PDS);
> >>-    pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
> >>-                        PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
> >>+    if (pci_is_function_0(pci_dev)) {
> >>+        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> >>+                                   PCI_EXP_SLTSTA_PDS);
> >>+        pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
> >>+                            PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
> >>+    }
> >>  }
> >>
> >>  static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
> >>diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> >>index f5e7fd8..2820cfd 100644
> >>--- a/include/hw/pci/pci.h
> >>+++ b/include/hw/pci/pci.h
> >>@@ -397,6 +397,7 @@ void pci_for_each_bus_depth_first(PCIBus *bus,
> >>                                    void *(*begin)(PCIBus *bus, void *parent_state),
> >>                                    void (*end)(PCIBus *bus, void *state),
> >>                                    void *parent_state);
> >>+bool pci_is_function_0(PCIDevice *pci_dev);
> >>
> >>  /* Use this wrapper when specific scan order is not required. */
> >>  static inline
> >>--
> >>2.1.0
> >.
> >
> 
> -- 
> Yours Sincerely,
> 
> Cao Jin
Cao jin Oct. 23, 2015, 8:14 a.m. UTC | #4
On 10/23/2015 02:50 PM, Michael S. Tsirkin wrote:
> On Fri, Oct 23, 2015 at 12:13:53PM +0800, Cao jin wrote:
>> Hi Michael
>>
>> On 10/22/2015 10:37 PM, Michael S. Tsirkin wrote:
>>> On Thu, Oct 22, 2015 at 07:57:52PM +0800, Cao jin wrote:
>>>> Enable pcie device multifunction hot, just ensure the function 0
>>>> added last, then driver will got the notification to scan all the
>>>> function in the slot.
>>>>
>>>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>>>> ---
>>>>   hw/pci/pci.c         | 29 +++++++++++++++++++++++++++++
>>>>   hw/pci/pci_host.c    | 11 +++++++++--
>>>>   hw/pci/pcie.c        | 18 +++++++++---------
>>>>   include/hw/pci/pci.h |  1 +
>>>>   4 files changed, 48 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>> index b0bf540..c068248 100644
>>>> --- a/hw/pci/pci.c
>>>> +++ b/hw/pci/pci.c
>>>> @@ -847,6 +847,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>>>>       PCIConfigWriteFunc *config_write = pc->config_write;
>>>>       Error *local_err = NULL;
>>>>       AddressSpace *dma_as;
>>>> +    DeviceState *dev = DEVICE(pci_dev);
>>>>
>>>>       if (devfn < 0) {
>>>>           for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
>>>> @@ -864,6 +865,16 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>>>>                      PCI_SLOT(devfn), PCI_FUNC(devfn), name,
>>>>                      bus->devices[devfn]->name);
>>>>           return NULL;
>>>> +    } else if (dev->hotplugged &&
>>>> +               ((!pc->is_express && bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]) ||
>>>> +                (pc->is_express && bus->devices[0]))) {
>>>
>>> So let's change pci_is_function_0 to pci_get_function_0 and reuse here?
>>
>> ok, will modify it and all the following comment.
>>
>>>
>>>> +        error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
>>>> +                   " new func %s cannot be exposed to guest.",
>>>> +                   PCI_SLOT(devfn),
>>>> +                   bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]->name,
>>>> +                   name);
>>>> +
>>>> +       return NULL;
>>>>       }
>>>>
>>>>       pci_dev->bus = bus;
>>>> @@ -2454,6 +2465,24 @@ void pci_bus_get_w64_range(PCIBus *bus, Range *range)
>>>>       pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>>>>   }
>>>>
>>>> +/* ARI device function number range is 0-255, means has only 1 function0;
>>>> + * while PCI device has 1 function0 in each slot(up to 32 slot) */
>>>> +bool pci_is_function_0(PCIDevice *pci_dev)
>>>> +{
>>>> +    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
>>>> +    bool is_func0 = false;
>>>> +
>>>> +    if (pc->is_express) {
>>>
>>>
>>> This is not what the spec says. It says:
>>> devices connected to an upstream port.
>>>
>> Yes, I am wrong here. I got confused about the ARI device definition in
>> spec:"a device associated with an Upstream Port". Because as I understand,
>> ARI device is a EP immediately below a ARI downstream port, just as Figure
>> 6-13(Example System Topology with ARI Devices) shows in the spec, but where
>> is upstream port in the definition come from, what the relationship between
>> upstream port and a ARI device?
>
> See Terms and Acronyms:
> 	The Port on a
> 	component that contains only Endpoint or Bridge Functions is an Upstream
> 	Port.
>
Thanks michael. if I understand right, ARI device has a upstream port on 
the device, while on-ARI device doesn`t have?
But in Figure 6-13(Example System Topology with ARI Devices) in the PCIe 
spec, don`t see any notation of upstream port on ARI Device X & Y, so 
maybe figure 6-13 is incomplete?
>
>>>
>>>> +        if (!pci_dev->devfn)
>>>> +            is_func0 = true;
>>>
>>> Just do return !pci_dev->devfn here.
>>>
>>>> +    } else {
>>>> +        if(!PCI_FUNC(pci_dev->devfn))
>>>> +            is_func0 = true;
>>>> +    }
>>>> +
>>>> +    return is_func0;
>>>> +}
>>>> +
>>>>   static const TypeInfo pci_device_type_info = {
>>>>       .name = TYPE_PCI_DEVICE,
>>>>       .parent = TYPE_DEVICE,
>>>> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
>>>> index 3e26f92..40087c9 100644
>>>> --- a/hw/pci/pci_host.c
>>>> +++ b/hw/pci/pci_host.c
>>>> @@ -20,6 +20,7 @@
>>>>
>>>>   #include "hw/pci/pci.h"
>>>>   #include "hw/pci/pci_host.h"
>>>> +#include "hw/pci/pci_bus.h"
>>>>   #include "trace.h"
>>>>
>>>>   /* debug PCI */
>>>> @@ -75,7 +76,10 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
>>>>       PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
>>>>       uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>>>>
>>>> -    if (!pci_dev) {
>>>> +    /* non-zero functions are only exposed when function 0 is present,
>>>> +     * allowing direct removal of unexposed functions.
>>>> +     */
>>>> +    if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
>>>
>>> Reuse pci_get_function_0 here too?
>>>
>>>>           return;
>>>>       }
>>>>
>>>> @@ -91,7 +95,10 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
>>>>       uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
>>>>       uint32_t val;
>>>>
>>>> -    if (!pci_dev) {
>>>> +    /* non-zero functions are only exposed when function 0 is present,
>>>> +     * allowing direct removal of unexposed functions.
>>>> +     */
>>>> +    if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
>>>
>>> And here?
>>>
>>>>           return ~0x0;
>>>>       }
>>>>
>>>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>>>> index b1adeaf..d0a8006 100644
>>>> --- a/hw/pci/pcie.c
>>>> +++ b/hw/pci/pcie.c
>>>> @@ -249,16 +249,16 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>>>>           return;
>>>>       }
>>>>
>>>> -    /* TODO: multifunction hot-plug.
>>>> -     * Right now, only a device of function = 0 is allowed to be
>>>> -     * hot plugged/unplugged.
>>>> +    /* To enable multifunction hot-plug, we just ensure the function
>>>> +     * 0 added last. Until function 0 added, we set the sltsta and
>>>> +     * inform OS via event notification.
>>>
>>> Should be "when function 0 is added".
>>>
>>>>        */
>>>> -    assert(PCI_FUNC(pci_dev->devfn) == 0);
>>>> -
>>>> -    pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
>>>> -                               PCI_EXP_SLTSTA_PDS);
>>>> -    pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
>>>> -                        PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
>>>> +    if (pci_is_function_0(pci_dev)) {
>>>> +        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
>>>> +                                   PCI_EXP_SLTSTA_PDS);
>>>> +        pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
>>>> +                            PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
>>>> +    }
>>>>   }
>>>>
>>>>   static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
>>>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>>>> index f5e7fd8..2820cfd 100644
>>>> --- a/include/hw/pci/pci.h
>>>> +++ b/include/hw/pci/pci.h
>>>> @@ -397,6 +397,7 @@ void pci_for_each_bus_depth_first(PCIBus *bus,
>>>>                                     void *(*begin)(PCIBus *bus, void *parent_state),
>>>>                                     void (*end)(PCIBus *bus, void *state),
>>>>                                     void *parent_state);
>>>> +bool pci_is_function_0(PCIDevice *pci_dev);
>>>>
>>>>   /* Use this wrapper when specific scan order is not required. */
>>>>   static inline
>>>> --
>>>> 2.1.0
>>> .
>>>
>>
>> --
>> Yours Sincerely,
>>
>> Cao Jin
> .
>
Michael S. Tsirkin Oct. 23, 2015, 1:41 p.m. UTC | #5
On Fri, Oct 23, 2015 at 04:14:07PM +0800, Cao jin wrote:
> 
> 
> On 10/23/2015 02:50 PM, Michael S. Tsirkin wrote:
> >On Fri, Oct 23, 2015 at 12:13:53PM +0800, Cao jin wrote:
> >>Hi Michael
> >>
> >>On 10/22/2015 10:37 PM, Michael S. Tsirkin wrote:
> >>>On Thu, Oct 22, 2015 at 07:57:52PM +0800, Cao jin wrote:
> >>>>Enable pcie device multifunction hot, just ensure the function 0
> >>>>added last, then driver will got the notification to scan all the
> >>>>function in the slot.
> >>>>
> >>>>Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> >>>>---
> >>>>  hw/pci/pci.c         | 29 +++++++++++++++++++++++++++++
> >>>>  hw/pci/pci_host.c    | 11 +++++++++--
> >>>>  hw/pci/pcie.c        | 18 +++++++++---------
> >>>>  include/hw/pci/pci.h |  1 +
> >>>>  4 files changed, 48 insertions(+), 11 deletions(-)
> >>>>
> >>>>diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >>>>index b0bf540..c068248 100644
> >>>>--- a/hw/pci/pci.c
> >>>>+++ b/hw/pci/pci.c
> >>>>@@ -847,6 +847,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> >>>>      PCIConfigWriteFunc *config_write = pc->config_write;
> >>>>      Error *local_err = NULL;
> >>>>      AddressSpace *dma_as;
> >>>>+    DeviceState *dev = DEVICE(pci_dev);
> >>>>
> >>>>      if (devfn < 0) {
> >>>>          for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
> >>>>@@ -864,6 +865,16 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> >>>>                     PCI_SLOT(devfn), PCI_FUNC(devfn), name,
> >>>>                     bus->devices[devfn]->name);
> >>>>          return NULL;
> >>>>+    } else if (dev->hotplugged &&
> >>>>+               ((!pc->is_express && bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]) ||
> >>>>+                (pc->is_express && bus->devices[0]))) {
> >>>
> >>>So let's change pci_is_function_0 to pci_get_function_0 and reuse here?
> >>
> >>ok, will modify it and all the following comment.
> >>
> >>>
> >>>>+        error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
> >>>>+                   " new func %s cannot be exposed to guest.",
> >>>>+                   PCI_SLOT(devfn),
> >>>>+                   bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]->name,
> >>>>+                   name);
> >>>>+
> >>>>+       return NULL;
> >>>>      }
> >>>>
> >>>>      pci_dev->bus = bus;
> >>>>@@ -2454,6 +2465,24 @@ void pci_bus_get_w64_range(PCIBus *bus, Range *range)
> >>>>      pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> >>>>  }
> >>>>
> >>>>+/* ARI device function number range is 0-255, means has only 1 function0;
> >>>>+ * while PCI device has 1 function0 in each slot(up to 32 slot) */
> >>>>+bool pci_is_function_0(PCIDevice *pci_dev)
> >>>>+{
> >>>>+    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
> >>>>+    bool is_func0 = false;
> >>>>+
> >>>>+    if (pc->is_express) {
> >>>
> >>>
> >>>This is not what the spec says. It says:
> >>>devices connected to an upstream port.
> >>>
> >>Yes, I am wrong here. I got confused about the ARI device definition in
> >>spec:"a device associated with an Upstream Port". Because as I understand,
> >>ARI device is a EP immediately below a ARI downstream port, just as Figure
> >>6-13(Example System Topology with ARI Devices) shows in the spec, but where
> >>is upstream port in the definition come from, what the relationship between
> >>upstream port and a ARI device?
> >
> >See Terms and Acronyms:
> >	The Port on a
> >	component that contains only Endpoint or Bridge Functions is an Upstream
> >	Port.
> >
> Thanks michael. if I understand right, ARI device has a upstream port on the
> device, while on-ARI device doesn`t have?

That's what it says, isn't it?

> But in Figure 6-13(Example System Topology with ARI Devices) in the PCIe
> spec, don`t see any notation of upstream port on ARI Device X & Y, so maybe
> figure 6-13 is incomplete?

You can see that they are connected to downstream or root ports.

> >
> >>>
> >>>>+        if (!pci_dev->devfn)
> >>>>+            is_func0 = true;
> >>>
> >>>Just do return !pci_dev->devfn here.
> >>>
> >>>>+    } else {
> >>>>+        if(!PCI_FUNC(pci_dev->devfn))
> >>>>+            is_func0 = true;
> >>>>+    }
> >>>>+
> >>>>+    return is_func0;
> >>>>+}
> >>>>+
> >>>>  static const TypeInfo pci_device_type_info = {
> >>>>      .name = TYPE_PCI_DEVICE,
> >>>>      .parent = TYPE_DEVICE,
> >>>>diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> >>>>index 3e26f92..40087c9 100644
> >>>>--- a/hw/pci/pci_host.c
> >>>>+++ b/hw/pci/pci_host.c
> >>>>@@ -20,6 +20,7 @@
> >>>>
> >>>>  #include "hw/pci/pci.h"
> >>>>  #include "hw/pci/pci_host.h"
> >>>>+#include "hw/pci/pci_bus.h"
> >>>>  #include "trace.h"
> >>>>
> >>>>  /* debug PCI */
> >>>>@@ -75,7 +76,10 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
> >>>>      PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
> >>>>      uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
> >>>>
> >>>>-    if (!pci_dev) {
> >>>>+    /* non-zero functions are only exposed when function 0 is present,
> >>>>+     * allowing direct removal of unexposed functions.
> >>>>+     */
> >>>>+    if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
> >>>
> >>>Reuse pci_get_function_0 here too?
> >>>
> >>>>          return;
> >>>>      }
> >>>>
> >>>>@@ -91,7 +95,10 @@ uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
> >>>>      uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
> >>>>      uint32_t val;
> >>>>
> >>>>-    if (!pci_dev) {
> >>>>+    /* non-zero functions are only exposed when function 0 is present,
> >>>>+     * allowing direct removal of unexposed functions.
> >>>>+     */
> >>>>+    if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
> >>>
> >>>And here?
> >>>
> >>>>          return ~0x0;
> >>>>      }
> >>>>
> >>>>diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> >>>>index b1adeaf..d0a8006 100644
> >>>>--- a/hw/pci/pcie.c
> >>>>+++ b/hw/pci/pcie.c
> >>>>@@ -249,16 +249,16 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> >>>>          return;
> >>>>      }
> >>>>
> >>>>-    /* TODO: multifunction hot-plug.
> >>>>-     * Right now, only a device of function = 0 is allowed to be
> >>>>-     * hot plugged/unplugged.
> >>>>+    /* To enable multifunction hot-plug, we just ensure the function
> >>>>+     * 0 added last. Until function 0 added, we set the sltsta and
> >>>>+     * inform OS via event notification.
> >>>
> >>>Should be "when function 0 is added".
> >>>
> >>>>       */
> >>>>-    assert(PCI_FUNC(pci_dev->devfn) == 0);
> >>>>-
> >>>>-    pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> >>>>-                               PCI_EXP_SLTSTA_PDS);
> >>>>-    pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
> >>>>-                        PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
> >>>>+    if (pci_is_function_0(pci_dev)) {
> >>>>+        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> >>>>+                                   PCI_EXP_SLTSTA_PDS);
> >>>>+        pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
> >>>>+                            PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
> >>>>+    }
> >>>>  }
> >>>>
> >>>>  static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
> >>>>diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> >>>>index f5e7fd8..2820cfd 100644
> >>>>--- a/include/hw/pci/pci.h
> >>>>+++ b/include/hw/pci/pci.h
> >>>>@@ -397,6 +397,7 @@ void pci_for_each_bus_depth_first(PCIBus *bus,
> >>>>                                    void *(*begin)(PCIBus *bus, void *parent_state),
> >>>>                                    void (*end)(PCIBus *bus, void *state),
> >>>>                                    void *parent_state);
> >>>>+bool pci_is_function_0(PCIDevice *pci_dev);
> >>>>
> >>>>  /* Use this wrapper when specific scan order is not required. */
> >>>>  static inline
> >>>>--
> >>>>2.1.0
> >>>.
> >>>
> >>
> >>--
> >>Yours Sincerely,
> >>
> >>Cao Jin
> >.
> >
> 
> -- 
> Yours Sincerely,
> 
> Cao Jin
Cao jin Oct. 23, 2015, 2:12 p.m. UTC | #6
On 10/23/2015 09:41 PM, Michael S. Tsirkin wrote:
> On Fri, Oct 23, 2015 at 04:14:07PM +0800, Cao jin wrote:
>>
>>
>> On 10/23/2015 02:50 PM, Michael S. Tsirkin wrote:
>>> On Fri, Oct 23, 2015 at 12:13:53PM +0800, Cao jin wrote:
>>>> Hi Michael
>>>>
>>>> On 10/22/2015 10:37 PM, Michael S. Tsirkin wrote:
>>>>> On Thu, Oct 22, 2015 at 07:57:52PM +0800, Cao jin wrote:
>>>>>> Enable pcie device multifunction hot, just ensure the function 0
>>>>>> added last, then driver will got the notification to scan all the
>>>>>> function in the slot.
>>>>>>
>>>>>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>>>>>> ---
>>>>>>   hw/pci/pci.c         | 29 +++++++++++++++++++++++++++++
>>>>>>   hw/pci/pci_host.c    | 11 +++++++++--
>>>>>>   hw/pci/pcie.c        | 18 +++++++++---------
>>>>>>   include/hw/pci/pci.h |  1 +
>>>>>>   4 files changed, 48 insertions(+), 11 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>>>> index b0bf540..c068248 100644
>>>>>> --- a/hw/pci/pci.c
>>>>>> +++ b/hw/pci/pci.c
>>>>>> @@ -847,6 +847,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>>>>>>       PCIConfigWriteFunc *config_write = pc->config_write;
>>>>>>       Error *local_err = NULL;
>>>>>>       AddressSpace *dma_as;
>>>>>> +    DeviceState *dev = DEVICE(pci_dev);
>>>>>>
>>>>>>       if (devfn < 0) {
>>>>>>           for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
>>>>>> @@ -864,6 +865,16 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>>>>>>                      PCI_SLOT(devfn), PCI_FUNC(devfn), name,
>>>>>>                      bus->devices[devfn]->name);
>>>>>>           return NULL;
>>>>>> +    } else if (dev->hotplugged &&
>>>>>> +               ((!pc->is_express && bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]) ||
>>>>>> +                (pc->is_express && bus->devices[0]))) {
>>>>>
>>>>> So let's change pci_is_function_0 to pci_get_function_0 and reuse here?
>>>>
>>>> ok, will modify it and all the following comment.
>>>>
>>>>>
>>>>>> +        error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
>>>>>> +                   " new func %s cannot be exposed to guest.",
>>>>>> +                   PCI_SLOT(devfn),
>>>>>> +                   bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]->name,
>>>>>> +                   name);
>>>>>> +
>>>>>> +       return NULL;
>>>>>>       }
>>>>>>
>>>>>>       pci_dev->bus = bus;
>>>>>> @@ -2454,6 +2465,24 @@ void pci_bus_get_w64_range(PCIBus *bus, Range *range)
>>>>>>       pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>>>>>>   }
>>>>>>
>>>>>> +/* ARI device function number range is 0-255, means has only 1 function0;
>>>>>> + * while PCI device has 1 function0 in each slot(up to 32 slot) */
>>>>>> +bool pci_is_function_0(PCIDevice *pci_dev)
>>>>>> +{
>>>>>> +    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
>>>>>> +    bool is_func0 = false;
>>>>>> +
>>>>>> +    if (pc->is_express) {
>>>>>
>>>>>
>>>>> This is not what the spec says. It says:
>>>>> devices connected to an upstream port.
>>>>>
>>>> Yes, I am wrong here. I got confused about the ARI device definition in
>>>> spec:"a device associated with an Upstream Port". Because as I understand,
>>>> ARI device is a EP immediately below a ARI downstream port, just as Figure
>>>> 6-13(Example System Topology with ARI Devices) shows in the spec, but where
>>>> is upstream port in the definition come from, what the relationship between
>>>> upstream port and a ARI device?
>>>
>>> See Terms and Acronyms:
>>> 	The Port on a
>>> 	component that contains only Endpoint or Bridge Functions is an Upstream
>>> 	Port.
>>>
>> Thanks michael. if I understand right, ARI device has a upstream port on the
>> device, while on-ARI device doesn`t have?
>
> That's what it says, isn't it?
>

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

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

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

Patch

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b0bf540..c068248 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -847,6 +847,7 @@  static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
     PCIConfigWriteFunc *config_write = pc->config_write;
     Error *local_err = NULL;
     AddressSpace *dma_as;
+    DeviceState *dev = DEVICE(pci_dev);
 
     if (devfn < 0) {
         for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
@@ -864,6 +865,16 @@  static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
                    PCI_SLOT(devfn), PCI_FUNC(devfn), name,
                    bus->devices[devfn]->name);
         return NULL;
+    } else if (dev->hotplugged &&
+               ((!pc->is_express && bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]) ||
+                (pc->is_express && bus->devices[0]))) {
+        error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
+                   " new func %s cannot be exposed to guest.",
+                   PCI_SLOT(devfn),
+                   bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]->name,
+                   name);
+
+       return NULL;
     }
 
     pci_dev->bus = bus;
@@ -2454,6 +2465,24 @@  void pci_bus_get_w64_range(PCIBus *bus, Range *range)
     pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
 }
 
+/* ARI device function number range is 0-255, means has only 1 function0;
+ * while PCI device has 1 function0 in each slot(up to 32 slot) */
+bool pci_is_function_0(PCIDevice *pci_dev)
+{
+    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
+    bool is_func0 = false;
+
+    if (pc->is_express) {
+        if (!pci_dev->devfn)
+            is_func0 = true;
+    } else {
+        if(!PCI_FUNC(pci_dev->devfn))
+            is_func0 = true;
+    }
+
+    return is_func0;
+}
+
 static const TypeInfo pci_device_type_info = {
     .name = TYPE_PCI_DEVICE,
     .parent = TYPE_DEVICE,
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index 3e26f92..40087c9 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -20,6 +20,7 @@ 
 
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_host.h"
+#include "hw/pci/pci_bus.h"
 #include "trace.h"
 
 /* debug PCI */
@@ -75,7 +76,10 @@  void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
     PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
     uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
 
-    if (!pci_dev) {
+    /* non-zero functions are only exposed when function 0 is present,
+     * allowing direct removal of unexposed functions.
+     */
+    if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
         return;
     }
 
@@ -91,7 +95,10 @@  uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
     uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
     uint32_t val;
 
-    if (!pci_dev) {
+    /* non-zero functions are only exposed when function 0 is present,
+     * allowing direct removal of unexposed functions.
+     */
+    if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) {
         return ~0x0;
     }
 
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index b1adeaf..d0a8006 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -249,16 +249,16 @@  void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
         return;
     }
 
-    /* TODO: multifunction hot-plug.
-     * Right now, only a device of function = 0 is allowed to be
-     * hot plugged/unplugged.
+    /* To enable multifunction hot-plug, we just ensure the function
+     * 0 added last. Until function 0 added, we set the sltsta and
+     * inform OS via event notification.
      */
-    assert(PCI_FUNC(pci_dev->devfn) == 0);
-
-    pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
-                               PCI_EXP_SLTSTA_PDS);
-    pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
-                        PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
+    if (pci_is_function_0(pci_dev)) {
+        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
+                                   PCI_EXP_SLTSTA_PDS);
+        pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
+                            PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
+    }
 }
 
 static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index f5e7fd8..2820cfd 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -397,6 +397,7 @@  void pci_for_each_bus_depth_first(PCIBus *bus,
                                   void *(*begin)(PCIBus *bus, void *parent_state),
                                   void (*end)(PCIBus *bus, void *state),
                                   void *parent_state);
+bool pci_is_function_0(PCIDevice *pci_dev);
 
 /* Use this wrapper when specific scan order is not required. */
 static inline