From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39797) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zek9z-0007oZ-6Z for qemu-devel@nongnu.org; Wed, 23 Sep 2015 09:32:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zek9u-0002rG-HG for qemu-devel@nongnu.org; Wed, 23 Sep 2015 09:32:35 -0400 Received: from [59.151.112.132] (port=54111 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zek9t-0002ov-K9 for qemu-devel@nongnu.org; Wed, 23 Sep 2015 09:32:30 -0400 References: <1442368976-15852-1-git-send-email-caoj.fnst@cn.fujitsu.com> <1442368976-15852-3-git-send-email-caoj.fnst@cn.fujitsu.com> <1442858455.23936.335.camel@redhat.com> <5601288E.4020305@cn.fujitsu.com> <1442944293.23936.387.camel@redhat.com> From: Cao jin Message-ID: <5602AB18.7050401@cn.fujitsu.com> Date: Wed, 23 Sep 2015 21:37:28 +0800 MIME-Version: 1.0 In-Reply-To: <1442944293.23936.387.camel@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 2/2] PCI-e device multi-function hot-add support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, izumi.taku@jp.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 >>>> --- >>>> 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