* [Qemu-devel] [PATCH v3 0/2] PCI-e device multi-function hot-add support @ 2015-10-13 8:41 Cao jin 2015-10-13 8:41 ` [Qemu-devel] [PATCH v3 1/2] enable multi-function hot-add Cao jin ` (3 more replies) 0 siblings, 4 replies; 23+ messages in thread From: Cao jin @ 2015-10-13 8:41 UTC (permalink / raw) To: qemu-devel; +Cc: pbonzini, alex.williamson, izumi.taku, 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 regret. changelog: 1. Flag device as unexposed when func 0 doesn`t exist, via return 0xFF in case of gratuitous pci bus scan. 2. Since device is unexposed to guest, can remove function individually, without interaction with the guest. Cao jin (2): enable multi-function hot-add remove function during multi-function hot-add hw/pci/pci.c | 10 ++++++++++ hw/pci/pci_host.c | 6 +++++- hw/pci/pcie.c | 38 +++++++++++++++++++++++++------------- 3 files changed, 40 insertions(+), 14 deletions(-) -- 2.1.0 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v3 1/2] enable multi-function hot-add 2015-10-13 8:41 [Qemu-devel] [PATCH v3 0/2] PCI-e device multi-function hot-add support Cao jin @ 2015-10-13 8:41 ` Cao jin 2015-10-13 8:41 ` [Qemu-devel] [PATCH v3 2/2] remove function during " Cao jin ` (2 subsequent siblings) 3 siblings, 0 replies; 23+ messages in thread From: Cao jin @ 2015-10-13 8:41 UTC (permalink / raw) To: qemu-devel; +Cc: pbonzini, alex.williamson, izumi.taku, mst Just ensure the function 0 added last, then driver will got the notification to scan all the function in the slot. Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> --- hw/pci/pci.c | 10 ++++++++++ hw/pci/pcie.c | 18 +++++++++--------- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index ccea628..15b53d9 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -847,6 +847,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, PCIConfigWriteFunc *config_write = pc->config_write; Error *local_err = NULL; AddressSpace *dma_as; + DeviceState *dev = DEVICE(pci_dev); if (devfn < 0) { for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices); @@ -864,6 +865,15 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, PCI_SLOT(devfn), PCI_FUNC(devfn), name, bus->devices[devfn]->name); return NULL; + } else if (dev->hotplugged && + bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]) { + error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s," + " new func %s cannot be exposed to guest.", + PCI_SLOT(devfn), + bus->devices[PCI_DEVFN(PCI_SLOT(devfn), 0)]->name, + name); + + return NULL; } pci_dev->bus = bus; diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index 6e28985..89bf61b 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -249,16 +249,16 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, return; } - /* TODO: multifunction hot-plug. - * Right now, only a device of function = 0 is allowed to be - * hot plugged/unplugged. + /* To enable multifunction hot-plug, we just ensure the function + * 0 added last. Until function 0 added, we set the sltsta and + * inform OS via event notification. */ - assert(PCI_FUNC(pci_dev->devfn) == 0); - - pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, - PCI_EXP_SLTSTA_PDS); - pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), - PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP); + if (PCI_FUNC(pci_dev->devfn) == 0) { + pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, + PCI_EXP_SLTSTA_PDS); + pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), + PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP); + } } void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev, -- 2.1.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH v3 2/2] remove function during multi-function hot-add 2015-10-13 8:41 [Qemu-devel] [PATCH v3 0/2] PCI-e device multi-function hot-add support Cao jin 2015-10-13 8:41 ` [Qemu-devel] [PATCH v3 1/2] enable multi-function hot-add Cao jin @ 2015-10-13 8:41 ` Cao jin 2015-10-13 8:48 ` [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio devices Michael S. Tsirkin ` (2 more replies) 2015-10-13 8:49 ` [Qemu-devel] [PATCH v3 0/2] PCI-e device multi-function hot-add support Michael S. Tsirkin 2015-10-13 8:50 ` Michael S. Tsirkin 3 siblings, 3 replies; 23+ messages in thread From: Cao jin @ 2015-10-13 8:41 UTC (permalink / raw) To: qemu-devel; +Cc: pbonzini, alex.williamson, izumi.taku, mst In case user regret when hot-adding multi-function, should roll back, device_del the function added but not exposed to the guest. Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> --- hw/pci/pci_host.c | 6 +++++- hw/pci/pcie.c | 22 +++++++++++++++++----- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c index 3e26f92..35e5cf3 100644 --- a/hw/pci/pci_host.c +++ b/hw/pci/pci_host.c @@ -20,6 +20,7 @@ #include "hw/pci/pci.h" #include "hw/pci/pci_host.h" +#include "hw/pci/pci_bus.h" #include "trace.h" /* debug PCI */ @@ -88,10 +89,13 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len) uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len) { PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr); + PCIDevice *f0 = NULL; uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); uint32_t val; + uint8_t slot = (addr >> 11) & 0x1F; - if (!pci_dev) { + f0 = s->devices[PCI_DEVFN(slot, 0)]; + if (!pci_dev || (!f0 && pci_dev)) { return ~0x0; } diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index 89bf61b..58d2153 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -261,13 +261,30 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, } } +static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque) +{ + object_unparent(OBJECT(dev)); +} + void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { uint8_t *exp_cap; + PCIDevice *pci_dev = PCI_DEVICE(dev); + PCIBus *bus = pci_dev->bus; pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp); + /* In case user regret when hot-adding multi function, remove the function + * that is unexposed to guest individually, without interaction with guest. + */ + if (PCI_FUNC(pci_dev->devfn) > 0 && + bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)] == NULL) { + pcie_unplug_device(bus, pci_dev, NULL); + + return; + } + pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev)); } @@ -378,11 +395,6 @@ void pcie_cap_slot_reset(PCIDevice *dev) hotplug_event_update_event_status(dev); } -static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque) -{ - object_unparent(OBJECT(dev)); -} - void pcie_cap_slot_write_config(PCIDevice *dev, uint32_t addr, uint32_t val, int len) { -- 2.1.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio devices 2015-10-13 8:41 ` [Qemu-devel] [PATCH v3 2/2] remove function during " Cao jin @ 2015-10-13 8:48 ` Michael S. Tsirkin 2015-10-13 12:19 ` Cao jin 2015-10-13 8:51 ` [Qemu-devel] [PATCH v3 2/2] remove function during multi-function hot-add Michael S. Tsirkin 2015-10-13 15:27 ` Alex Williamson 2 siblings, 1 reply; 23+ messages in thread From: Michael S. Tsirkin @ 2015-10-13 8:48 UTC (permalink / raw) To: Cao jin; +Cc: pbonzini, alex.williamson, qemu-devel, izumi.taku On Tue, Oct 13, 2015 at 04:41:35PM +0800, Cao jin wrote: > In case user regret when hot-adding multi-function, should roll back, > device_del the function added but not exposed to the guest. > > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > --- > hw/pci/pci_host.c | 6 +++++- > hw/pci/pcie.c | 22 +++++++++++++++++----- > 2 files changed, 22 insertions(+), 6 deletions(-) > > diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c > index 3e26f92..35e5cf3 100644 > --- a/hw/pci/pci_host.c > +++ b/hw/pci/pci_host.c > @@ -20,6 +20,7 @@ > > #include "hw/pci/pci.h" > #include "hw/pci/pci_host.h" > +#include "hw/pci/pci_bus.h" > #include "trace.h" > > /* debug PCI */ > @@ -88,10 +89,13 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len) > uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len) > { > PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr); > + PCIDevice *f0 = NULL; > uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); > uint32_t val; > + uint8_t slot = (addr >> 11) & 0x1F; > > - if (!pci_dev) { > + f0 = s->devices[PCI_DEVFN(slot, 0)]; > + if (!pci_dev || (!f0 && pci_dev)) { > return ~0x0; > } > This seems to belong to the previous patch? > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index 89bf61b..58d2153 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -261,13 +261,30 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > } > } > > +static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque) > +{ > + object_unparent(OBJECT(dev)); > +} > + > void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > uint8_t *exp_cap; > + PCIDevice *pci_dev = PCI_DEVICE(dev); > + PCIBus *bus = pci_dev->bus; > > pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp); > > + /* In case user regret when hot-adding multi function, remove the function > + * that is unexposed to guest individually, without interaction with guest. > + */ > + if (PCI_FUNC(pci_dev->devfn) > 0 && > + bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)] == NULL) { Shorter: PCI_FUNC(pci_dev->devfn) && !bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)] > + pcie_unplug_device(bus, pci_dev, NULL); > + > + return; > + } > + > pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev)); > } > > @@ -378,11 +395,6 @@ void pcie_cap_slot_reset(PCIDevice *dev) > hotplug_event_update_event_status(dev); > } > > -static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque) > -{ > - object_unparent(OBJECT(dev)); > -} > - > void pcie_cap_slot_write_config(PCIDevice *dev, > uint32_t addr, uint32_t val, int len) > { > -- > 2.1.0 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio devices 2015-10-13 8:48 ` [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio devices Michael S. Tsirkin @ 2015-10-13 12:19 ` Cao jin 0 siblings, 0 replies; 23+ messages in thread From: Cao jin @ 2015-10-13 12:19 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: pbonzini, alex.williamson, qemu-devel, izumi.taku Hi, Michael Thanks for your quick response:) On 10/13/2015 04:48 PM, Michael S. Tsirkin wrote: > On Tue, Oct 13, 2015 at 04:41:35PM +0800, Cao jin wrote: >> In case user regret when hot-adding multi-function, should roll back, >> device_del the function added but not exposed to the guest. >> >> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> >> --- >> hw/pci/pci_host.c | 6 +++++- >> hw/pci/pcie.c | 22 +++++++++++++++++----- >> 2 files changed, 22 insertions(+), 6 deletions(-) >> >> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c >> index 3e26f92..35e5cf3 100644 >> --- a/hw/pci/pci_host.c >> +++ b/hw/pci/pci_host.c >> @@ -20,6 +20,7 @@ >> >> #include "hw/pci/pci.h" >> #include "hw/pci/pci_host.h" >> +#include "hw/pci/pci_bus.h" >> #include "trace.h" >> >> /* debug PCI */ >> @@ -88,10 +89,13 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len) >> uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len) >> { >> PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr); >> + PCIDevice *f0 = NULL; >> uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); >> uint32_t val; >> + uint8_t slot = (addr >> 11) & 0x1F; >> >> - if (!pci_dev) { >> + f0 = s->devices[PCI_DEVFN(slot, 0)]; >> + if (!pci_dev || (!f0 && pci_dev)) { >> return ~0x0; >> } >> > > This seems to belong to the previous patch? > Strictly speaking, yes, will move it in next version. >> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c >> index 89bf61b..58d2153 100644 >> --- a/hw/pci/pcie.c >> +++ b/hw/pci/pcie.c >> @@ -261,13 +261,30 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, >> } >> } >> >> +static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque) >> +{ >> + object_unparent(OBJECT(dev)); >> +} >> + >> void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev, >> DeviceState *dev, Error **errp) >> { >> uint8_t *exp_cap; >> + PCIDevice *pci_dev = PCI_DEVICE(dev); >> + PCIBus *bus = pci_dev->bus; >> >> pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp); >> >> + /* In case user regret when hot-adding multi function, remove the function >> + * that is unexposed to guest individually, without interaction with guest. >> + */ >> + if (PCI_FUNC(pci_dev->devfn) > 0 && >> + bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)] == NULL) { > > Shorter: PCI_FUNC(pci_dev->devfn) && > !bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)] > Ok, will fix it in next vertion > >> + pcie_unplug_device(bus, pci_dev, NULL); >> + >> + return; >> + } >> + >> pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev)); >> } >> >> @@ -378,11 +395,6 @@ void pcie_cap_slot_reset(PCIDevice *dev) >> hotplug_event_update_event_status(dev); >> } >> >> -static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque) >> -{ >> - object_unparent(OBJECT(dev)); >> -} >> - >> void pcie_cap_slot_write_config(PCIDevice *dev, >> uint32_t addr, uint32_t val, int len) >> { >> -- >> 2.1.0 > . > -- Yours Sincerely, Cao Jin ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] remove function during multi-function hot-add 2015-10-13 8:41 ` [Qemu-devel] [PATCH v3 2/2] remove function during " Cao jin 2015-10-13 8:48 ` [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio devices Michael S. Tsirkin @ 2015-10-13 8:51 ` Michael S. Tsirkin 2015-10-13 9:54 ` Cao jin 2015-10-13 15:27 ` Alex Williamson 2 siblings, 1 reply; 23+ messages in thread From: Michael S. Tsirkin @ 2015-10-13 8:51 UTC (permalink / raw) To: Cao jin; +Cc: pbonzini, alex.williamson, qemu-devel, izumi.taku On Tue, Oct 13, 2015 at 04:41:35PM +0800, Cao jin wrote: > In case user regret when hot-adding multi-function, should roll back, > device_del the function added but not exposed to the guest. > > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> I think this patch should come first, before we enable the functionality that depends on it. > --- > hw/pci/pci_host.c | 6 +++++- > hw/pci/pcie.c | 22 +++++++++++++++++----- > 2 files changed, 22 insertions(+), 6 deletions(-) > > diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c > index 3e26f92..35e5cf3 100644 > --- a/hw/pci/pci_host.c > +++ b/hw/pci/pci_host.c > @@ -20,6 +20,7 @@ > > #include "hw/pci/pci.h" > #include "hw/pci/pci_host.h" > +#include "hw/pci/pci_bus.h" > #include "trace.h" > > /* debug PCI */ > @@ -88,10 +89,13 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len) > uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len) > { > PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr); > + PCIDevice *f0 = NULL; > uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); > uint32_t val; > + uint8_t slot = (addr >> 11) & 0x1F; > > - if (!pci_dev) { > + f0 = s->devices[PCI_DEVFN(slot, 0)]; > + if (!pci_dev || (!f0 && pci_dev)) { > return ~0x0; > } > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index 89bf61b..58d2153 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -261,13 +261,30 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > } > } > > +static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque) > +{ > + object_unparent(OBJECT(dev)); > +} > + > void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > uint8_t *exp_cap; > + PCIDevice *pci_dev = PCI_DEVICE(dev); > + PCIBus *bus = pci_dev->bus; > > pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp); > > + /* In case user regret when hot-adding multi function, remove the function > + * that is unexposed to guest individually, without interaction with guest. > + */ > + if (PCI_FUNC(pci_dev->devfn) > 0 && > + bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)] == NULL) { > + pcie_unplug_device(bus, pci_dev, NULL); > + > + return; > + } > + > pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev)); > } > > @@ -378,11 +395,6 @@ void pcie_cap_slot_reset(PCIDevice *dev) > hotplug_event_update_event_status(dev); > } > > -static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque) > -{ > - object_unparent(OBJECT(dev)); > -} > - > void pcie_cap_slot_write_config(PCIDevice *dev, > uint32_t addr, uint32_t val, int len) > { > -- > 2.1.0 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] remove function during multi-function hot-add 2015-10-13 8:51 ` [Qemu-devel] [PATCH v3 2/2] remove function during multi-function hot-add Michael S. Tsirkin @ 2015-10-13 9:54 ` Cao jin 2015-10-13 10:21 ` Michael S. Tsirkin 0 siblings, 1 reply; 23+ messages in thread From: Cao jin @ 2015-10-13 9:54 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: pbonzini, alex.williamson, qemu-devel, izumi.taku Hi Michael On 10/13/2015 04:51 PM, Michael S. Tsirkin wrote: > On Tue, Oct 13, 2015 at 04:41:35PM +0800, Cao jin wrote: >> In case user regret when hot-adding multi-function, should roll back, >> device_del the function added but not exposed to the guest. >> >> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > > I think this patch should come first, before we enable the > functionality that depends on it. > Do you mean, the function should be removed individually in any condition? Because as you know, device_del pci_dev will remove all the functions in the slot that are fulled exposed to the guest, Alex also mentioned this limitation before. > >> --- >> hw/pci/pci_host.c | 6 +++++- >> hw/pci/pcie.c | 22 +++++++++++++++++----- >> 2 files changed, 22 insertions(+), 6 deletions(-) >> >> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c >> index 3e26f92..35e5cf3 100644 >> --- a/hw/pci/pci_host.c >> +++ b/hw/pci/pci_host.c >> @@ -20,6 +20,7 @@ >> >> #include "hw/pci/pci.h" >> #include "hw/pci/pci_host.h" >> +#include "hw/pci/pci_bus.h" >> #include "trace.h" >> >> /* debug PCI */ >> @@ -88,10 +89,13 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len) >> uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len) >> { >> PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr); >> + PCIDevice *f0 = NULL; >> uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); >> uint32_t val; >> + uint8_t slot = (addr >> 11) & 0x1F; >> >> - if (!pci_dev) { >> + f0 = s->devices[PCI_DEVFN(slot, 0)]; >> + if (!pci_dev || (!f0 && pci_dev)) { >> return ~0x0; >> } >> >> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c >> index 89bf61b..58d2153 100644 >> --- a/hw/pci/pcie.c >> +++ b/hw/pci/pcie.c >> @@ -261,13 +261,30 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, >> } >> } >> >> +static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque) >> +{ >> + object_unparent(OBJECT(dev)); >> +} >> + >> void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev, >> DeviceState *dev, Error **errp) >> { >> uint8_t *exp_cap; >> + PCIDevice *pci_dev = PCI_DEVICE(dev); >> + PCIBus *bus = pci_dev->bus; >> >> pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp); >> >> + /* In case user regret when hot-adding multi function, remove the function >> + * that is unexposed to guest individually, without interaction with guest. >> + */ >> + if (PCI_FUNC(pci_dev->devfn) > 0 && >> + bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)] == NULL) { >> + pcie_unplug_device(bus, pci_dev, NULL); >> + >> + return; >> + } >> + >> pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev)); >> } >> >> @@ -378,11 +395,6 @@ void pcie_cap_slot_reset(PCIDevice *dev) >> hotplug_event_update_event_status(dev); >> } >> >> -static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque) >> -{ >> - object_unparent(OBJECT(dev)); >> -} >> - >> void pcie_cap_slot_write_config(PCIDevice *dev, >> uint32_t addr, uint32_t val, int len) >> { >> -- >> 2.1.0 > . > -- Yours Sincerely, Cao Jin ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] remove function during multi-function hot-add 2015-10-13 9:54 ` Cao jin @ 2015-10-13 10:21 ` Michael S. Tsirkin 0 siblings, 0 replies; 23+ messages in thread From: Michael S. Tsirkin @ 2015-10-13 10:21 UTC (permalink / raw) To: Cao jin; +Cc: pbonzini, alex.williamson, qemu-devel, izumi.taku On Tue, Oct 13, 2015 at 05:54:34PM +0800, Cao jin wrote: > Hi Michael > > On 10/13/2015 04:51 PM, Michael S. Tsirkin wrote: > >On Tue, Oct 13, 2015 at 04:41:35PM +0800, Cao jin wrote: > >>In case user regret when hot-adding multi-function, should roll back, > >>device_del the function added but not exposed to the guest. > >> > >>Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > > > >I think this patch should come first, before we enable the > >functionality that depends on it. > > > Do you mean, the function should be removed individually in any condition? I just mean the patches in the series should be reordered. > Because as you know, device_del pci_dev will remove all the functions in the > slot that are fulled exposed to the guest, Alex also mentioned this > limitation before. > > > >>--- > >> hw/pci/pci_host.c | 6 +++++- > >> hw/pci/pcie.c | 22 +++++++++++++++++----- > >> 2 files changed, 22 insertions(+), 6 deletions(-) > >> > >>diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c > >>index 3e26f92..35e5cf3 100644 > >>--- a/hw/pci/pci_host.c > >>+++ b/hw/pci/pci_host.c > >>@@ -20,6 +20,7 @@ > >> > >> #include "hw/pci/pci.h" > >> #include "hw/pci/pci_host.h" > >>+#include "hw/pci/pci_bus.h" > >> #include "trace.h" > >> > >> /* debug PCI */ > >>@@ -88,10 +89,13 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len) > >> uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len) > >> { > >> PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr); > >>+ PCIDevice *f0 = NULL; > >> uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); > >> uint32_t val; > >>+ uint8_t slot = (addr >> 11) & 0x1F; > >> > >>- if (!pci_dev) { > >>+ f0 = s->devices[PCI_DEVFN(slot, 0)]; > >>+ if (!pci_dev || (!f0 && pci_dev)) { > >> return ~0x0; > >> } > >> > >>diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > >>index 89bf61b..58d2153 100644 > >>--- a/hw/pci/pcie.c > >>+++ b/hw/pci/pcie.c > >>@@ -261,13 +261,30 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > >> } > >> } > >> > >>+static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque) > >>+{ > >>+ object_unparent(OBJECT(dev)); > >>+} > >>+ > >> void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev, > >> DeviceState *dev, Error **errp) > >> { > >> uint8_t *exp_cap; > >>+ PCIDevice *pci_dev = PCI_DEVICE(dev); > >>+ PCIBus *bus = pci_dev->bus; > >> > >> pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp); > >> > >>+ /* In case user regret when hot-adding multi function, remove the function > >>+ * that is unexposed to guest individually, without interaction with guest. > >>+ */ > >>+ if (PCI_FUNC(pci_dev->devfn) > 0 && > >>+ bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)] == NULL) { > >>+ pcie_unplug_device(bus, pci_dev, NULL); > >>+ > >>+ return; > >>+ } > >>+ > >> pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev)); > >> } > >> > >>@@ -378,11 +395,6 @@ void pcie_cap_slot_reset(PCIDevice *dev) > >> hotplug_event_update_event_status(dev); > >> } > >> > >>-static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque) > >>-{ > >>- object_unparent(OBJECT(dev)); > >>-} > >>- > >> void pcie_cap_slot_write_config(PCIDevice *dev, > >> uint32_t addr, uint32_t val, int len) > >> { > >>-- > >>2.1.0 > >. > > > > -- > Yours Sincerely, > > Cao Jin ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] remove function during multi-function hot-add 2015-10-13 8:41 ` [Qemu-devel] [PATCH v3 2/2] remove function during " Cao jin 2015-10-13 8:48 ` [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio devices Michael S. Tsirkin 2015-10-13 8:51 ` [Qemu-devel] [PATCH v3 2/2] remove function during multi-function hot-add Michael S. Tsirkin @ 2015-10-13 15:27 ` Alex Williamson 2015-10-14 5:46 ` Cao jin 2 siblings, 1 reply; 23+ messages in thread From: Alex Williamson @ 2015-10-13 15:27 UTC (permalink / raw) To: Cao jin; +Cc: pbonzini, izumi.taku, qemu-devel, mst On Tue, 2015-10-13 at 16:41 +0800, Cao jin wrote: > In case user regret when hot-adding multi-function, should roll back, > device_del the function added but not exposed to the guest. As Michael suggests, this patch should come first, before we actually enable multi-function hot-add. > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> > --- > hw/pci/pci_host.c | 6 +++++- > hw/pci/pcie.c | 22 +++++++++++++++++----- > 2 files changed, 22 insertions(+), 6 deletions(-) > > diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c > index 3e26f92..35e5cf3 100644 > --- a/hw/pci/pci_host.c > +++ b/hw/pci/pci_host.c > @@ -20,6 +20,7 @@ > > #include "hw/pci/pci.h" > #include "hw/pci/pci_host.h" > +#include "hw/pci/pci_bus.h" > #include "trace.h" > > /* debug PCI */ > @@ -88,10 +89,13 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len) > uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len) > { > PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr); > + PCIDevice *f0 = NULL; > uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); > uint32_t val; > + uint8_t slot = (addr >> 11) & 0x1F; > > - if (!pci_dev) { > + f0 = s->devices[PCI_DEVFN(slot, 0)]; > + if (!pci_dev || (!f0 && pci_dev)) { This uses a lot more variables and operations than it needs to: if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) { Shouldn't we do the same on pci_data_write()? A well behaved guest won't blindly write to config space, but not all guests are well behaved. Comments in the code would be nice here to explain that non-zero functions are only exposed when function zero is present, allowing direct removal of unexposed devices. I imagine that due to qemu locking that we don't have a race here, but note that devices[] is populated early in the core pci realize function, prior to the device initialize function, and there are any number of reasons that failure could still occur, which would create a window where the function is accessible. I doubt this is an issue, but simply note it for completeness. > return ~0x0; > } > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index 89bf61b..58d2153 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -261,13 +261,30 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > } > } > > +static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque) > +{ > + object_unparent(OBJECT(dev)); > +} > + > void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > uint8_t *exp_cap; > + PCIDevice *pci_dev = PCI_DEVICE(dev); > + PCIBus *bus = pci_dev->bus; > > pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp); > > + /* In case user regret when hot-adding multi function, remove the function > + * that is unexposed to guest individually, without interaction with guest. > + */ > + if (PCI_FUNC(pci_dev->devfn) > 0 && > + bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)] == NULL) { Similarly, if (PCI_FUNC(pci_dev->devfn) && !bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) { > + pcie_unplug_device(bus, pci_dev, NULL); > + > + return; > + } > + > pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev)); > } > > @@ -378,11 +395,6 @@ void pcie_cap_slot_reset(PCIDevice *dev) > hotplug_event_update_event_status(dev); > } > > -static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque) > -{ > - object_unparent(OBJECT(dev)); > -} > - > void pcie_cap_slot_write_config(PCIDevice *dev, > uint32_t addr, uint32_t val, int len) > { ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/2] remove function during multi-function hot-add 2015-10-13 15:27 ` Alex Williamson @ 2015-10-14 5:46 ` Cao jin 0 siblings, 0 replies; 23+ messages in thread From: Cao jin @ 2015-10-14 5:46 UTC (permalink / raw) To: Alex Williamson; +Cc: pbonzini, izumi.taku, qemu-devel, mst Hi, Alex On 10/13/2015 11:27 PM, Alex Williamson wrote: > On Tue, 2015-10-13 at 16:41 +0800, Cao jin wrote: >> In case user regret when hot-adding multi-function, should roll back, >> device_del the function added but not exposed to the guest. > > As Michael suggests, this patch should come first, before we actually > enable multi-function hot-add. > OK. >> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> >> --- >> hw/pci/pci_host.c | 6 +++++- >> hw/pci/pcie.c | 22 +++++++++++++++++----- >> 2 files changed, 22 insertions(+), 6 deletions(-) >> >> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c >> index 3e26f92..35e5cf3 100644 >> --- a/hw/pci/pci_host.c >> +++ b/hw/pci/pci_host.c >> @@ -20,6 +20,7 @@ >> >> #include "hw/pci/pci.h" >> #include "hw/pci/pci_host.h" >> +#include "hw/pci/pci_bus.h" >> #include "trace.h" >> >> /* debug PCI */ >> @@ -88,10 +89,13 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len) >> uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len) >> { >> PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr); >> + PCIDevice *f0 = NULL; >> uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1); >> uint32_t val; >> + uint8_t slot = (addr >> 11) & 0x1F; >> >> - if (!pci_dev) { >> + f0 = s->devices[PCI_DEVFN(slot, 0)]; >> + if (!pci_dev || (!f0 && pci_dev)) { > > > This uses a lot more variables and operations than it needs to: > > if (!pci_dev || !s->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) { > Ok. variables is intended to make the line shorter. > Shouldn't we do the same on pci_data_write()? A well behaved guest > won't blindly write to config space, but not all guests are well > behaved. > Yup, agree. I missed the consideration of bad behavior. I thought anyone use the device should read the vendor ID first(good behavior), then do anything he/she want. Thanks for reminding > Comments in the code would be nice here to explain that non-zero > functions are only exposed when function zero is present, allowing > direct removal of unexposed devices. > OK > I imagine that due to qemu locking that we don't have a race here, but > note that devices[] is populated early in the core pci realize function, > prior to the device initialize function, and there are any number of > reasons that failure could still occur, which would create a window > where the function is accessible. I doubt this is an issue, but simply > note it for completeness. Ok, will consider the "function access window" condition, to see what I can do with it >> return ~0x0; >> } >> >> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c >> index 89bf61b..58d2153 100644 >> --- a/hw/pci/pcie.c >> +++ b/hw/pci/pcie.c >> @@ -261,13 +261,30 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, >> } >> } >> >> +static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque) >> +{ >> + object_unparent(OBJECT(dev)); >> +} >> + >> void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev, >> DeviceState *dev, Error **errp) >> { >> uint8_t *exp_cap; >> + PCIDevice *pci_dev = PCI_DEVICE(dev); >> + PCIBus *bus = pci_dev->bus; >> >> pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp); >> >> + /* In case user regret when hot-adding multi function, remove the function >> + * that is unexposed to guest individually, without interaction with guest. >> + */ >> + if (PCI_FUNC(pci_dev->devfn) > 0 && >> + bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)] == NULL) { > > Similarly, > > if (PCI_FUNC(pci_dev->devfn) && !bus->devices[PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 0)]) { > Ok >> + pcie_unplug_device(bus, pci_dev, NULL); >> + >> + return; >> + } >> + >> pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev)); >> } >> >> @@ -378,11 +395,6 @@ void pcie_cap_slot_reset(PCIDevice *dev) >> hotplug_event_update_event_status(dev); >> } >> >> -static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque) >> -{ >> - object_unparent(OBJECT(dev)); >> -} >> - >> void pcie_cap_slot_write_config(PCIDevice *dev, >> uint32_t addr, uint32_t val, int len) >> { > > > > . > -- Yours Sincerely, Cao Jin ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/2] PCI-e device multi-function hot-add support 2015-10-13 8:41 [Qemu-devel] [PATCH v3 0/2] PCI-e device multi-function hot-add support Cao jin 2015-10-13 8:41 ` [Qemu-devel] [PATCH v3 1/2] enable multi-function hot-add Cao jin 2015-10-13 8:41 ` [Qemu-devel] [PATCH v3 2/2] remove function during " Cao jin @ 2015-10-13 8:49 ` Michael S. Tsirkin 2015-10-13 11:54 ` Cao jin 2015-10-13 8:50 ` Michael S. Tsirkin 3 siblings, 1 reply; 23+ messages in thread From: Michael S. Tsirkin @ 2015-10-13 8:49 UTC (permalink / raw) To: Cao jin; +Cc: pbonzini, alex.williamson, qemu-devel, izumi.taku On Tue, Oct 13, 2015 at 04:41:33PM +0800, Cao jin wrote: > Support PCI-e device hot-add multi-function via device_add, just ensure > add the function 0 is added last. While allow user to roll back in the > middle via device_del, in case user regret. This patch doesn't seem to account of AIR though. > changelog: > 1. Flag device as unexposed when func 0 doesn`t exist, via return 0xFF > in case of gratuitous pci bus scan. > 2. Since device is unexposed to guest, can remove function individually, > without interaction with the guest. > > Cao jin (2): > enable multi-function hot-add > remove function during multi-function hot-add > > hw/pci/pci.c | 10 ++++++++++ > hw/pci/pci_host.c | 6 +++++- > hw/pci/pcie.c | 38 +++++++++++++++++++++++++------------- > 3 files changed, 40 insertions(+), 14 deletions(-) > > -- > 2.1.0 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/2] PCI-e device multi-function hot-add support 2015-10-13 8:49 ` [Qemu-devel] [PATCH v3 0/2] PCI-e device multi-function hot-add support Michael S. Tsirkin @ 2015-10-13 11:54 ` Cao jin 2015-10-13 13:10 ` Michael S. Tsirkin 0 siblings, 1 reply; 23+ messages in thread From: Cao jin @ 2015-10-13 11:54 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: pbonzini, alex.williamson, qemu-devel, izumi.taku On 10/13/2015 04:49 PM, Michael S. Tsirkin wrote: > On Tue, Oct 13, 2015 at 04:41:33PM +0800, Cao jin wrote: >> Support PCI-e device hot-add multi-function via device_add, just ensure >> add the function 0 is added last. While allow user to roll back in the >> middle via device_del, in case user regret. > > This patch doesn't seem to account of AIR though. > Yes, but the AIR function seems never be used(nobody calls the function pcie_ari_init()), so I am a little confused about should it be consindered? >> changelog: >> 1. Flag device as unexposed when func 0 doesn`t exist, via return 0xFF >> in case of gratuitous pci bus scan. >> 2. Since device is unexposed to guest, can remove function individually, >> without interaction with the guest. >> >> Cao jin (2): >> enable multi-function hot-add >> remove function during multi-function hot-add >> >> hw/pci/pci.c | 10 ++++++++++ >> hw/pci/pci_host.c | 6 +++++- >> hw/pci/pcie.c | 38 +++++++++++++++++++++++++------------- >> 3 files changed, 40 insertions(+), 14 deletions(-) >> >> -- >> 2.1.0 > . > -- Yours Sincerely, Cao Jin ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/2] PCI-e device multi-function hot-add support 2015-10-13 11:54 ` Cao jin @ 2015-10-13 13:10 ` Michael S. Tsirkin 2015-10-14 5:48 ` Cao jin 2015-10-21 8:32 ` Cao jin 0 siblings, 2 replies; 23+ messages in thread From: Michael S. Tsirkin @ 2015-10-13 13:10 UTC (permalink / raw) To: Cao jin; +Cc: pbonzini, alex.williamson, qemu-devel, izumi.taku On Tue, Oct 13, 2015 at 07:54:07PM +0800, Cao jin wrote: > > > On 10/13/2015 04:49 PM, Michael S. Tsirkin wrote: > >On Tue, Oct 13, 2015 at 04:41:33PM +0800, Cao jin wrote: > >>Support PCI-e device hot-add multi-function via device_add, just ensure > >>add the function 0 is added last. While allow user to roll back in the > >>middle via device_del, in case user regret. > > > >This patch doesn't seem to account of AIR though. > > > > Yes, but the AIR function seems never be used(nobody calls the function > pcie_ari_init()), so I am a little confused about should it be consindered? Yes please - we'll likely use that in the future. Pls add an API that takes ari into account. > >>changelog: > >>1. Flag device as unexposed when func 0 doesn`t exist, via return 0xFF > >> in case of gratuitous pci bus scan. > >>2. Since device is unexposed to guest, can remove function individually, > >> without interaction with the guest. > >> > >>Cao jin (2): > >> enable multi-function hot-add > >> remove function during multi-function hot-add > >> > >> hw/pci/pci.c | 10 ++++++++++ > >> hw/pci/pci_host.c | 6 +++++- > >> hw/pci/pcie.c | 38 +++++++++++++++++++++++++------------- > >> 3 files changed, 40 insertions(+), 14 deletions(-) > >> > >>-- > >>2.1.0 > >. > > > > -- > Yours Sincerely, > > Cao Jin ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/2] PCI-e device multi-function hot-add support 2015-10-13 13:10 ` Michael S. Tsirkin @ 2015-10-14 5:48 ` Cao jin 2015-10-21 8:32 ` Cao jin 1 sibling, 0 replies; 23+ messages in thread From: Cao jin @ 2015-10-14 5:48 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: pbonzini, alex.williamson, qemu-devel, izumi.taku On 10/13/2015 09:10 PM, Michael S. Tsirkin wrote: > On Tue, Oct 13, 2015 at 07:54:07PM +0800, Cao jin wrote: >> >> >> On 10/13/2015 04:49 PM, Michael S. Tsirkin wrote: >>> On Tue, Oct 13, 2015 at 04:41:33PM +0800, Cao jin wrote: >>>> Support PCI-e device hot-add multi-function via device_add, just ensure >>>> add the function 0 is added last. While allow user to roll back in the >>>> middle via device_del, in case user regret. >>> >>> This patch doesn't seem to account of AIR though. >>> >> >> Yes, but the AIR function seems never be used(nobody calls the function >> pcie_ari_init()), so I am a little confused about should it be consindered? > > Yes please - we'll likely use that in the future. Pls add an API > that takes ari into account. > Ok, I am on it >>>> changelog: >>>> 1. Flag device as unexposed when func 0 doesn`t exist, via return 0xFF >>>> in case of gratuitous pci bus scan. >>>> 2. Since device is unexposed to guest, can remove function individually, >>>> without interaction with the guest. >>>> >>>> Cao jin (2): >>>> enable multi-function hot-add >>>> remove function during multi-function hot-add >>>> >>>> hw/pci/pci.c | 10 ++++++++++ >>>> hw/pci/pci_host.c | 6 +++++- >>>> hw/pci/pcie.c | 38 +++++++++++++++++++++++++------------- >>>> 3 files changed, 40 insertions(+), 14 deletions(-) >>>> >>>> -- >>>> 2.1.0 >>> . >>> >> >> -- >> Yours Sincerely, >> >> Cao Jin > . > -- Yours Sincerely, Cao Jin ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/2] PCI-e device multi-function hot-add support 2015-10-13 13:10 ` Michael S. Tsirkin 2015-10-14 5:48 ` Cao jin @ 2015-10-21 8:32 ` Cao jin 2015-10-21 9:11 ` Michael S. Tsirkin 1 sibling, 1 reply; 23+ messages in thread From: Cao jin @ 2015-10-21 8:32 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: izumi.taku, alex.williamson, qemu-devel Hello Michael On 10/13/2015 09:10 PM, Michael S. Tsirkin wrote: > On Tue, Oct 13, 2015 at 07:54:07PM +0800, Cao jin wrote: >> >> >> On 10/13/2015 04:49 PM, Michael S. Tsirkin wrote: >>> On Tue, Oct 13, 2015 at 04:41:33PM +0800, Cao jin wrote: >>>> Support PCI-e device hot-add multi-function via device_add, just ensure >>>> add the function 0 is added last. While allow user to roll back in the >>>> middle via device_del, in case user regret. >>> >>> This patch doesn't seem to account of AIR though. >>> >> >> Yes, but the AIR function seems never be used(nobody calls the function >> pcie_ari_init()), so I am a little confused about should it be consindered? > > Yes please - we'll likely use that in the future. Pls add an API > that takes ari into account. > after analysed the code and read the spec, but still a little confused about "add an API that takes ari into account". what is new API for? Could you give a detailed description? AFAICT now, taking ari into account will affect the "addr" property of PCI device(maybe also need a new prop of "ari=on" to enable ari). >>>> changelog: >>>> 1. Flag device as unexposed when func 0 doesn`t exist, via return 0xFF >>>> in case of gratuitous pci bus scan. >>>> 2. Since device is unexposed to guest, can remove function individually, >>>> without interaction with the guest. >>>> >>>> Cao jin (2): >>>> enable multi-function hot-add >>>> remove function during multi-function hot-add >>>> >>>> hw/pci/pci.c | 10 ++++++++++ >>>> hw/pci/pci_host.c | 6 +++++- >>>> hw/pci/pcie.c | 38 +++++++++++++++++++++++++------------- >>>> 3 files changed, 40 insertions(+), 14 deletions(-) >>>> >>>> -- >>>> 2.1.0 >>> . >>> >> >> -- >> Yours Sincerely, >> >> Cao Jin > . > -- Yours Sincerely, Cao Jin ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/2] PCI-e device multi-function hot-add support 2015-10-21 8:32 ` Cao jin @ 2015-10-21 9:11 ` Michael S. Tsirkin 0 siblings, 0 replies; 23+ messages in thread From: Michael S. Tsirkin @ 2015-10-21 9:11 UTC (permalink / raw) To: Cao jin; +Cc: izumi.taku, alex.williamson, qemu-devel On Wed, Oct 21, 2015 at 04:32:17PM +0800, Cao jin wrote: > Hello Michael > > On 10/13/2015 09:10 PM, Michael S. Tsirkin wrote: > >On Tue, Oct 13, 2015 at 07:54:07PM +0800, Cao jin wrote: > >> > >> > >>On 10/13/2015 04:49 PM, Michael S. Tsirkin wrote: > >>>On Tue, Oct 13, 2015 at 04:41:33PM +0800, Cao jin wrote: > >>>>Support PCI-e device hot-add multi-function via device_add, just ensure > >>>>add the function 0 is added last. While allow user to roll back in the > >>>>middle via device_del, in case user regret. > >>> > >>>This patch doesn't seem to account of AIR though. > >>> > >> > >>Yes, but the AIR function seems never be used(nobody calls the function > >>pcie_ari_init()), so I am a little confused about should it be consindered? > > > >Yes please - we'll likely use that in the future. Pls add an API > >that takes ari into account. > > > after analysed the code and read the spec, but still a little confused about > "add an API that takes ari into account". what is new API for? Could you > give a detailed description? You are assuming PCI_FUNC == 0 means function 9. But the real rule is: for an express device associated with an upstream port, it's devfn == 0. For any other device (non express or not connected to an upstream port) - PCI_FUNC == 0. I think a reasonable API could be e.g. bool pci_is_function_0() with a comment explaining the issues. > AFAICT now, taking ari into account will affect the "addr" property of PCI > device(maybe also need a new prop of "ari=on" to enable ari). > > >>>>changelog: > >>>>1. Flag device as unexposed when func 0 doesn`t exist, via return 0xFF > >>>> in case of gratuitous pci bus scan. > >>>>2. Since device is unexposed to guest, can remove function individually, > >>>> without interaction with the guest. > >>>> > >>>>Cao jin (2): > >>>> enable multi-function hot-add > >>>> remove function during multi-function hot-add > >>>> > >>>> hw/pci/pci.c | 10 ++++++++++ > >>>> hw/pci/pci_host.c | 6 +++++- > >>>> hw/pci/pcie.c | 38 +++++++++++++++++++++++++------------- > >>>> 3 files changed, 40 insertions(+), 14 deletions(-) > >>>> > >>>>-- > >>>>2.1.0 > >>>. > >>> > >> > >>-- > >>Yours Sincerely, > >> > >>Cao Jin > >. > > > > -- > Yours Sincerely, > > Cao Jin ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/2] PCI-e device multi-function hot-add support 2015-10-13 8:41 [Qemu-devel] [PATCH v3 0/2] PCI-e device multi-function hot-add support Cao jin ` (2 preceding siblings ...) 2015-10-13 8:49 ` [Qemu-devel] [PATCH v3 0/2] PCI-e device multi-function hot-add support Michael S. Tsirkin @ 2015-10-13 8:50 ` Michael S. Tsirkin 3 siblings, 0 replies; 23+ messages in thread From: Michael S. Tsirkin @ 2015-10-13 8:50 UTC (permalink / raw) To: Cao jin; +Cc: pbonzini, alex.williamson, qemu-devel, izumi.taku On Tue, Oct 13, 2015 at 04:41:33PM +0800, Cao jin wrote: > Support PCI-e device hot-add multi-function via device_add, just ensure > add the function 0 is added last. While allow user to roll back in the > middle via device_del, in case user regret. s/regret/cancels the operation/ > changelog: > 1. Flag device as unexposed when func 0 doesn`t exist, via return 0xFF > in case of gratuitous pci bus scan. > 2. Since device is unexposed to guest, can remove function individually, > without interaction with the guest. > > Cao jin (2): > enable multi-function hot-add > remove function during multi-function hot-add > > hw/pci/pci.c | 10 ++++++++++ > hw/pci/pci_host.c | 6 +++++- > hw/pci/pcie.c | 38 +++++++++++++++++++++++++------------- > 3 files changed, 40 insertions(+), 14 deletions(-) > > -- > 2.1.0 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio devices @ 2015-10-12 15:17 Marcel Apfelbaum 2015-10-12 15:42 ` Michael S. Tsirkin 0 siblings, 1 reply; 23+ messages in thread From: Marcel Apfelbaum @ 2015-10-12 15:17 UTC (permalink / raw) To: qemu-devel; +Cc: marcel, mst The virtio devices are converted to PCI-Express if they are plugged into a PCI-Express bus and the 'modern' protocol is enabled. Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> --- This is an RFC because all it does it adds the PCIe capability and nothing more. I post it early so I can get feedbacks on what is the best way to continue it. Any comments would be appreciated, Thanks, Marcel hw/virtio/virtio-pci.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c index 6703806..f7c93d9 100644 --- a/hw/virtio/virtio-pci.c +++ b/hw/virtio/virtio-pci.c @@ -1591,6 +1591,17 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) address_space_init(&proxy->modern_as, &proxy->modern_cfg, "virtio-pci-cfg-as"); + if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN) + && pci_bus_is_express(pci_dev->bus)) { + int pos = pci_add_capability(pci_dev, PCI_CAP_ID_EXP, 0, + PCI_EXP_VER2_SIZEOF); + + if (pos > 0) { + pci_dev->exp.exp_cap = pos; + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; + } + } + virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy); if (k->realize) { k->realize(proxy, errp); -- 2.1.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio devices 2015-10-12 15:17 [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio devices Marcel Apfelbaum @ 2015-10-12 15:42 ` Michael S. Tsirkin 2015-10-13 8:13 ` Gerd Hoffmann 2015-10-13 14:31 ` Marcel Apfelbaum 0 siblings, 2 replies; 23+ messages in thread From: Michael S. Tsirkin @ 2015-10-12 15:42 UTC (permalink / raw) To: Marcel Apfelbaum; +Cc: qemu-devel On Mon, Oct 12, 2015 at 06:17:54PM +0300, Marcel Apfelbaum wrote: > The virtio devices are converted to PCI-Express > if they are plugged into a PCI-Express bus and > the 'modern' protocol is enabled. > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > --- > > This is an RFC because all it does it adds the PCIe capability and nothing more. Express capability is easy. But if you go over express space you will see that a bunch of other capabilities are required, such as PM capability etc. These might need more work. > I post it early so I can get feedbacks on what is the best way to continue it. > > Any comments would be appreciated, > Thanks, > Marcel > > hw/virtio/virtio-pci.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 6703806..f7c93d9 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1591,6 +1591,17 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp) > > address_space_init(&proxy->modern_as, &proxy->modern_cfg, "virtio-pci-cfg-as"); > > + if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN) > + && pci_bus_is_express(pci_dev->bus)) { One point: we probably want to avoid doing this for integrated devices on root bus. Does pci_bus_is_express return true there? > + int pos = pci_add_capability(pci_dev, PCI_CAP_ID_EXP, 0, > + PCI_EXP_VER2_SIZEOF); > + > + if (pos > 0) { We probably want to assert on pos < 0 instead. That implies a code bug. > + pci_dev->exp.exp_cap = pos; > + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; > + } > + } > + > virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy); > if (k->realize) { > k->realize(proxy, errp); > -- > 2.1.0 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio devices 2015-10-12 15:42 ` Michael S. Tsirkin @ 2015-10-13 8:13 ` Gerd Hoffmann 2015-10-13 8:44 ` Michael S. Tsirkin 2015-10-13 14:31 ` Marcel Apfelbaum 1 sibling, 1 reply; 23+ messages in thread From: Gerd Hoffmann @ 2015-10-13 8:13 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: Marcel Apfelbaum, qemu-devel On Mo, 2015-10-12 at 18:42 +0300, Michael S. Tsirkin wrote: > On Mon, Oct 12, 2015 at 06:17:54PM +0300, Marcel Apfelbaum wrote: > > The virtio devices are converted to PCI-Express > > if they are plugged into a PCI-Express bus and > > the 'modern' protocol is enabled. > > > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > > --- > > > > This is an RFC because all it does it adds the PCIe capability and nothing more. > > Express capability is easy. > But if you go over express space you will see that a bunch of > other capabilities are required, such as PM capability etc. > These might need more work. Also what about the legacy io bar? I guess we'd better avoid that for express devices. Maybe it makes sense to add virtio-*-pcie devices (virtio-1.0 only, with pcie caps)? cheers, Gerd ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio devices 2015-10-13 8:13 ` Gerd Hoffmann @ 2015-10-13 8:44 ` Michael S. Tsirkin 0 siblings, 0 replies; 23+ messages in thread From: Michael S. Tsirkin @ 2015-10-13 8:44 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: Marcel Apfelbaum, qemu-devel On Tue, Oct 13, 2015 at 10:13:37AM +0200, Gerd Hoffmann wrote: > On Mo, 2015-10-12 at 18:42 +0300, Michael S. Tsirkin wrote: > > On Mon, Oct 12, 2015 at 06:17:54PM +0300, Marcel Apfelbaum wrote: > > > The virtio devices are converted to PCI-Express > > > if they are plugged into a PCI-Express bus and > > > the 'modern' protocol is enabled. > > > > > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > > > --- > > > > > > This is an RFC because all it does it adds the PCIe capability and nothing more. > > > > Express capability is easy. > > But if you go over express space you will see that a bunch of > > other capabilities are required, such as PM capability etc. > > These might need more work. > > Also what about the legacy io bar? I guess we'd better avoid that for > express devices. This needs some thought. We definitely still want a way to enable it I think, e.g. for old guests. > Maybe it makes sense to add virtio-*-pcie devices (virtio-1.0 only, with > pcie caps)? > > cheers, > Gerd Personally I think it's ugly enough to remember the need to specify -pci. OTOH the need to specify upstream and downstream ports is even uglier. Any idea how to address both issues at the same time? -- MST ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio devices 2015-10-12 15:42 ` Michael S. Tsirkin 2015-10-13 8:13 ` Gerd Hoffmann @ 2015-10-13 14:31 ` Marcel Apfelbaum 2015-10-13 14:47 ` Michael S. Tsirkin 1 sibling, 1 reply; 23+ messages in thread From: Marcel Apfelbaum @ 2015-10-13 14:31 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: Marcel Apfelbaum, qemu-devel [-- Attachment #1: Type: text/plain, Size: 2240 bytes --] On Monday, October 12, 2015, Michael S. Tsirkin <mst@redhat.com> wrote: > On Mon, Oct 12, 2015 at 06:17:54PM +0300, Marcel Apfelbaum wrote: > > The virtio devices are converted to PCI-Express > > if they are plugged into a PCI-Express bus and > > the 'modern' protocol is enabled. > > > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com <javascript:;>> > > --- > > > > This is an RFC because all it does it adds the PCIe capability and > nothing more. > > Express capability is easy. > But if you go over express space you will see that a bunch of > other capabilities are required, such as PM capability etc. > These might need more work. Sure, I'll look on the PCIe spec for the minimum requirements. > > > I post it early so I can get feedbacks on what is the best way to > continue it. > > > > Any comments would be appreciated, > > Thanks, > > Marcel > > > > hw/virtio/virtio-pci.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > index 6703806..f7c93d9 100644 > > --- a/hw/virtio/virtio-pci.c > > +++ b/hw/virtio/virtio-pci.c > > @@ -1591,6 +1591,17 @@ static void virtio_pci_realize(PCIDevice > *pci_dev, Error **errp) > > > > address_space_init(&proxy->modern_as, &proxy->modern_cfg, > "virtio-pci-cfg-as"); > > > > + if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN) > > + && pci_bus_is_express(pci_dev->bus)) { > > One point: we probably want to avoid doing this for integrated > devices on root bus. Does pci_bus_is_express return true there? Hmm, I'll check, but I think it does. Is this a must? > > > + int pos = pci_add_capability(pci_dev, PCI_CAP_ID_EXP, 0, > > + PCI_EXP_VER2_SIZEOF); > > + > > + if (pos > 0) { > > We probably want to assert on pos < 0 instead. > That implies a code bug. > > I was wondering what to do here, thanks for the idea! Thanks, Marcel > > > + pci_dev->exp.exp_cap = pos; > > + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; > > + } > > + } > > + > > virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy); > > if (k->realize) { > > k->realize(proxy, errp); > > -- > > 2.1.0 > > [-- Attachment #2: Type: text/html, Size: 3429 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio devices 2015-10-13 14:31 ` Marcel Apfelbaum @ 2015-10-13 14:47 ` Michael S. Tsirkin 0 siblings, 0 replies; 23+ messages in thread From: Michael S. Tsirkin @ 2015-10-13 14:47 UTC (permalink / raw) To: Marcel Apfelbaum; +Cc: Marcel Apfelbaum, qemu-devel On Tue, Oct 13, 2015 at 05:31:27PM +0300, Marcel Apfelbaum wrote: > > > On Monday, October 12, 2015, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Oct 12, 2015 at 06:17:54PM +0300, Marcel Apfelbaum wrote: > > The virtio devices are converted to PCI-Express > > if they are plugged into a PCI-Express bus and > > the 'modern' protocol is enabled. > > > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > > --- > > > > This is an RFC because all it does it adds the PCIe capability and > nothing more. > > Express capability is easy. > But if you go over express space you will see that a bunch of > other capabilities are required, such as PM capability etc. > These might need more work. > > > Sure, I'll look on the PCIe spec for the minimum requirements. > > > > > I post it early so I can get feedbacks on what is the best way to > continue it. > > > > Any comments would be appreciated, > > Thanks, > > Marcel > > > > hw/virtio/virtio-pci.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > > index 6703806..f7c93d9 100644 > > --- a/hw/virtio/virtio-pci.c > > +++ b/hw/virtio/virtio-pci.c > > @@ -1591,6 +1591,17 @@ static void virtio_pci_realize(PCIDevice *pci_dev, > Error **errp) > > > > address_space_init(&proxy->modern_as, &proxy->modern_cfg, > "virtio-pci-cfg-as"); > > > > + if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN) > > + && pci_bus_is_express(pci_dev->bus)) { > > One point: we probably want to avoid doing this for integrated > devices on root bus. Does pci_bus_is_express return true there? > > > Hmm, I'll check, but I think it does. Is this a must? It's probably a smart thing to do because you can't e.g. hot-unplug them. So supporting PCI E capability for integrated devices is probably more work than just making the PCI devices (which is not spec compliant but very popular so guests support this). > > > > + int pos = pci_add_capability(pci_dev, PCI_CAP_ID_EXP, 0, > > + PCI_EXP_VER2_SIZEOF); > > + > > + if (pos > 0) { > > We probably want to assert on pos < 0 instead. > That implies a code bug. > > > > I was wondering what to do here, thanks for the idea! > > Thanks, > Marcel > > > > > > + pci_dev->exp.exp_cap = pos; > > + pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS; > > + } > > + } > > + > > virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy); > > if (k->realize) { > > k->realize(proxy, errp); > > -- > > 2.1.0 > > ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2015-10-21 9:11 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-13 8:41 [Qemu-devel] [PATCH v3 0/2] PCI-e device multi-function hot-add support Cao jin 2015-10-13 8:41 ` [Qemu-devel] [PATCH v3 1/2] enable multi-function hot-add Cao jin 2015-10-13 8:41 ` [Qemu-devel] [PATCH v3 2/2] remove function during " Cao jin 2015-10-13 8:48 ` [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio devices Michael S. Tsirkin 2015-10-13 12:19 ` Cao jin 2015-10-13 8:51 ` [Qemu-devel] [PATCH v3 2/2] remove function during multi-function hot-add Michael S. Tsirkin 2015-10-13 9:54 ` Cao jin 2015-10-13 10:21 ` Michael S. Tsirkin 2015-10-13 15:27 ` Alex Williamson 2015-10-14 5:46 ` Cao jin 2015-10-13 8:49 ` [Qemu-devel] [PATCH v3 0/2] PCI-e device multi-function hot-add support Michael S. Tsirkin 2015-10-13 11:54 ` Cao jin 2015-10-13 13:10 ` Michael S. Tsirkin 2015-10-14 5:48 ` Cao jin 2015-10-21 8:32 ` Cao jin 2015-10-21 9:11 ` Michael S. Tsirkin 2015-10-13 8:50 ` Michael S. Tsirkin -- strict thread matches above, loose matches on Subject: below -- 2015-10-12 15:17 [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio devices Marcel Apfelbaum 2015-10-12 15:42 ` Michael S. Tsirkin 2015-10-13 8:13 ` Gerd Hoffmann 2015-10-13 8:44 ` Michael S. Tsirkin 2015-10-13 14:31 ` Marcel Apfelbaum 2015-10-13 14:47 ` Michael S. Tsirkin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.