From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60054) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XN792-0005Zs-4E for qemu-devel@nongnu.org; Thu, 28 Aug 2014 17:22:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XN78t-0008OF-36 for qemu-devel@nongnu.org; Thu, 28 Aug 2014 17:22:12 -0400 Received: from e35.co.us.ibm.com ([32.97.110.153]:53761) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XN78s-0008O8-TE for qemu-devel@nongnu.org; Thu, 28 Aug 2014 17:22:03 -0400 Received: from /spool/local by e35.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 28 Aug 2014 15:22:01 -0600 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <20140827134751.GA18523@redhat.com> References: <1408407718-10835-1-git-send-email-mdroth@linux.vnet.ibm.com> <1408407718-10835-9-git-send-email-mdroth@linux.vnet.ibm.com> <20140827134751.GA18523@redhat.com> Message-ID: <20140828212157.21832.67572@loki> Date: Thu, 28 Aug 2014 16:21:57 -0500 Subject: Re: [Qemu-devel] [PATCH 08/12] pci: allow 0 address for PCI IO regions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: aik@ozlabs.ru, qemu-devel@nongnu.org, agraf@suse.de, ncmike@ncultra.org, qemu-ppc@nongnu.org, tyreld@linux.vnet.ibm.com, nfont@linux.vnet.ibm.com Quoting Michael S. Tsirkin (2014-08-27 08:47:51) > On Mon, Aug 18, 2014 at 07:21:54PM -0500, Michael Roth wrote: > > Some kernels program a 0 address for io regions. PCI 3.0 spec > > section 6.2.5.1 doesn't seem to disallow this. > > = > > Signed-off-by: Michael Roth > = > Yes the PCI spec does not care. > = > But unfortunately as documented in the comment, at > least for PC (maybe others) priorities aren't > currently setup correctly, so programming PCI BAR at > address zero (during sizing) conflicts with > whatever else is there. I'm not sure I understand: that note was included as part of the following fixup to 9f1a029abf15751e32a4b1df99ed2b8315f9072c: - if (last_addr <=3D new_addr || new_addr =3D=3D 0) { + /* Check if BAR is being sized explicitly. + * TODO: make priorities correct and remove this work around. + */ + if (last_addr <=3D new_addr || new_addr =3D=3D 0 || last_addr >=3D= UINT32_MAX) which forces the BAR to PCI_BAR_UNMAPPED and unmaps the io region if the address range extends beyond UINT32_MAX (which would happen during sizing when guest writes -1...and I guess maybe last_addr <=3D new_addr covered the same case back when we used uint32_t for pcibus_t?) ... But the (new_addr =3D=3D 0) seems to be something unrelated..., it means the guest actually attempted to program a 0 address, or... since pci_update_mappings unconditionally updates all IO regions for a device whenever a particular BAR is written to, it would prevent us from temporarily mapping all the IO regions to 0 (until guest re-assigns them) ... You mentioned in the past this could lead to dispatch tables getting permanantly corrupted, so maybe that's what the check was for? But I guess there's still a separate issue, where there's a high liklihood = that a 0 address would conflict with some hard-wired IO address? Wouldn't this b= e a guest bug though? Well, I guess it would be a QEMU bug if the above scenario is a real one...but if we fix or verify that's not the case, would this be an acceptable change? > = > To make address 0 work, you'll have to fix up the prioriorities for a > bunch of machine types :( > = > > --- > > hw/pci/pci.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > = > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > > index 351d320..9578749 100644 > > --- a/hw/pci/pci.c > > +++ b/hw/pci/pci.c > > @@ -1035,7 +1035,7 @@ static pcibus_t pci_bar_address(PCIDevice *d, > > /* Check if 32 bit BAR wraps around explicitly. > > * TODO: make priorities correct and remove this work around. > > */ > > - if (last_addr <=3D new_addr || new_addr =3D=3D 0 || last_addr = >=3D UINT32_MAX) { > > + if (last_addr <=3D new_addr || last_addr >=3D UINT32_MAX) { > > return PCI_BAR_UNMAPPED; > > } > > return new_addr; > > -- = > > 1.9.1 > >