From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51247) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WyftH-00037j-3n for qemu-devel@nongnu.org; Sun, 22 Jun 2014 07:25:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WyftA-0005UZ-TM for qemu-devel@nongnu.org; Sun, 22 Jun 2014 07:24:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:8451) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WyftA-0005UU-Ke for qemu-devel@nongnu.org; Sun, 22 Jun 2014 07:24:48 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s5MBOlIB003554 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Sun, 22 Jun 2014 07:24:48 -0400 Message-ID: <1403436283.2109.20.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Sun, 22 Jun 2014 14:24:43 +0300 In-Reply-To: <20140622111254.GA31022@redhat.com> References: <1403185941-19561-1-git-send-email-marcel.a@redhat.com> <1403185941-19561-4-git-send-email-marcel.a@redhat.com> <20140619144358.GB10359@redhat.com> <1403434446.2109.9.camel@localhost.localdomain> <20140622110359.GA22355@redhat.com> <1403435465.2109.13.camel@localhost.localdomain> <20140622111254.GA31022@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/3] hw/pcie: better hotplug/hotunplug support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org On Sun, 2014-06-22 at 14:12 +0300, Michael S. Tsirkin wrote: > On Sun, Jun 22, 2014 at 02:11:05PM +0300, Marcel Apfelbaum wrote: > > On Sun, 2014-06-22 at 14:03 +0300, Michael S. Tsirkin wrote: > > > On Sun, Jun 22, 2014 at 01:54:06PM +0300, Marcel Apfelbaum wrote: > > > > On Thu, 2014-06-19 at 17:43 +0300, Michael S. Tsirkin wrote: > > > > > On Thu, Jun 19, 2014 at 04:52:21PM +0300, Marcel Apfelbaum wrote: > > > > > > Hotplug triggers both 'present detect change' and > > > > > > 'attention button pressed'. > > > > > > > > > > > > Hotunplug starts by triggering 'attention button pressed', > > > > > > then waits for the OS to power off the device and only > > > > > > then detaches it. > > > > > > > > > > > > > > > > Pls not here that current code is broken: it does surprise removal which > > > > > crashes guests. > > > > I'll add a note, sure. > > > > > > > > > > > > > > > Signed-off-by: Marcel Apfelbaum > > > > > > --- > > > > > > hw/pci/pcie.c | 20 +++++++++++++++----- > > > > > > 1 file changed, 15 insertions(+), 5 deletions(-) > > > > > > > > > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > > > > > index f8bf515..9cfd93d 100644 > > > > > > --- a/hw/pci/pcie.c > > > > > > +++ b/hw/pci/pcie.c > > > > > > @@ -258,7 +258,8 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > > > > > > > > > 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); > > > > > > + pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), > > > > > > + PCI_EXP_HP_EV_PDC | PCI_EXP_HP_EV_ABP); > > > > > > } > > > > > > > > > > > > void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > > > @@ -268,10 +269,7 @@ void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > > > > > > > > > pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp); > > > > > > > > > > > > - object_unparent(OBJECT(dev)); > > > > > > - pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > > > > > > - PCI_EXP_SLTSTA_PDS); > > > > > > - pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC); > > > > > > + pcie_cap_slot_push_attention_button(PCI_DEVICE(hotplug_dev)); > > > > > > } > > > > > > > > > > > > /* pci express slot for pci express root/downstream port > > > > > > @@ -389,6 +387,18 @@ void pcie_cap_slot_write_config(PCIDevice *dev, > > > > > > sltsta); > > > > > > } > > > > > > > > > > > > > > > > pls add code comments explaining the logic here. > > > > I thought is clear :( > > > > Basically: If the device is present, power indicator off and power > > > > controller off, it is safe to detach the device. > > > > > > Sure, put this in the comment. > > > > > > > > > > > > > > + if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && > > > > > > + ((val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF)) { > > > > > > + PCIDevice *slot_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0]; > > > > > > + if (slot_dev) { > > > > > > > > > > Here you want to remove all devices behind the bridge? > > > > Yes, but since it is PCIe we only have one device, > > > > but we may have a multi-function device... > > > > > > Exactly. Up to 256 functions. > > > > > > > > You need to do this for all functions, not just function 0. > > > > So bus devices are actually functions... OK, I'll run a loop here. > > > > > > > > > > > > > > > + object_unparent(OBJECT(slot_dev)); > > > > > > + 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); > > > > > > > > > > These bits need to be cleared in any case? > > > > No, "PDS on" means the devices is present, so we clear it only after the > > > > OS powers it off. > > > > > > So why not clear PDS unconditionally? > > Because we are 'allowed' to remove the device only when the OS tells us > > the power is off. Before that, it can result in data loss, > > since the device driver is not yet unloaded. > > Yes. But if (slot_dev) seems not to be needed: > if slot is empty we can clear PDC. You mean PDS, PDC is the event we send to the OS. The main reason I added this "if clause" is to protect against multiple OS events of 'power controller off' and 'power indicator off', but now I see that indeed I don't need that because I check "sltsta & PCI_EXP_SLTSTA_PDS". I will remove that if. Thanks, Marcel > > > > > > > > > > > > > > > > + } > > > > > > + } > > > > > > + > > > > > > hotplug_event_notify(dev); > > > > > > > > > > > > /* > > > > > > -- > > > > > > 1.8.3.1 > > > > > > > > > > > >