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

Enable PCI-e device multifunction hot, just ensure the function 0
added last, then driver will got the notification to scan all the
function in the slot.

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

On Thu, 2015-09-10 at 20:12 +0800, Cao jin wrote:
> Enable PCI-e device multifunction hot, just ensure the function 0
> added last, then driver will got the notification to scan all the
> function in the slot.
> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
>  hw/pci/pcie.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 6e28985..61ebefd 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -249,16 +249,20 @@ 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_dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) &&
> +            PCI_FUNC(pci_dev->devfn) > 0) {

The PCI spec is actually not entirely clear about whether each function
in a multifunction device needs to have the multifunction bit set or if
only function 0 needs to set it.  From a discovery perspective, a kernel
should only scan for function 0 and only scan non-zero funcions if
function 0 reports the multifunction bit set.  Therefore, it doesn't
particularly matter what non-zero functions report for the multifunction
bit.  QEMU allows either interpretation (see comment in
pci_init_multifunction), so this appears to be a new requirement that
breaks assumptions elsewhere.  Thanks,

Alex

> +        error_setg(errp, "single function device, function number must be 0,"
> +                "but got %d", PCI_FUNC(pci_dev->devfn));
> +    } else 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,

Hi, Alex,
     Thanks very much for your quick review and I am sorry for my late 
response.

On 09/10/2015 11:29 PM, Alex Williamson wrote:
> On Thu, 2015-09-10 at 20:12 +0800, Cao jin wrote:
>> Enable PCI-e device multifunction hot, just ensure the function 0
>> added last, then driver will got the notification to scan all the
>> function in the slot.
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   hw/pci/pcie.c | 22 +++++++++++++---------
>>   1 file changed, 13 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index 6e28985..61ebefd 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -249,16 +249,20 @@ 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_dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) &&
>> +            PCI_FUNC(pci_dev->devfn) > 0) {
>
> The PCI spec is actually not entirely clear about whether each function
> in a multifunction device needs to have the multifunction bit set or if
> only function 0 needs to set it.  From a discovery perspective, a kernel
> should only scan for function 0 and only scan non-zero funcions if
> function 0 reports the multifunction bit set.  Therefore, it doesn't
> particularly matter what non-zero functions report for the multifunction
> bit.  QEMU allows either interpretation (see comment in
> pci_init_multifunction), so this appears to be a new requirement that
> breaks assumptions elsewhere.  Thanks,
>
> Alex
>
Yes, PCI Spec does not describe it clearly. The if() here, is based on 
the condition like following:

   (qemu) device_add e1000,bus=pb1,id=net1,addr=00.1
   (qemu) device_add e1000,bus=pb1,id=net0,addr=00.0
   PCI: 0.0 indicates single function, but 0.1 is already populated.

Now, we can see the net1 via "info pci" while can`t see net1 in guest 
via "lspci -t", so I think maybe it is not good condition, but no big 
deal, so I use the if() to prevent the condition.

will remove the if() in next version.

>> +        error_setg(errp, "single function device, function number must be 0,"
>> +                "but got %d", PCI_FUNC(pci_dev->devfn));
>> +    } else 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,
>
>
>
> .
>