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

["Michael S. Tsirkin" <mst@redhat.com>]

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