Patchwork Re: [PATCH 3/5] Use the correct mask to size the PCI option ROM BAR.

login
register
mail settings
Submitter Michael S. Tsirkin
Date Oct. 12, 2009, 11:03 a.m.
Message ID <20091012110335.GA12546@redhat.com>
Download mbox | patch
Permalink /patch/35747/
State New
Headers show

Comments

Michael S. Tsirkin - Oct. 12, 2009, 11:03 a.m.
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.

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(-)
Michael S. Tsirkin - Oct. 12, 2009, 11:45 a.m.
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
> 
> 
>
Gleb Natapov - Oct. 12, 2009, 11:48 a.m.
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.
Michael S. Tsirkin - Oct. 12, 2009, 11:59 a.m.
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.
Gleb Natapov - Oct. 12, 2009, 12:08 p.m.
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.

Patch

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;