[v3,2/2] remove function during multi-function hot-add

Message ID	1444725695-27517-3-git-send-email-caoj.fnst@cn.fujitsu.com
State	New
Headers	show Return-Path: <qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org> From: Cao jin <caoj.fnst@cn.fujitsu.com> To: <qemu-devel@nongnu.org> Date: Tue, 13 Oct 2015 16:41:35 +0800 Message-ID: <1444725695-27517-3-git-send-email-caoj.fnst@cn.fujitsu.com> In-Reply-To: <1444725695-27517-1-git-send-email-caoj.fnst@cn.fujitsu.com> References: <1444725695-27517-1-git-send-email-caoj.fnst@cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain Cc: pbonzini@redhat.com, alex.williamson@redhat.com, izumi.taku@jp.fujitsu.com, mst@redhat.com Subject: [Qemu-devel] [PATCH v3 2/2] remove function during multi-function hot-add Precedence: list Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org

Cao jin Oct. 13, 2015, 8:41 a.m. UTC

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(-)

Michael S. Tsirkin Oct. 13, 2015, 8:48 a.m. UTC | #1

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

Michael S. Tsirkin Oct. 13, 2015, 8:51 a.m. UTC | #2

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

Cao jin Oct. 13, 2015, 9:54 a.m. UTC | #3

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
> .
>

Michael S. Tsirkin Oct. 13, 2015, 10:21 a.m. UTC | #4

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

Cao jin Oct. 13, 2015, 12:19 p.m. UTC | #5

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
> .
>

Alex Williamson Oct. 13, 2015, 3:27 p.m. UTC | #6

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)
>  {

Cao jin Oct. 14, 2015, 5:46 a.m. UTC | #7

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)
>>   {
>
>
>
> .
>

[v3,2/2] remove function during multi-function hot-add

Commit Message

Comments

Patch