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

[Michael S. Tsirkin]

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 <address@hidden>
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > 
> > 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.