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

[Gleb Natapov]

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 <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.

--
                        Gleb.