| Message ID | 20091012110335.GA12546@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <mst@redhat.com> > --- > 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 > > >
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? > > ---> > > 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. > Section 6.2.5.1 says: Software saves the original value of the Base Address register, writes 0 FFFF FFFFh to the register, then reads it back. Section 6.2.5.2 says: Device independent configuration software can determine how much address space the device requires by writing a value of all 1's to the address portion of the register and then reading the value back. (address portion are bits 31-11). So we should write PCI_ROM_ADDRESS_MASK in case of ROM and 0xffffffff in case of regular BAR. > See also Qemu pcbios commit 6ddb9f5c742b2b82b1755d7ec2a127f6e20e3806 > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > 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 > > -- Gleb.
On Mon, Oct 12, 2009 at 01:48:41PM +0200, Gleb Natapov wrote: > 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? > > > > ---> > > > > 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. > > > Section 6.2.5.1 says: > Software saves the original value of the Base Address register, writes > 0 FFFF FFFFh to the register, then reads it back. I think you miss something. Here it is in full: Decode (I/O or memory) of a register is disabled via the command register before sizing a Base Address register. Software saves the original value of the Base Address register, writes 0 FFFF FFFFh to the register, then reads it back. Size calculation can be done from the 32-bit value read by first clearing encoding information bits (bit 0 for I/O, bits 0-3 for memory), inverting all 32 bits (logical NOT), then incrementing by 1. The resultant 32-bit value is the memory/I/O range size decoded by the register. Note that the upper 16 bits of the result is ignored if the Base Address register is for I/O and bits 16-31 returned zero upon read. The original value in the Base Address register is restored before re-enabling decode in the command register of the device. Note the bit about restoring back the original value. You can not assume that reserved bits are read-only. > Section 6.2.5.2 says: > Device independent configuration software can determine how much address > space the device requires by writing a value of all 1's to the address > portion of the register and then reading the value back. (address > portion are bits 31-11). We must also save and restore the lower bits. > So we should write PCI_ROM_ADDRESS_MASK in case of ROM and 0xffffffff > in case of regular BAR. > > > See also Qemu pcbios commit 6ddb9f5c742b2b82b1755d7ec2a127f6e20e3806 > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > 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 > > > > > > -- > Gleb.
On Mon, Oct 12, 2009 at 01:59:16PM +0200, Michael S. Tsirkin wrote: > On Mon, Oct 12, 2009 at 01:48:41PM +0200, Gleb Natapov wrote: > > 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? > > > > > > ---> > > > > > > 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. > > > > > Section 6.2.5.1 says: > > Software saves the original value of the Base Address register, writes > > 0 FFFF FFFFh to the register, then reads it back. > > I think you miss something. Here it is in full: > > Decode (I/O or memory) of a register is disabled via the command register before sizing a > Base Address register. Software saves the original value of the Base Address register, writes > 0 FFFF FFFFh to the register, then reads it back. Size calculation can be done from the > 32-bit value read by first clearing encoding information bits (bit 0 for I/O, bits 0-3 for > memory), inverting all 32 bits (logical NOT), then incrementing by 1. The resultant 32-bit > value is the memory/I/O range size decoded by the register. Note that the upper 16 bits of > the result is ignored if the Base Address register is for I/O and bits 16-31 returned zero > upon read. The original value in the Base Address register is restored before re-enabling > decode in the command register of the device. > > Note the bit about restoring back the original value. > You can not assume that reserved bits are read-only. > I assume nothing. I am saying the code was correct in writing 0xffffffff there. If coded does not restore original value that is another issue. > > Section 6.2.5.2 says: > > Device independent configuration software can determine how much address > > space the device requires by writing a value of all 1's to the address > > portion of the register and then reading the value back. (address > > portion are bits 31-11). > > We must also save and restore the lower bits. > > > So we should write PCI_ROM_ADDRESS_MASK in case of ROM and 0xffffffff > > in case of regular BAR. > > > > > See also Qemu pcbios commit 6ddb9f5c742b2b82b1755d7ec2a127f6e20e3806 > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > --- > > > 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 > > > > > > > > > > -- > > Gleb. -- Gleb.
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;