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, 1:20 p.m.
Message ID <20091012132025.GA13022@redhat.com>
Download mbox | patch
Permalink /patch/35760/
State New
Headers show

Comments

Michael S. Tsirkin - Oct. 12, 2009, 1:20 p.m.
On Mon, Oct 12, 2009 at 02:08:57PM +0200, Gleb Natapov wrote:
> 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.


Right. And there's another bug that I see and that is that size for I/O
BAR was calculated incorrectly: val & ~0xf is wrong for I/O as the size
could be 4 bytes.  So here's a patch that addresses all 3 issues:
Ack?

---->

Subject: [PATCH] seabios: fix low bits in ROM and I/O sizing

This cleans up handling of low bits during BAR sizing,
to match PCI spec requirements, and to use symbolic
constants from pci_regs.h

Issues fixed:
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 preserve any values there.
Bits 3:2 in I/O BAR might be writeable, so it
is wrong to mask them when calculating BAR size.

Spec references:
See 6.2.5.1 for I/O and memory, and 6.2.5.2 for ROM,
6.1 for reserved bit handling;
pages 225, 228 and 214 in PCI spec revision 3.0.

See also Qemu pcbios commit 6ddb9f5c742b2b82b1755d7ec2a127f6e20e3806

Original-by: Gleb Natapov <gleb@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Gleb Natapov - Oct. 12, 2009, 1:29 p.m.
On Mon, Oct 12, 2009 at 03:20:27PM +0200, Michael S. Tsirkin wrote:
> Right. And there's another bug that I see and that is that size for I/O
> BAR was calculated incorrectly: val & ~0xf is wrong for I/O as the size
> could be 4 bytes.  So here's a patch that addresses all 3 issues:
> Ack?
> 
Pleas do whatever spec recommends and use 0xffffffff to size the BAR.
Specs says this, linux does this, lets not be different.

> 
> ---->
> 
> Subject: [PATCH] seabios: fix low bits in ROM and I/O sizing
> 
> This cleans up handling of low bits during BAR sizing,
> to match PCI spec requirements, and to use symbolic
> constants from pci_regs.h
> 
> Issues fixed:
> 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 preserve any values there.
> Bits 3:2 in I/O BAR might be writeable, so it
> is wrong to mask them when calculating BAR size.
> 
> Spec references:
> See 6.2.5.1 for I/O and memory, and 6.2.5.2 for ROM,
> 6.1 for reserved bit handling;
> pages 225, 228 and 214 in PCI spec revision 3.0.
> 
> See also Qemu pcbios commit 6ddb9f5c742b2b82b1755d7ec2a127f6e20e3806
> 
> Original-by: Gleb Natapov <gleb@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> diff --git a/src/pciinit.c b/src/pciinit.c
> index 7d2ea00..f9ebd61 100644
> --- a/src/pciinit.c
> +++ b/src/pciinit.c
> @@ -136,16 +136,23 @@ static void pci_bios_init_device(u16 bdf)
>          /* default memory mappings */
>          for (i = 0; i < PCI_NUM_REGIONS; i++) {
>              int ofs;
> -            u32 val, size;
> -
> +            u32 val, mask, size;
>              if (i == PCI_ROM_SLOT)
>                  ofs = PCI_ROM_ADDRESS;
>              else
>                  ofs = PCI_BASE_ADDRESS_0 + i * 4;
> -            pci_config_writel(bdf, ofs, 0xffffffff);
> +
> +            val = pci_config_readl(bdf, ofs);
> +            if (i == PCI_ROM_SLOT)
> +		mask = PCI_ROM_ADDRESS_MASK;
> +            else if (val & PCI_BASE_ADDRESS_SPACE_IO)
> +                mask = PCI_BASE_ADDRESS_IO_MASK;
> +            else
> +                mask = PCI_BASE_ADDRESS_MEM_MASK;
> +            pci_config_writel(bdf, ofs, val | mask);
>              val = pci_config_readl(bdf, ofs);
>              if (val != 0) {
> -                size = (~(val & ~0xf)) + 1;
> +                size = (~(val & mask)) + 1;
>                  if (val & PCI_BASE_ADDRESS_SPACE_IO)
>                      paddr = &pci_bios_io_addr;
>                  else if (size >= 0x04000000)
> -- 
> 1.6.5.rc2
> 

--
			Gleb.
Michael S. Tsirkin - Oct. 12, 2009, 1:51 p.m.
On Mon, Oct 12, 2009 at 03:29:25PM +0200, Gleb Natapov wrote:
> On Mon, Oct 12, 2009 at 03:20:27PM +0200, Michael S. Tsirkin wrote:
> > Right. And there's another bug that I see and that is that size for I/O
> > BAR was calculated incorrectly: val & ~0xf is wrong for I/O as the size
> > could be 4 bytes.  So here's a patch that addresses all 3 issues:
> > Ack?
> > 
> Pleas do whatever spec recommends and use 0xffffffff to size the BAR.

What we do is equivalent: since next thing we override the BAR value,
there's no reason for the save/restore dance.  But if you like it this
way, OK. Send a patch. Note this applies to I/O and memory only,
ROM has to be sized the way I coded it.

> Specs says this, linux does this, lets not be different.

BTW, note that what linux does for ROM is incorrect:
the implementation note you cite only applies to I/O and memory BARs.
I'll send a patch to lkml when I have the time.

> > 
> > ---->
> > 
> > Subject: [PATCH] seabios: fix low bits in ROM and I/O sizing
> > 
> > This cleans up handling of low bits during BAR sizing,
> > to match PCI spec requirements, and to use symbolic
> > constants from pci_regs.h
> > 
> > Issues fixed:
> > 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 preserve any values there.
> > Bits 3:2 in I/O BAR might be writeable, so it
> > is wrong to mask them when calculating BAR size.
> > 
> > Spec references:
> > See 6.2.5.1 for I/O and memory, and 6.2.5.2 for ROM,
> > 6.1 for reserved bit handling;
> > pages 225, 228 and 214 in PCI spec revision 3.0.
> > 
> > See also Qemu pcbios commit 6ddb9f5c742b2b82b1755d7ec2a127f6e20e3806
> > 
> > Original-by: Gleb Natapov <gleb@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > diff --git a/src/pciinit.c b/src/pciinit.c
> > index 7d2ea00..f9ebd61 100644
> > --- a/src/pciinit.c
> > +++ b/src/pciinit.c
> > @@ -136,16 +136,23 @@ static void pci_bios_init_device(u16 bdf)
> >          /* default memory mappings */
> >          for (i = 0; i < PCI_NUM_REGIONS; i++) {
> >              int ofs;
> > -            u32 val, size;
> > -
> > +            u32 val, mask, size;
> >              if (i == PCI_ROM_SLOT)
> >                  ofs = PCI_ROM_ADDRESS;
> >              else
> >                  ofs = PCI_BASE_ADDRESS_0 + i * 4;
> > -            pci_config_writel(bdf, ofs, 0xffffffff);
> > +
> > +            val = pci_config_readl(bdf, ofs);
> > +            if (i == PCI_ROM_SLOT)
> > +		mask = PCI_ROM_ADDRESS_MASK;
> > +            else if (val & PCI_BASE_ADDRESS_SPACE_IO)
> > +                mask = PCI_BASE_ADDRESS_IO_MASK;
> > +            else
> > +                mask = PCI_BASE_ADDRESS_MEM_MASK;
> > +            pci_config_writel(bdf, ofs, val | mask);
> >              val = pci_config_readl(bdf, ofs);
> >              if (val != 0) {
> > -                size = (~(val & ~0xf)) + 1;
> > +                size = (~(val & mask)) + 1;
> >                  if (val & PCI_BASE_ADDRESS_SPACE_IO)
> >                      paddr = &pci_bios_io_addr;
> >                  else if (size >= 0x04000000)
> > -- 
> > 1.6.5.rc2
> > 
> 
> --
> 			Gleb.
Gleb Natapov - Oct. 12, 2009, 2:04 p.m.
On Mon, Oct 12, 2009 at 03:51:12PM +0200, Michael S. Tsirkin wrote:
> On Mon, Oct 12, 2009 at 03:29:25PM +0200, Gleb Natapov wrote:
> > On Mon, Oct 12, 2009 at 03:20:27PM +0200, Michael S. Tsirkin wrote:
> > > Right. And there's another bug that I see and that is that size for I/O
> > > BAR was calculated incorrectly: val & ~0xf is wrong for I/O as the size
> > > could be 4 bytes.  So here's a patch that addresses all 3 issues:
> > > Ack?
> > > 
> > Pleas do whatever spec recommends and use 0xffffffff to size the BAR.
> 
> What we do is equivalent: since next thing we override the BAR value,
> there's no reason for the save/restore dance.  But if you like it this
That what spec says if you are not trying to interpret it too hard. And
simple is always better.

> way, OK. Send a patch. Note this applies to I/O and memory only,
> ROM has to be sized the way I coded it.
> 
> > Specs says this, linux does this, lets not be different.
> 
> BTW, note that what linux does for ROM is incorrect:
> the implementation note you cite only applies to I/O and memory BARs.
> I'll send a patch to lkml when I have the time.
> 
It does mask = type ? ~PCI_ROM_ADDRESS_ENABLE : ~0; so it definitely
treats ROM specially, You may argue that it shouldn't set bits 1-10
though.

The patch that currently in KVM bios (the one I've sent to seabios
initially) uses the same technique to size the BAR as Linux kernel. The only
thing missing is restoring of old value.

> > > 
> > > ---->
> > > 
> > > Subject: [PATCH] seabios: fix low bits in ROM and I/O sizing
> > > 
> > > This cleans up handling of low bits during BAR sizing,
> > > to match PCI spec requirements, and to use symbolic
> > > constants from pci_regs.h
> > > 
> > > Issues fixed:
> > > 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 preserve any values there.
> > > Bits 3:2 in I/O BAR might be writeable, so it
> > > is wrong to mask them when calculating BAR size.
> > > 
> > > Spec references:
> > > See 6.2.5.1 for I/O and memory, and 6.2.5.2 for ROM,
> > > 6.1 for reserved bit handling;
> > > pages 225, 228 and 214 in PCI spec revision 3.0.
> > > 
> > > See also Qemu pcbios commit 6ddb9f5c742b2b82b1755d7ec2a127f6e20e3806
> > > 
> > > Original-by: Gleb Natapov <gleb@redhat.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > 
> > > diff --git a/src/pciinit.c b/src/pciinit.c
> > > index 7d2ea00..f9ebd61 100644
> > > --- a/src/pciinit.c
> > > +++ b/src/pciinit.c
> > > @@ -136,16 +136,23 @@ static void pci_bios_init_device(u16 bdf)
> > >          /* default memory mappings */
> > >          for (i = 0; i < PCI_NUM_REGIONS; i++) {
> > >              int ofs;
> > > -            u32 val, size;
> > > -
> > > +            u32 val, mask, size;
> > >              if (i == PCI_ROM_SLOT)
> > >                  ofs = PCI_ROM_ADDRESS;
> > >              else
> > >                  ofs = PCI_BASE_ADDRESS_0 + i * 4;
> > > -            pci_config_writel(bdf, ofs, 0xffffffff);
> > > +
> > > +            val = pci_config_readl(bdf, ofs);
> > > +            if (i == PCI_ROM_SLOT)
> > > +		mask = PCI_ROM_ADDRESS_MASK;
> > > +            else if (val & PCI_BASE_ADDRESS_SPACE_IO)
> > > +                mask = PCI_BASE_ADDRESS_IO_MASK;
> > > +            else
> > > +                mask = PCI_BASE_ADDRESS_MEM_MASK;
> > > +            pci_config_writel(bdf, ofs, val | mask);
> > >              val = pci_config_readl(bdf, ofs);
> > >              if (val != 0) {
> > > -                size = (~(val & ~0xf)) + 1;
> > > +                size = (~(val & mask)) + 1;
> > >                  if (val & PCI_BASE_ADDRESS_SPACE_IO)
> > >                      paddr = &pci_bios_io_addr;
> > >                  else if (size >= 0x04000000)
> > > -- 
> > > 1.6.5.rc2
> > > 
> > 
> > --
> > 			Gleb.

--
			Gleb.
Michael S. Tsirkin - Oct. 12, 2009, 2:11 p.m.
On Mon, Oct 12, 2009 at 04:04:25PM +0200, Gleb Natapov wrote:
> On Mon, Oct 12, 2009 at 03:51:12PM +0200, Michael S. Tsirkin wrote:
> > On Mon, Oct 12, 2009 at 03:29:25PM +0200, Gleb Natapov wrote:
> > > On Mon, Oct 12, 2009 at 03:20:27PM +0200, Michael S. Tsirkin wrote:
> > > > Right. And there's another bug that I see and that is that size for I/O
> > > > BAR was calculated incorrectly: val & ~0xf is wrong for I/O as the size
> > > > could be 4 bytes.  So here's a patch that addresses all 3 issues:
> > > > Ack?
> > > > 
> > > Pleas do whatever spec recommends and use 0xffffffff to size the BAR.
> > 
> > What we do is equivalent: since next thing we override the BAR value,
> > there's no reason for the save/restore dance.  But if you like it this
> That what spec says if you are not trying to interpret it too hard.
> And simple is always better.

Implementation note is not a recommentation, just one way to do things.
We don't need to restore old BAR so no reason to do this, but
I won't lose sleep if we do this either. So go ahead.

> 
> > way, OK. Send a patch. Note this applies to I/O and memory only,
> > ROM has to be sized the way I coded it.
> > 
> > > Specs says this, linux does this, lets not be different.
> > 
> > BTW, note that what linux does for ROM is incorrect:
> > the implementation note you cite only applies to I/O and memory BARs.
> > I'll send a patch to lkml when I have the time.
> > 
> It does mask = type ? ~PCI_ROM_ADDRESS_ENABLE : ~0; so it definitely
> treats ROM specially, You may argue that it shouldn't set bits 1-10
> though.

Right.

> The patch that currently in KVM bios (the one I've sent to seabios
> initially) uses the same technique to size the BAR as Linux kernel. The only
> thing missing is restoring of old value.

That, too. Also let's use symbolic constants, and fix size math:
(val & ~0xf) is wrong for both ROM and I/O.

> > > > 
> > > > ---->
> > > > 
> > > > Subject: [PATCH] seabios: fix low bits in ROM and I/O sizing
> > > > 
> > > > This cleans up handling of low bits during BAR sizing,
> > > > to match PCI spec requirements, and to use symbolic
> > > > constants from pci_regs.h
> > > > 
> > > > Issues fixed:
> > > > 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 preserve any values there.
> > > > Bits 3:2 in I/O BAR might be writeable, so it
> > > > is wrong to mask them when calculating BAR size.
> > > > 
> > > > Spec references:
> > > > See 6.2.5.1 for I/O and memory, and 6.2.5.2 for ROM,
> > > > 6.1 for reserved bit handling;
> > > > pages 225, 228 and 214 in PCI spec revision 3.0.
> > > > 
> > > > See also Qemu pcbios commit 6ddb9f5c742b2b82b1755d7ec2a127f6e20e3806
> > > > 
> > > > Original-by: Gleb Natapov <gleb@redhat.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > 
> > > > diff --git a/src/pciinit.c b/src/pciinit.c
> > > > index 7d2ea00..f9ebd61 100644
> > > > --- a/src/pciinit.c
> > > > +++ b/src/pciinit.c
> > > > @@ -136,16 +136,23 @@ static void pci_bios_init_device(u16 bdf)
> > > >          /* default memory mappings */
> > > >          for (i = 0; i < PCI_NUM_REGIONS; i++) {
> > > >              int ofs;
> > > > -            u32 val, size;
> > > > -
> > > > +            u32 val, mask, size;
> > > >              if (i == PCI_ROM_SLOT)
> > > >                  ofs = PCI_ROM_ADDRESS;
> > > >              else
> > > >                  ofs = PCI_BASE_ADDRESS_0 + i * 4;
> > > > -            pci_config_writel(bdf, ofs, 0xffffffff);
> > > > +
> > > > +            val = pci_config_readl(bdf, ofs);
> > > > +            if (i == PCI_ROM_SLOT)
> > > > +		mask = PCI_ROM_ADDRESS_MASK;
> > > > +            else if (val & PCI_BASE_ADDRESS_SPACE_IO)
> > > > +                mask = PCI_BASE_ADDRESS_IO_MASK;
> > > > +            else
> > > > +                mask = PCI_BASE_ADDRESS_MEM_MASK;
> > > > +            pci_config_writel(bdf, ofs, val | mask);
> > > >              val = pci_config_readl(bdf, ofs);
> > > >              if (val != 0) {
> > > > -                size = (~(val & ~0xf)) + 1;
> > > > +                size = (~(val & mask)) + 1;
> > > >                  if (val & PCI_BASE_ADDRESS_SPACE_IO)
> > > >                      paddr = &pci_bios_io_addr;
> > > >                  else if (size >= 0x04000000)
> > > > -- 
> > > > 1.6.5.rc2
> > > > 
> > > 
> > > --
> > > 			Gleb.
> 
> --
> 			Gleb.
Gleb Natapov - Oct. 12, 2009, 2:17 p.m.
On Mon, Oct 12, 2009 at 04:11:29PM +0200, Michael S. Tsirkin wrote:
> On Mon, Oct 12, 2009 at 04:04:25PM +0200, Gleb Natapov wrote:
> > On Mon, Oct 12, 2009 at 03:51:12PM +0200, Michael S. Tsirkin wrote:
> > > On Mon, Oct 12, 2009 at 03:29:25PM +0200, Gleb Natapov wrote:
> > > > On Mon, Oct 12, 2009 at 03:20:27PM +0200, Michael S. Tsirkin wrote:
> > > > > Right. And there's another bug that I see and that is that size for I/O
> > > > > BAR was calculated incorrectly: val & ~0xf is wrong for I/O as the size
> > > > > could be 4 bytes.  So here's a patch that addresses all 3 issues:
> > > > > Ack?
> > > > > 
> > > > Pleas do whatever spec recommends and use 0xffffffff to size the BAR.
> > > 
> > > What we do is equivalent: since next thing we override the BAR value,
> > > there's no reason for the save/restore dance.  But if you like it this
> > That what spec says if you are not trying to interpret it too hard.
> > And simple is always better.
> 
> Implementation note is not a recommentation, just one way to do things.
Agree. But I prefer to use a way recommended by spec. It much less
likely to break.

> We don't need to restore old BAR so no reason to do this, but
We need to restore lower bits.

> I won't lose sleep if we do this either. So go ahead.
> 
> > 
> > > way, OK. Send a patch. Note this applies to I/O and memory only,
> > > ROM has to be sized the way I coded it.
> > > 
> > > > Specs says this, linux does this, lets not be different.
> > > 
> > > BTW, note that what linux does for ROM is incorrect:
> > > the implementation note you cite only applies to I/O and memory BARs.
> > > I'll send a patch to lkml when I have the time.
> > > 
> > It does mask = type ? ~PCI_ROM_ADDRESS_ENABLE : ~0; so it definitely
> > treats ROM specially, You may argue that it shouldn't set bits 1-10
> > though.
> 
> Right.
> 
> > The patch that currently in KVM bios (the one I've sent to seabios
> > initially) uses the same technique to size the BAR as Linux kernel. The only
> > thing missing is restoring of old value.
> 
> That, too. Also let's use symbolic constants, and fix size math:
> (val & ~0xf) is wrong for both ROM and I/O.
> 
0xf fix should be really in a separate patch.

> > > > > 
> > > > > ---->
> > > > > 
> > > > > Subject: [PATCH] seabios: fix low bits in ROM and I/O sizing
> > > > > 
> > > > > This cleans up handling of low bits during BAR sizing,
> > > > > to match PCI spec requirements, and to use symbolic
> > > > > constants from pci_regs.h
> > > > > 
> > > > > Issues fixed:
> > > > > 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 preserve any values there.
> > > > > Bits 3:2 in I/O BAR might be writeable, so it
> > > > > is wrong to mask them when calculating BAR size.
> > > > > 
> > > > > Spec references:
> > > > > See 6.2.5.1 for I/O and memory, and 6.2.5.2 for ROM,
> > > > > 6.1 for reserved bit handling;
> > > > > pages 225, 228 and 214 in PCI spec revision 3.0.
> > > > > 
> > > > > See also Qemu pcbios commit 6ddb9f5c742b2b82b1755d7ec2a127f6e20e3806
> > > > > 
> > > > > Original-by: Gleb Natapov <gleb@redhat.com>
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > 
> > > > > diff --git a/src/pciinit.c b/src/pciinit.c
> > > > > index 7d2ea00..f9ebd61 100644
> > > > > --- a/src/pciinit.c
> > > > > +++ b/src/pciinit.c
> > > > > @@ -136,16 +136,23 @@ static void pci_bios_init_device(u16 bdf)
> > > > >          /* default memory mappings */
> > > > >          for (i = 0; i < PCI_NUM_REGIONS; i++) {
> > > > >              int ofs;
> > > > > -            u32 val, size;
> > > > > -
> > > > > +            u32 val, mask, size;
> > > > >              if (i == PCI_ROM_SLOT)
> > > > >                  ofs = PCI_ROM_ADDRESS;
> > > > >              else
> > > > >                  ofs = PCI_BASE_ADDRESS_0 + i * 4;
> > > > > -            pci_config_writel(bdf, ofs, 0xffffffff);
> > > > > +
> > > > > +            val = pci_config_readl(bdf, ofs);
> > > > > +            if (i == PCI_ROM_SLOT)
> > > > > +		mask = PCI_ROM_ADDRESS_MASK;
> > > > > +            else if (val & PCI_BASE_ADDRESS_SPACE_IO)
> > > > > +                mask = PCI_BASE_ADDRESS_IO_MASK;
> > > > > +            else
> > > > > +                mask = PCI_BASE_ADDRESS_MEM_MASK;
> > > > > +            pci_config_writel(bdf, ofs, val | mask);
> > > > >              val = pci_config_readl(bdf, ofs);
> > > > >              if (val != 0) {
> > > > > -                size = (~(val & ~0xf)) + 1;
> > > > > +                size = (~(val & mask)) + 1;
> > > > >                  if (val & PCI_BASE_ADDRESS_SPACE_IO)
> > > > >                      paddr = &pci_bios_io_addr;
> > > > >                  else if (size >= 0x04000000)
> > > > > -- 
> > > > > 1.6.5.rc2
> > > > > 
> > > > 
> > > > --
> > > > 			Gleb.
> > 
> > --
> > 			Gleb.

--
			Gleb.
Michael S. Tsirkin - Oct. 12, 2009, 2:24 p.m.
On Mon, Oct 12, 2009 at 04:17:11PM +0200, Gleb Natapov wrote:
> On Mon, Oct 12, 2009 at 04:11:29PM +0200, Michael S. Tsirkin wrote:
> > On Mon, Oct 12, 2009 at 04:04:25PM +0200, Gleb Natapov wrote:
> > > On Mon, Oct 12, 2009 at 03:51:12PM +0200, Michael S. Tsirkin wrote:
> > > > On Mon, Oct 12, 2009 at 03:29:25PM +0200, Gleb Natapov wrote:
> > > > > On Mon, Oct 12, 2009 at 03:20:27PM +0200, Michael S. Tsirkin wrote:
> > > > > > Right. And there's another bug that I see and that is that size for I/O
> > > > > > BAR was calculated incorrectly: val & ~0xf is wrong for I/O as the size
> > > > > > could be 4 bytes.  So here's a patch that addresses all 3 issues:
> > > > > > Ack?
> > > > > > 
> > > > > Pleas do whatever spec recommends and use 0xffffffff to size the BAR.
> > > > 
> > > > What we do is equivalent: since next thing we override the BAR value,
> > > > there's no reason for the save/restore dance.  But if you like it this
> > > That what spec says if you are not trying to interpret it too hard.
> > > And simple is always better.
> > 
> > Implementation note is not a recommentation, just one way to do things.
> Agree. But I prefer to use a way recommended by spec. It much less
> likely to break.
> 
> > We don't need to restore old BAR so no reason to do this, but
> We need to restore lower bits.

The way I did it was always keeping original values for the lower bits.

> > I won't lose sleep if we do this either. So go ahead.
> > 
> > > 
> > > > way, OK. Send a patch. Note this applies to I/O and memory only,
> > > > ROM has to be sized the way I coded it.
> > > > 
> > > > > Specs says this, linux does this, lets not be different.
> > > > 
> > > > BTW, note that what linux does for ROM is incorrect:
> > > > the implementation note you cite only applies to I/O and memory BARs.
> > > > I'll send a patch to lkml when I have the time.
> > > > 
> > > It does mask = type ? ~PCI_ROM_ADDRESS_ENABLE : ~0; so it definitely
> > > treats ROM specially, You may argue that it shouldn't set bits 1-10
> > > though.
> > 
> > Right.
> > 
> > > The patch that currently in KVM bios (the one I've sent to seabios
> > > initially) uses the same technique to size the BAR as Linux kernel. The only
> > > thing missing is restoring of old value.
> > 
> > That, too. Also let's use symbolic constants, and fix size math:
> > (val & ~0xf) is wrong for both ROM and I/O.
> > 
> 0xf fix should be really in a separate patch.

It could be, but it won't be an independent patch: I'm using
the same mask value all over the code.

> > > > > > 
> > > > > > ---->
> > > > > > 
> > > > > > Subject: [PATCH] seabios: fix low bits in ROM and I/O sizing
> > > > > > 
> > > > > > This cleans up handling of low bits during BAR sizing,
> > > > > > to match PCI spec requirements, and to use symbolic
> > > > > > constants from pci_regs.h
> > > > > > 
> > > > > > Issues fixed:
> > > > > > 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 preserve any values there.
> > > > > > Bits 3:2 in I/O BAR might be writeable, so it
> > > > > > is wrong to mask them when calculating BAR size.
> > > > > > 
> > > > > > Spec references:
> > > > > > See 6.2.5.1 for I/O and memory, and 6.2.5.2 for ROM,
> > > > > > 6.1 for reserved bit handling;
> > > > > > pages 225, 228 and 214 in PCI spec revision 3.0.
> > > > > > 
> > > > > > See also Qemu pcbios commit 6ddb9f5c742b2b82b1755d7ec2a127f6e20e3806
> > > > > > 
> > > > > > Original-by: Gleb Natapov <gleb@redhat.com>
> > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > 
> > > > > > diff --git a/src/pciinit.c b/src/pciinit.c
> > > > > > index 7d2ea00..f9ebd61 100644
> > > > > > --- a/src/pciinit.c
> > > > > > +++ b/src/pciinit.c
> > > > > > @@ -136,16 +136,23 @@ static void pci_bios_init_device(u16 bdf)
> > > > > >          /* default memory mappings */
> > > > > >          for (i = 0; i < PCI_NUM_REGIONS; i++) {
> > > > > >              int ofs;
> > > > > > -            u32 val, size;
> > > > > > -
> > > > > > +            u32 val, mask, size;
> > > > > >              if (i == PCI_ROM_SLOT)
> > > > > >                  ofs = PCI_ROM_ADDRESS;
> > > > > >              else
> > > > > >                  ofs = PCI_BASE_ADDRESS_0 + i * 4;
> > > > > > -            pci_config_writel(bdf, ofs, 0xffffffff);
> > > > > > +
> > > > > > +            val = pci_config_readl(bdf, ofs);
> > > > > > +            if (i == PCI_ROM_SLOT)
> > > > > > +		mask = PCI_ROM_ADDRESS_MASK;
> > > > > > +            else if (val & PCI_BASE_ADDRESS_SPACE_IO)
> > > > > > +                mask = PCI_BASE_ADDRESS_IO_MASK;
> > > > > > +            else
> > > > > > +                mask = PCI_BASE_ADDRESS_MEM_MASK;
> > > > > > +            pci_config_writel(bdf, ofs, val | mask);
> > > > > >              val = pci_config_readl(bdf, ofs);
> > > > > >              if (val != 0) {
> > > > > > -                size = (~(val & ~0xf)) + 1;
> > > > > > +                size = (~(val & mask)) + 1;
> > > > > >                  if (val & PCI_BASE_ADDRESS_SPACE_IO)
> > > > > >                      paddr = &pci_bios_io_addr;
> > > > > >                  else if (size >= 0x04000000)
> > > > > > -- 
> > > > > > 1.6.5.rc2
> > > > > > 
> > > > > 
> > > > > --
> > > > > 			Gleb.
> > > 
> > > --
> > > 			Gleb.
> 
> --
> 			Gleb.

Patch

diff --git a/src/pciinit.c b/src/pciinit.c
index 7d2ea00..f9ebd61 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -136,16 +136,23 @@  static void pci_bios_init_device(u16 bdf)
         /* default memory mappings */
         for (i = 0; i < PCI_NUM_REGIONS; i++) {
             int ofs;
-            u32 val, size;
-
+            u32 val, mask, size;
             if (i == PCI_ROM_SLOT)
                 ofs = PCI_ROM_ADDRESS;
             else
                 ofs = PCI_BASE_ADDRESS_0 + i * 4;
-            pci_config_writel(bdf, ofs, 0xffffffff);
+
+            val = pci_config_readl(bdf, ofs);
+            if (i == PCI_ROM_SLOT)
+		mask = PCI_ROM_ADDRESS_MASK;
+            else if (val & PCI_BASE_ADDRESS_SPACE_IO)
+                mask = PCI_BASE_ADDRESS_IO_MASK;
+            else
+                mask = PCI_BASE_ADDRESS_MEM_MASK;
+            pci_config_writel(bdf, ofs, val | mask);
             val = pci_config_readl(bdf, ofs);
             if (val != 0) {
-                size = (~(val & ~0xf)) + 1;
+                size = (~(val & mask)) + 1;
                 if (val & PCI_BASE_ADDRESS_SPACE_IO)
                     paddr = &pci_bios_io_addr;
                 else if (size >= 0x04000000)