From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50017) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wyffp-000784-N4 for qemu-devel@nongnu.org; Sun, 22 Jun 2014 07:11:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wyffg-0001he-Aa for qemu-devel@nongnu.org; Sun, 22 Jun 2014 07:11:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:10662) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wyffg-0001hW-1s for qemu-devel@nongnu.org; Sun, 22 Jun 2014 07:10:52 -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 s5MBApIJ029708 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Sun, 22 Jun 2014 07:10:51 -0400 Date: Sun, 22 Jun 2014 14:11:18 +0300 From: "Michael S. Tsirkin" Message-ID: <20140622111118.GA27736@redhat.com> References: <1403185941-19561-1-git-send-email-marcel.a@redhat.com> <1403185941-19561-3-git-send-email-marcel.a@redhat.com> <20140619143915.GA10359@redhat.com> <1403434044.2109.3.camel@localhost.localdomain> <20140622105246.GA18067@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140622105246.GA18067@redhat.com> Subject: Re: [Qemu-devel] [PATCH 2/3] hw/pcie: implement power controller functionality List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum Cc: qemu-devel@nongnu.org On Sun, Jun 22, 2014 at 01:52:46PM +0300, Michael S. Tsirkin wrote: > On Sun, Jun 22, 2014 at 01:47:24PM +0300, Marcel Apfelbaum wrote: > > On Thu, 2014-06-19 at 17:39 +0300, Michael S. Tsirkin wrote: > > > On Thu, Jun 19, 2014 at 04:52:20PM +0300, Marcel Apfelbaum wrote: > > > > It is needed by hot-unplug in order to get an indication > > > > from the OS when the device can be physically detached. > > > > > > > > Signed-off-by: Marcel Apfelbaum > > > > --- > > > > hw/pci/pcie.c | 15 ++++++++++++++- > > > > include/hw/pci/pcie_regs.h | 2 ++ > > > > 2 files changed, 16 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > > > index ae92f00..f8bf515 100644 > > > > --- a/hw/pci/pcie.c > > > > +++ b/hw/pci/pcie.c > > > > @@ -292,16 +292,19 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot) > > > > PCI_EXP_SLTCAP_HPC | > > > > PCI_EXP_SLTCAP_PIP | > > > > PCI_EXP_SLTCAP_AIP | > > > > + PCI_EXP_SLTCAP_PCP | > > > > PCI_EXP_SLTCAP_ABP); > > > > > > > > pci_word_test_and_clear_mask(dev->config + pos + PCI_EXP_SLTCTL, > > > > PCI_EXP_SLTCTL_PIC | > > > > + PCI_EXP_SLTCTL_PCC | > > > > PCI_EXP_SLTCTL_AIC); > > > > pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_SLTCTL, > > > > PCI_EXP_SLTCTL_PIC_OFF | > > > > PCI_EXP_SLTCTL_AIC_OFF); > > > > pci_word_test_and_set_mask(dev->wmask + pos + PCI_EXP_SLTCTL, > > > > PCI_EXP_SLTCTL_PIC | > > > > + PCI_EXP_SLTCTL_PCC | > > > > PCI_EXP_SLTCTL_AIC | > > > > PCI_EXP_SLTCTL_HPIE | > > > > PCI_EXP_SLTCTL_CCIE | > > > > > > Need to disable for compat types? > > Does Paolo's explanation answer your question? > > Kind of hacky - we do have compat work for > q35. So I'd prefer consistency. > > > > > > > > > > > @@ -327,21 +330,31 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot) > > > > void pcie_cap_slot_reset(PCIDevice *dev) > > > > { > > > > uint8_t *exp_cap = dev->config + dev->exp.exp_cap; > > > > + PCIDevice *slot_dev = pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0]; > > > > > > What does this mean? > > > Downstream port? > > Yes > > well it's not really clear, needs a comment. > > > > A switch can have multiple downstream ports at any slot #. > > It doesn't matter how many devices are under this slot, we need only > > one to power up the slot. The question here was "Do we have at least > > one device attahc to slot? If yes, power up the slot." > > You need a loop for that I think. There's no guarantee the > device is at devfn=0. Unless something guarantees this is a downstream port, if yes maybe add an assert? slot_dev is also not a very good variable name. How about bool populated = pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0]; ? This is all you care about ... And maybe add a comment /* Downstream ports enforce device number 0. */ > > > > > > > + int pic; > > > > > > uint16_t please. > > Sure, > > > > > > > > > + > > > > + pic = slot_dev ? PCI_EXP_SLTCTL_PIC_ON : PCI_EXP_SLTCTL_PIC_OFF; > > > > > > > > PCIE_DEV_PRINTF(dev, "reset\n"); > > > > > > > > pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTCTL, > > > > PCI_EXP_SLTCTL_EIC | > > > > PCI_EXP_SLTCTL_PIC | > > > > + PCI_EXP_SLTCTL_PCC | > > > > PCI_EXP_SLTCTL_AIC | > > > > PCI_EXP_SLTCTL_HPIE | > > > > PCI_EXP_SLTCTL_CCIE | > > > > PCI_EXP_SLTCTL_PDCE | > > > > PCI_EXP_SLTCTL_ABPE); > > > > pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL, > > > > - PCI_EXP_SLTCTL_PIC_OFF | > > > > + pic | > > > > PCI_EXP_SLTCTL_AIC_OFF); > > > > > > > > + if (!slot_dev) { > > > > + pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL, > > > > + PCI_EXP_SLTCTL_PCC); > > > > > > I dislike it when we clear bits then set them back. > > > Please just add else here. > > Sure, > > > > > > > > > + } > > > > + > > > > > > Need to disable for compat types? > > As above, > > > > Thanks for the review, > > Marcel > > > > > > > > > pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > > > > PCI_EXP_SLTSTA_EIS |/* on reset, > > > > the lock is released */ > > > > diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h > > > > index 4d123d9..652d9fc 100644 > > > > --- a/include/hw/pci/pcie_regs.h > > > > +++ b/include/hw/pci/pcie_regs.h > > > > @@ -57,6 +57,8 @@ > > > > #define PCI_EXP_SLTCTL_PIC_SHIFT (ffs(PCI_EXP_SLTCTL_PIC) - 1) > > > > #define PCI_EXP_SLTCTL_PIC_OFF \ > > > > (PCI_EXP_SLTCTL_IND_OFF << PCI_EXP_SLTCTL_PIC_SHIFT) > > > > +#define PCI_EXP_SLTCTL_PIC_ON \ > > > > + (PCI_EXP_SLTCTL_IND_ON << PCI_EXP_SLTCTL_PIC_SHIFT) > > > > > > > > #define PCI_EXP_SLTCTL_SUPPORTED \ > > > > (PCI_EXP_SLTCTL_ABPE | \ > > > > -- > > > > 1.8.3.1 > > > >