Date: Mon, 11 Nov 1996 00:10:20 +0100 From: se@zpr.uni-koeln.de (Stefan Esser) To: durian@plutotech.com (Mike Durian) Cc: freebsd-hackers@freebsd.org Subject: Re: small bugs in pci code Message-ID: <199611102311.AAA00997@x14.mi.uni-koeln.de> In-Reply-To: <199611071835.LAA25115@pluto.plutotech.com>; from Mike Durian on Nov 7, 1996 11:35:09 -0700 References: <199611071835.LAA25115@pluto.plutotech.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Mike Durian writes:
> I've discovered three bugs in the pci code (at least I believe they
> are bugs). These are in the 2.1.5 release. Since we've made some
> local changes to these files, I don't have proper diffs, but the
> changes are very small and isolated.
Thanks a lot for sending such a good problem report!
Please test the patches you'll find appended to this message ...
> 1) In pcireg.h, the definitions of PCI_PPB_IOBASE_EXTRACT and
> PCI_PPB_IOLIMIT_EXTRACT should be:
>
> #define PCI_PPB_IOBASE_EXTRACT(x) (((x) << 8) & 0xF000)
> #define PCI_PPB_IOLIMIT_EXTRACT(x) (((x) << 0) & 0xF000)
Modified the sources in a slightly different way ...
> 2) Related to #1, the mask that is OR'd with the PCI_PPB_IOLIMIT_EXTRACT
> value should be 0xfff. It should not be calculated from the
> amount of memory requested. I don't have a line number for this,
> but it is in pci.c near the PCI_PPB_IOLIMIT_EXTRACT string, which
> only occurs once. The PCI standard says, "the upper 4 bits of I/O
> Limit register are writeable and correspond to address bits AD[15::12].
> For purpose of address decoding the bridge assumes that the lower 12
> address bits, AD[11::0], of the I/O limit address (not implemented
> in the I/O Limit register) are FFFh."
Yes.
Hmmm, but the same should apply to the memory regions!
Well, looking at it more closely, the whole code appears to be bogus.
Please test my suggested fix (you seem to have the required hardware,
or you wouldn't have noticed there was a problem, I suppose ;-)
> 3) In the same pci_bus_config() function, there is a section of for
> loop code that begins with the comment, "Read the current mapping,
> and update the pcicb fields." In this for loop, there is a switch
> statement that handles the two types of mapping: I/O and memory.
> The line:
> switch (data & 7) {
> should read:
> switch (map & 7) {
> The lower three bits that determine the mapping type are found in
> the address value, not the size value.
Well, I can only agree to half of that sentence: The mapping type is
found in the address, but also in the size. The low bits of the map
registers are hardwired. For that reason, (data & 7) is guaranteed
to be identical to (map & 7) ...
Regards, STefan
Index: pcireg.h
===================================================================
RCS file: /usr/cvs/src/sys/pci/pcireg.h,v
retrieving revision 1.5.4.2
diff -C2 -r1.5.4.2 pcireg.h
*** pcireg.h 1996/06/26 04:36:50 1.5.4.2
--- pcireg.h 1996/11/10 22:59:21
***************
*** 174,182 ****
#define PCI_SUBORDINATE_BUS_INSERT(x, y) (((x) & ~PCI_SUBORDINATE_BUS_MASK) | ((y) << 16))
! #define PCI_PPB_IOBASE_EXTRACT(x) (((x) << 8) & 0xFF00)
! #define PCI_PPB_IOLIMIT_EXTRACT(x) (((x) << 0) & 0xFF00)
! #define PCI_PPB_MEMBASE_EXTRACT(x) (((x) << 16) & 0xFFFF0000)
! #define PCI_PPB_MEMLIMIT_EXTRACT(x) (((x) << 0) & 0xFFFF0000)
/*
--- 174,182 ----
#define PCI_SUBORDINATE_BUS_INSERT(x, y) (((x) & ~PCI_SUBORDINATE_BUS_MASK) | ((y) << 16))
! #define PCI_PPB_IOBASE_EXTRACT(x) (((x) << 8) & 0xF000)
! #define PCI_PPB_IOLIMIT_EXTRACT(x) (((x) << 0) & 0xF000 | 0x0FFF)
! #define PCI_PPB_MEMBASE_EXTRACT(x) (((x) << 16) & 0xFFF00000)
! #define PCI_PPB_MEMLIMIT_EXTRACT(x) (((x) << 0) & 0xFFF00000 | 0x000FFFFF)
/*
Index: pci.c
===================================================================
RCS file: /usr/cvs/src/sys/pci/pci.c,v
retrieving revision 1.23.4.6
diff -C2 -r1.23.4.6 pci.c
*** pci.c 1996/06/26 04:36:49 1.23.4.6
--- pci.c 1996/11/10 23:03:34
***************
*** 774,794 ****
** Read out the mapped io region.
*/
! u_int reg, data, mask;
reg = pci_conf_read (tag,
PCI_PCI_BRIDGE_IO_REG);
- pci_conf_write(tag,
- PCI_PCI_BRIDGE_IO_REG, 0xFFFF);
- data = pci_conf_read (tag,
- PCI_PCI_BRIDGE_IO_REG);
- pci_conf_write(tag,
- PCI_PCI_BRIDGE_IO_REG, reg & 0xffff);
-
- mask = (0xFF00 ^ (data & 0xFF00)) | 0xFF;
-
this->pcicb_iobase =
PCI_PPB_IOBASE_EXTRACT (reg);
this->pcicb_iolimit =
! PCI_PPB_IOLIMIT_EXTRACT(reg) | mask;
/*
--- 774,785 ----
** Read out the mapped io region.
*/
! unsigned reg;
reg = pci_conf_read (tag,
PCI_PCI_BRIDGE_IO_REG);
this->pcicb_iobase =
PCI_PPB_IOBASE_EXTRACT (reg);
this->pcicb_iolimit =
! PCI_PPB_IOLIMIT_EXTRACT(reg);
/*
***************
*** 805,809 ****
** Read out the mapped memory regions.
*/
! u_int reg, data, mask;
/*
--- 796,800 ----
** Read out the mapped memory regions.
*/
! unsigned reg;
/*
***************
*** 812,827 ****
reg = pci_conf_read (tag,
PCI_PCI_BRIDGE_MEM_REG);
- pci_conf_write(tag,
- PCI_PCI_BRIDGE_MEM_REG, 0xFFFFFFFF);
- data = pci_conf_read (tag,
- PCI_PCI_BRIDGE_MEM_REG);
- pci_conf_write(tag,
- PCI_PCI_BRIDGE_MEM_REG, reg);
-
- mask = 0xFFFFFFFF ^ (data & 0xFFFF0000);
this->pcicb_membase =
PCI_PPB_MEMBASE_EXTRACT (reg);
this->pcicb_memlimit =
! PCI_PPB_MEMLIMIT_EXTRACT(reg) | mask;
/*
--- 803,810 ----
reg = pci_conf_read (tag,
PCI_PCI_BRIDGE_MEM_REG);
this->pcicb_membase =
PCI_PPB_MEMBASE_EXTRACT (reg);
this->pcicb_memlimit =
! PCI_PPB_MEMLIMIT_EXTRACT(reg);
/*
***************
*** 837,852 ****
reg = pci_conf_read (tag,
PCI_PCI_BRIDGE_PMEM_REG);
- pci_conf_write(tag,
- PCI_PCI_BRIDGE_PMEM_REG, 0xFFFFFFFF);
- data = pci_conf_read (tag,
- PCI_PCI_BRIDGE_PMEM_REG);
- pci_conf_write(tag,
- PCI_PCI_BRIDGE_PMEM_REG, reg);
-
- mask = 0xFFFFFFFF ^ (data & 0xFFFF0000);
this->pcicb_p_membase=
PCI_PPB_MEMBASE_EXTRACT (reg);
this->pcicb_p_memlimit=
! PCI_PPB_MEMLIMIT_EXTRACT(reg) | mask;
/*
--- 820,827 ----
reg = pci_conf_read (tag,
PCI_PCI_BRIDGE_PMEM_REG);
this->pcicb_p_membase=
PCI_PPB_MEMBASE_EXTRACT (reg);
this->pcicb_p_memlimit=
! PCI_PPB_MEMLIMIT_EXTRACT(reg);
/*
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199611102311.AAA00997>