Re: [Qemu-devel] [PATCH v2 2/2] PCI-e device multi-function hot-add support

[Cao jin <caoj.fnst@cn.fujitsu.com>]

Hi Alex,

On 09/23/2015 01:51 AM, Alex Williamson wrote:
> On Tue, 2015-09-22 at 18:08 +0800, Cao jin wrote:
>> Hi Alex
>>
>> On 09/22/2015 02:00 AM, Alex Williamson wrote:
>>>
>>> Please use different subjects that uniquely identify what each patch
>>> does, don't simply re-use the subject for the cover patch on each.
>>
>> OK, will change it in next version.
>>>
>>> On Wed, 2015-09-16 at 10:02 +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 | 18 ++++++++++++++++++
>>>>    1 file changed, 18 insertions(+)
>>>>
>>>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>>>> index 89bf61b..497f390 100644
>>>> --- a/hw/pci/pcie.c
>>>> +++ b/hw/pci/pcie.c
>>>> @@ -265,9 +265,27 @@ 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);
>>>>
>>>> +    /* handle the condition: user hot-add multi function, but regret before
>>>> +     * finish it, and want to delete the added but not worked function. Fake
>>>> +     * the condition: the slot is polulated, power indicator is off and power
>>>> +     * controller is off, so device can be detached when OS write config space.
>>>> +     */
>>>> +    if (PCI_FUNC(pci_dev->devfn) > 0 &&
>>>> +            bus->devices[PCI_DEVFN(0, 0)] == NULL) {
>>>> +        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
>>>> +                PCI_EXP_SLTSTA_PDS);
>>>
>>> AFAICT, we're only setting this to make pcie_cap_slot_write_config()
>>> consider this device for being unplugged.  Would it not be cleaner to
>>> flag the device as unexposed to the guest and also use that flag to
>>> prevent config reads and writes to the device until function 0 is
>>> populated, so we know that the guest hasn't interacted with the device?
>>>
>> Yes, set PDS bit here, for the purpose that fake the unplug condition in
>> pcie_cap_slot_write_config(), which means, let guest decide when to
>> unplug device. So I think setting PDS bit here is necessary, am I right?
>
> I would consider it a hack.  You're setting up the device a certain way
> to make it appear as if the guest has configured it that way, then
> effectively sending the guest a spurious hotplug request for a device
> that it theoretically doesn't know about.  If we were to prevent access
> to the device, couldn't we remove it directly?
>

I agree with the judgement "hack", but I am confused about the last 
sentence. please correct me if I understand it wrong.
I design the hot-add feature via executing device_add cmd several times 
with func 0 added last. Assume we have a solution implemented to prevent 
access to the device before adding func 0, but we mustn`t remove other 
func directly, because we don`t know whether user want to add func 0 at 
last or just regret.

>> I am not quite clear about "flag device as unexposed", does the flag
>> means PCI_EXP_SLTSTA_PDS bit, or anything else? Could you give more
>> hints about it?
>
> If function 0 doesn't exist in the slot, should the guest be able to
> perform PCI config accesses to the device?  If the guest cannot do
> config cycle accesses to the device, then we know the device is unused
> and we don't need to involve the guest in removing it.

if func 0 doesn`t exist, theoretically as I think, guest has no reason 
to perform PCI config access to the device, but as you said before, if 
guest does do a gratuitous full PCI bus scan(actually I am not aware in 
what condition it will happen), guest is able to find the device without 
func 0 exist.

in the condition you said above, assume we already have the solution to 
forbidden the access to device before func 0 added, does that means the 
result: guest think there is no device in the slot, but in qemu, we 
still have device data structure in, and won`t destroy it?

or I have another solution of this feature: make multi-function hot-add 
atomic, which means creating a new api, with all func params following, 
like "multifunction_device_add func0,func1,func2...", but it will be 
more and more complicated, which maybe the last solution I prefer to choose.

another question: in what way do we flag the device unexposed to guest 
before func 0 populated? My thoughts is: return 0xFFFF as vendor id when 
being accessed, do you think it is a effective way?
>>>> +
>>>> +        pcie_cap_slot_event(PCI_DEVICE(hotplug_dev),
>>>> +                PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP);
>>>
>>> Why do we need to test both vs just ABP, which is signaled in the
>>> existing patch below?
>>>
>>
>> Test the two hotplug event, yes, ABP is enough for device_del. will
>> remove PDC in next version.
>>
>>>> +
>>>> +        return;
>>>> +    }
>>>> +
>>>>        pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev));
>>>>    }
>>>>
>>>
>>>
>>> .
>>>
>>
>
>
>
> .
>

-- 
Yours Sincerely,

Cao Jin