From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MxJNW-0001J1-Ne for qemu-devel@nongnu.org; Mon, 12 Oct 2009 07:47:50 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MxJNS-0001IK-Ai for qemu-devel@nongnu.org; Mon, 12 Oct 2009 07:47:50 -0400 Received: from [199.232.76.173] (port=37157 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MxJNS-0001IH-0I for qemu-devel@nongnu.org; Mon, 12 Oct 2009 07:47:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36845) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MxJNR-0005BL-CW for qemu-devel@nongnu.org; Mon, 12 Oct 2009 07:47:45 -0400 Date: Mon, 12 Oct 2009 13:45:37 +0200 From: "Michael S. Tsirkin" Message-ID: <20091012114537.GA12691@redhat.com> References: <1255287547-28329-1-git-send-email-gleb@redhat.com> <1255287547-28329-3-git-send-email-gleb@redhat.com> <20091011215356.GC6411@redhat.com> <20091012065024.GS16702@redhat.com> <20091012095225.GA11741@redhat.com> <20091012100821.GE16702@redhat.com> <20091012110335.GA12546@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20091012110335.GA12546@redhat.com> Subject: [Qemu-devel] Re: [PATCH 3/5] Use the correct mask to size the PCI option ROM BAR. List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gleb Natapov Cc: kevin@koconnor.net, qemu-devel@nongnu.org On Mon, Oct 12, 2009 at 01:03:36PM +0200, Michael S. Tsirkin wrote: > On Mon, Oct 12, 2009 at 12:08:21PM +0200, Gleb Natapov wrote: > > On Mon, Oct 12, 2009 at 11:52:25AM +0200, Michael S. Tsirkin wrote: > > > On Mon, Oct 12, 2009 at 08:50:24AM +0200, Gleb Natapov wrote: > > > > Send patch with your favorite interpretation to qemu pcbios/seabios. > > > > The regression concern from my previous mail applicable here as well. > > > > > > Okay. Can you ack the following? > > > > > I can if you'll add PCI spec reference for me to double check. > > > > Also I prefer strict spec reading :) > > OK, the issue is that reserved bits in BARs are not > defined as read-only. So here's a strict one: > can you ack? Actually, that's not right either. It says on page 214: Software must take care to deal correctly with bit-encoded fields that have some bits reserved for future use. On reads, software must use appropriate masks to extract the defined bits, and may not rely on reserved bits being any particular value. On writes, software must ensure that the values of reserved bit positions are preserved; that is, the values of reserved bit positions must first be read, merged with the new values for other bit positions and the data then written back. So let's do this. Patch forthcoming. > ---> > > seabios: fix ROM and I/O sizing > > For ROM BARs, bit 0 is writeable (enable bit), which we not > only don't want to set, but it will stick and make us think > it's an I/O port resource. > Further, PCI spec defines the following bits as reserved: > - bit 1 in I/O BAR > - bits 10:1 in ROM BAR > and we should be careful and write 0 there. > For memory, bits 0-3 are reserved, so it's safe to handle it > in the same way as I/O. > > See 6.2.5.1 for I/O and memory, and 6.2.5.2 for ROM; > pages 225 and 228 in PCI spec revision 3.0. > > See also Qemu pcbios commit 6ddb9f5c742b2b82b1755d7ec2a127f6e20e3806 > > Signed-off-by: Michael S. Tsirkin > --- > src/pciinit.c | 8 +++++--- > 1 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/src/pciinit.c b/src/pciinit.c > index 1d0f784..29b3901 100644 > --- a/src/pciinit.c > +++ b/src/pciinit.c > @@ -139,11 +139,13 @@ static void pci_bios_init_device(u16 bdf) > int ofs; > u32 val, size; > > - if (i == PCI_ROM_SLOT) > + if (i == PCI_ROM_SLOT) { > ofs = PCI_ROM_ADDRESS; > - else > + pci_config_writel(bdf, ofs, PCI_ROM_ADDRESS_MASK); > + } else { > ofs = PCI_BASE_ADDRESS_0 + i * 4; > - pci_config_writel(bdf, ofs, 0xffffffff); > + pci_config_writel(bdf, ofs, PCI_BASE_ADDRESS_IO_MASK); > + } > val = pci_config_readl(bdf, ofs); > if (val != 0) { > size = (~(val & ~0xf)) + 1; > -- > 1.6.3.3 > > >