* [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).