[2/2] PCI-e device multi-function hot-add support

In case user regret when hot-add multi-function, we should roll back,
device_del the function added but still not worked.

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 hw/pci/pcie.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

On Thu, 2015-09-10 at 20:12 +0800, Cao jin wrote:
> In case user regret when hot-add multi-function, we should roll back,
> device_del the function added but still not worked.
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/pci/pcie.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 61ebefd..b83a244 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -265,10 +265,33 @@ 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;
> +
> +    /* handle the condition: user want to hot-add multi function, but regret
> +     * before finish it, and want to delete the added but not worked function.
> +     */
> +    if (PCI_FUNC(pci_dev->devfn) > 0 &&
> +        bus->devices[PCI_DEVFN(0,0)] == NULL) {
> +        pci_for_each_device(bus, pci_bus_num(bus),
> +                            pcie_unplug_device, NULL);
> +
> +        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
> +                                     PCI_EXP_SLTSTA_PDS);
> +        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> +                                   PCI_EXP_SLTSTA_PDC);
> +
> +        return;
> +    }

How can we know that the guest hasn't discovered the device and made use
of it?  The device is fully accessible to the guest even is the
notification and standard discovery mechanism are not in place.  A
gratuitous full PCI bus scan in the guest could find the device.
Thanks,

Alex

>  
>      pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp);
>  
> @@ -382,11 +405,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)
>  {

On Thu, 10 Sep 2015 20:12:23 +0800
Cao jin <caoj.fnst@cn.fujitsu.com> wrote:

> In case user regret when hot-add multi-function, we should roll back,
> device_del the function added but still not worked.
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/pci/pcie.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 61ebefd..b83a244 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -265,10 +265,33 @@ 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;
> +
> +    /* handle the condition: user want to hot-add multi function, but regret
> +     * before finish it, and want to delete the added but not worked function.
> +     */
sorry, I couldn't parse this comment and commit message as well. Please rephrase it.

> +    if (PCI_FUNC(pci_dev->devfn) > 0 &&
> +        bus->devices[PCI_DEVFN(0,0)] == NULL) {
> +        pci_for_each_device(bus, pci_bus_num(bus),
> +                            pcie_unplug_device, NULL);
*_unplug_request_cb is a way to communicate to guest that it should free
and eject device. So you are not allowed to destroy device from this path,
it's upto guest to decide what do on this request.

Look where pcie_unplug_device() is actually used, that's the place where
guest voluntary ejects device.

> +
> +        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
> +                                     PCI_EXP_SLTSTA_PDS);
> +        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
> +                                   PCI_EXP_SLTSTA_PDC);
> +
> +        return;
> +    }
>  
>      pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp);
>  
> @@ -382,11 +405,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)
>  {

Hi Igor,
     sorry I missed you mail.

On 09/11/2015 10:35 PM, Igor Mammedov wrote:
> On Thu, 10 Sep 2015 20:12:23 +0800
> Cao jin <caoj.fnst@cn.fujitsu.com> wrote:
>
>> In case user regret when hot-add multi-function, we should roll back,
>> device_del the function added but still not worked.
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/pci/pcie.c | 28 +++++++++++++++++++++++-----
>>   1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index 61ebefd..b83a244 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -265,10 +265,33 @@ 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;
>> +
>> +    /* handle the condition: user want to hot-add multi function, but regret
>> +     * before finish it, and want to delete the added but not worked function.
>> +     */
> sorry, I couldn't parse this comment and commit message as well. Please rephrase it.
>

Ok, I send v2 before saw your mail, sorry. Maybe I can rephrase it in v3.

>> +    if (PCI_FUNC(pci_dev->devfn) > 0 &&
>> +        bus->devices[PCI_DEVFN(0,0)] == NULL) {
>> +        pci_for_each_device(bus, pci_bus_num(bus),
>> +                            pcie_unplug_device, NULL);
> *_unplug_request_cb is a way to communicate to guest that it should free
> and eject device. So you are not allowed to destroy device from this path,
> it's upto guest to decide what do on this request.
>
> Look where pcie_unplug_device() is actually used, that's the place where
> guest voluntary ejects device.
>

Agree. In v2 patch, I changed the delete process, fake the condition, 
let the Guest voluntary ejects device

>> +
>> +        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
>> +                                     PCI_EXP_SLTSTA_PDS);
>> +        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
>> +                                   PCI_EXP_SLTSTA_PDC);
>> +
>> +        return;
>> +    }
>>
>>       pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp);
>>
>> @@ -382,11 +405,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)
>>   {
>
> .
>