* [Qemu-devel] [PATCH v4 0/2] PCI-e device multi-function hot-add support
@ 2015-10-22 11:57 Cao jin
2015-10-22 11:57 ` [Qemu-devel] [PATCH v4 1/2] remove function during multi-function hot-add Cao jin
2015-10-22 11:57 ` [Qemu-devel] [PATCH v4 2/2] enable " Cao jin
0 siblings, 2 replies; 9+ messages in thread
From: Cao jin @ 2015-10-22 11:57 UTC (permalink / raw)
To: qemu-devel; +Cc: izumi.taku, alex.williamson, mst
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 cancle the operation.
changelog v4:
1. take ari into account, add api: pci_is_function_0()
2. reorder the patch
3. other minor fix according to the v3 comment
changelog v3:
1. Flag device as unexposed when func 0 doesn`t exist, via return 0xFF
in case of gratuitous pci bus scan.
2. Since device is unexposed to guest, can remove function individually,
without interaction with the guest.
Cao jin (2):
remove function during multi-function hot-add
enable multi-function hot-add
hw/pci/pci.c | 29 +++++++++++++++++++++++++++++
hw/pci/pci_host.c | 11 +++++++++--
hw/pci/pcie.c | 39 ++++++++++++++++++++++++++-------------
include/hw/pci/pci.h | 1 +
4 files changed, 65 insertions(+), 15 deletions(-)
--
2.1.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v4 1/2] remove function during multi-function hot-add
2015-10-22 11:57 [Qemu-devel] [PATCH v4 0/2] PCI-e device multi-function hot-add support Cao jin
@ 2015-10-22 11:57 ` Cao jin
2015-10-22 11:57 ` [Qemu-devel] [PATCH v4 2/2] enable " Cao jin
1 sibling, 0 replies; 9+ messages in thread
From: Cao jin @ 2015-10-22 11:57 UTC (permalink / raw)
To: qemu-devel; +Cc: izumi.taku, alex.williamson, mst
In case user want to cancel the operation, 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 | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 6e28985..b1adeaf 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -261,13 +261,31 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
}
+static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
+{
+ object_unparent(OBJECT(dev));
+}
+
void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
{
uint8_t *exp_cap;
+ PCIDevice *pci_dev = PCI_DEVICE(dev);
+ PCIBus *bus = pci_dev->bus;
pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp);
+ /* In case user cancel the operation of multi-function hot-add,
+ * remove the function that is unexposed to guest individually,
+ * without interaction with guest.
+ */
+ if (PCI_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 +396,6 @@ void pcie_cap_slot_reset(PCIDevice *dev)
hotplug_event_update_event_status(dev);
}
-static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
-{
- object_unparent(OBJECT(dev));
-}
-
void pcie_cap_slot_write_config(PCIDevice *dev,
uint32_t addr, uint32_t val, int len)
{
--
2.1.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v4 2/2] enable multi-function hot-add
2015-10-22 11:57 [Qemu-devel] [PATCH v4 0/2] PCI-e device multi-function hot-add support Cao jin
2015-10-22 11:57 ` [Qemu-devel] [PATCH v4 1/2] remove function during multi-function hot-add Cao jin
@ 2015-10-22 11:57 ` Cao jin
2015-10-22 14:37 ` Michael S. Tsirkin
1 sibling, 1 reply; 9+ messages in thread
From: Cao jin @ 2015-10-22 11:57 UTC (permalink / raw)
To: qemu-devel; +Cc: izumi.taku, alex.williamson, mst
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]))) {
+ 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
--
2.1.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] enable multi-function hot-add
2015-10-22 11:57 ` [Qemu-devel] [PATCH v4 2/2] enable " Cao jin
@ 2015-10-22 14:37 ` Michael S. Tsirkin
2015-10-23 4:13 ` Cao jin
0 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2015-10-22 14:37 UTC (permalink / raw)
To: Cao jin; +Cc: izumi.taku, alex.williamson, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] enable multi-function hot-add
2015-10-22 14:37 ` Michael S. Tsirkin
@ 2015-10-23 4:13 ` Cao jin
2015-10-23 6:50 ` Michael S. Tsirkin
0 siblings, 1 reply; 9+ messages in thread
From: Cao jin @ 2015-10-23 4:13 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: izumi.taku, alex.williamson, qemu-devel
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
> .
>
--
Yours Sincerely,
Cao Jin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] enable multi-function hot-add
2015-10-23 4:13 ` Cao jin
@ 2015-10-23 6:50 ` Michael S. Tsirkin
2015-10-23 8:14 ` Cao jin
0 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2015-10-23 6:50 UTC (permalink / raw)
To: Cao jin; +Cc: izumi.taku, alex.williamson, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] enable multi-function hot-add
2015-10-23 6:50 ` Michael S. Tsirkin
@ 2015-10-23 8:14 ` Cao jin
2015-10-23 13:41 ` Michael S. Tsirkin
0 siblings, 1 reply; 9+ messages in thread
From: Cao jin @ 2015-10-23 8:14 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: izumi.taku, alex.williamson, qemu-devel
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
> .
>
--
Yours Sincerely,
Cao Jin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] enable multi-function hot-add
2015-10-23 8:14 ` Cao jin
@ 2015-10-23 13:41 ` Michael S. Tsirkin
2015-10-23 14:12 ` Cao jin
0 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2015-10-23 13:41 UTC (permalink / raw)
To: Cao jin; +Cc: izumi.taku, alex.williamson, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/2] enable multi-function hot-add
2015-10-23 13:41 ` Michael S. Tsirkin
@ 2015-10-23 14:12 ` Cao jin
0 siblings, 0 replies; 9+ messages in thread
From: Cao jin @ 2015-10-23 14:12 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: izumi.taku, alex.williamson, qemu-devel
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
> .
>
--
Yours Sincerely,
Cao Jin
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-10-23 14:06 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-10-22 11:57 [Qemu-devel] [PATCH v4 0/2] PCI-e device multi-function hot-add support Cao jin
2015-10-22 11:57 ` [Qemu-devel] [PATCH v4 1/2] remove function during multi-function hot-add Cao jin
2015-10-22 11:57 ` [Qemu-devel] [PATCH v4 2/2] enable " Cao jin
2015-10-22 14:37 ` Michael S. Tsirkin
2015-10-23 4:13 ` Cao jin
2015-10-23 6:50 ` Michael S. Tsirkin
2015-10-23 8:14 ` Cao jin
2015-10-23 13:41 ` Michael S. Tsirkin
2015-10-23 14:12 ` Cao jin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).