From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57128) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZeKQ8-0006eD-KG for qemu-devel@nongnu.org; Tue, 22 Sep 2015 06:03:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZeKQ3-0005oW-Vf for qemu-devel@nongnu.org; Tue, 22 Sep 2015 06:03:32 -0400 Received: from [59.151.112.132] (port=42801 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZeKQ2-0005iN-11 for qemu-devel@nongnu.org; Tue, 22 Sep 2015 06:03:27 -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> From: Cao jin Message-ID: <5601288E.4020305@cn.fujitsu.com> Date: Tue, 22 Sep 2015 18:08:14 +0800 MIME-Version: 1.0 In-Reply-To: <1442858455.23936.335.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/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 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? >> + >> + 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