* 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ 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; 17+ 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] 17+ messages in thread