Re: Documentation error in pci_mapreg_probe(9)
On 2022-01-10 3:53 a.m., Jonathan Gray wrote: > On Mon, Jan 10, 2022 at 09:17:25PM +1100, Jonathan Gray wrote: >> On Mon, Jan 10, 2022 at 02:24:05AM -0700, Ted Bullock wrote: >>> >>> On 2022-01-10 2:18 a.m., Jonathan Gray wrote: On Sat, Jan 08, 2022 at 01:43:32PM -0700, Ted Bullock wrote: > The manpage incorrectly describes the behaviour and usage of > pci_mapreg_probe(9). This function does not return 0 for success and !0 > for failure as described in the manual, see the diff below for a > possible re-wording and corrected description. I agree the return value is wrong but your description introduces an error in that type bits can contain PCI_MAPREG_MEM_TYPE_64BIT for example. Which is something that the pci_mapreg_type text also gets wrong. How about this to start with? >>> >>> It certainly corrects the mechanical issue with the returns, it doesn't help >>> though with understanding how to use the function correctly though. Under >>> what circumstances will the type bits get set as you indicate? I wasn't at >>> all clear about that from peering through the call tree. >> >> as the define would suggest, when it is a 64-bit mem bar > > expanding on this some more Here is what I have written so far. I added what I found to be necessary to actually use the API not having used it in OpenBSD before. More experienced devs might just *know* this but as someone who had to try and understand what it was doing using just the manual and the source code this is what was missing for me. Index: share/man/man9/pci_mapreg_map.9 === RCS file: /cvs/src/share/man/man9/pci_mapreg_map.9,v retrieving revision 1.1 diff -u -p -r1.1 pci_mapreg_map.9 --- share/man/man9/pci_mapreg_map.9 23 Feb 2019 04:54:25 - 1.1 +++ share/man/man9/pci_mapreg_map.9 11 Jan 2022 04:12:32 - @@ -1,6 +1,7 @@ .\"$OpenBSD: pci_mapreg_map.9,v 1.1 2019/02/23 04:54:25 dlg Exp $ .\" .\" Copyright (c) 2019 David Gwynne +.\" Copyright (c) 2022 Ted Bullock .\" All rights reserved. .\" .\" Permission to use, copy, modify, and distribute this software for any @@ -15,7 +16,7 @@ .\" ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF .\" OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. .\" -.Dd $Mdocdate: February 23 2019 $ +.Dd $Mdocdate: January 10 2022 $ .Dt PCI_MAPREG_MAP 9 .Os .Sh NAME @@ -50,19 +51,21 @@ .Fa "bus_size_t *sizep" .Fa "int *flagsp" .Fc -.Ft int -.Fo pci_mapreg_probe +.Ft pcireg_t +.Fo pci_mapreg_type .Fa "pci_chipset_tag_t pc" .Fa "pcitag_t tag" .Fa "int reg" -.Fa "pcireg_t *typep" .Fc -.Ft pcireg_t -.Fo pci_mapreg_type +.Ft int +.Fo pci_mapreg_probe .Fa "pci_chipset_tag_t pc" .Fa "pcitag_t tag" .Fa "int reg" +.Fa "pcireg_t *typep" .Fc +.Fd #define PCI_MAPREG_TYPE(type) +.Fd #define PCI_MAPREG_MEM_TYPE(type) .\" .Ft int .\" .Fo pci_mem_find .\" .Fa "pci_chipset_tag_t pc" @@ -85,7 +88,43 @@ These functions provide wrappers and helpers around .Xr bus_space 9 mappings for device registers described by the Base Address Registers -(BARs) in a PCI devices configuration space. +(BARs) in a PCI devices configuration space. There are 6 BARs located in the +pci address space at 0x10, 0x14, 0x18, 0x1C, 0x20 and 0x24. The addresses +stored in the PCI BARS are used to map IO and Memory Mapped device registers. +.Pp +BARS have a complex bitmap described a follows: +.TS +center,allbox; +cb s s +cb cb cb +cb s s +c r l +c r l +cb s s +c r l +^ r l +^ r l +c r l +^ r l +c r l +cb s s +c r l. +PCI BAR Bitmap +Bits Description Values +All PCI BARs +0 Type0 Memory +\^ \^ 1 I/O +Memory BARs +1-2Location00 Any 32-bit +\^ \^ 10 < 1MB +\^ \^ 11 Any 64-bit +3 Prefetchable0 No +\^ \^ 1 Yes +4-31 Base Address16-byte aligned +I/O BARs +1 Reserved\_ +2-31 Base Address4-byte aligned +.TE .Pp .Nm pci_mapreg_map wraps a call to @@ -124,29 +163,70 @@ and .Fa flagsp pointers. .Pp +.Nm pci_mapreg_type +returns the type of register access for the registers at the BAR +referenced by +.Fa reg . +.Pp .Nm pci_mapreg_probe attempts to determine if the BAR referenced by .Fa reg -describes a valid register mapping. +describes a valid register mapping. If it does, the register type will be +updated via the pointer +.Fa typep +matching those described for the return value of +.Nm pci_mapreg_type . +If the mapping is invalid the register type should not be used. +.Fa typep +can be +.Dv NULL +if determining the register type is not desired. .Pp +.Nm PCI_MAPREG_TYPE(type) +is a helper macro useful for logical operations on types returned by .Nm pci_mapreg_type -returns the type of register access for the registers at the BAR -referenced by -.Fa reg . -.Sh RETURN VALUES -.Nm pci_mapreg_map , -.Nm pci_mapreg_info , and -.Nm pci_mapreg_probe -return 0 on success, or an +.Nm
Re: Documentation error in pci_mapreg_probe(9)
On 2022-01-10 3:53 a.m., Jonathan Gray wrote: > On Mon, Jan 10, 2022 at 09:17:25PM +1100, Jonathan Gray wrote: >> On Mon, Jan 10, 2022 at 02:24:05AM -0700, Ted Bullock wrote: >>> >>> On 2022-01-10 2:18 a.m., Jonathan Gray wrote: On Sat, Jan 08, 2022 at 01:43:32PM -0700, Ted Bullock wrote: > The manpage incorrectly describes the behaviour and usage of > pci_mapreg_probe(9). This function does not return 0 for success and !0 > for failure as described in the manual, see the diff below for a > possible re-wording and corrected description. I agree the return value is wrong but your description introduces an error in that type bits can contain PCI_MAPREG_MEM_TYPE_64BIT for example. Which is something that the pci_mapreg_type text also gets wrong. How about this to start with? >>> >>> It certainly corrects the mechanical issue with the returns, it doesn't help >>> though with understanding how to use the function correctly though. Under >>> what circumstances will the type bits get set as you indicate? I wasn't at >>> all clear about that from peering through the call tree. >> >> as the define would suggest, when it is a 64-bit mem bar Ok, then still missing from the document is the semantic distinctions between the return types and the correct usage for those types with pci_mapreg_info and pci_mapreg_map. There is also the undocumented PCI_MAPREG_TYPE and PCI_MAPREG_MEM_TYPE macros too: This usage should be incorrect? if (type == PCI_MAPREG_TYPE_MEM) I think correct form should be then if (PCI_MAPREG_TYPE(type) == PCI_MAPREG_TYPE_MEM) switch (PCI_MAPREG_MEM_TYPE(type)) PCI_MAPREG_MEM_TYPE_32BIT_1M: PCI_MAPREG_MEM_TYPE_32BIT: pci_mapreg_map for 32 bit bar PCI_MAPREG_MEM_TYPE_64BIT: pci_mapreg_map for 64 bit bar This pci stack doesn't also seem to handle pci devices with a 16 bit bar, but I also don't know if such devices actually exist so maybe this doesn't matter > expanding on this some more > > Index: pci_mapreg_map.9 > === > RCS file: /cvs/src/share/man/man9/pci_mapreg_map.9,v > retrieving revision 1.1 > diff -u -p -r1.1 pci_mapreg_map.9 > --- pci_mapreg_map.9 23 Feb 2019 04:54:25 - 1.1 > +++ pci_mapreg_map.9 10 Jan 2022 10:51:36 - > @@ -128,6 +128,11 @@ pointers. > attempts to determine if the BAR referenced by > .Fa reg > describes a valid register mapping. > +If it does > +.Fa typep > +will point to a value with flags matching those described for the return > value > +of > +.Nm pci_mapreg_type . > .Pp > .Nm pci_mapreg_type > returns the type of register access for the registers at the BAR > @@ -135,18 +140,22 @@ referenced by > .Fa reg . > .Sh RETURN VALUES > .Nm pci_mapreg_map , > -.Nm pci_mapreg_info , > and > -.Nm pci_mapreg_probe > +.Nm pci_mapreg_info > return 0 on success, or an > .Xr errno 2 > style value on failure. > .Pp > +.Nm pci_mapreg_probe > +returns 1 if if the BAR describes a valid mapping, 0 if not. > +.Pp > .Nm pci_mapreg_type > -returns either > -.Dv PCI_MAPREG_TYPE_IO > -or > -.Dv PCI_MAPREG_TYPE_MEM . > +returns a value with flags for type information. > +.Dv PCI_MAPREG_TYPE_IO , > +.Dv PCI_MAPREG_TYPE_MEM , > +.Dv PCI_MAPREG_MEM_TYPE_32BIT , > +.Dv PCI_MAPREG_MEM_TYPE_32BIT_1M and > +.Dv PCI_MAPREG_MEM_TYPE_64BIT . > .Sh SEE ALSO > .Xr pci 4 , > .Xr bus_space 9 , -- Ted Bullock
Re: Documentation error in pci_mapreg_probe(9)
On Mon, Jan 10, 2022 at 09:17:25PM +1100, Jonathan Gray wrote: > On Mon, Jan 10, 2022 at 02:24:05AM -0700, Ted Bullock wrote: > > > > On 2022-01-10 2:18 a.m., Jonathan Gray wrote: > > > On Sat, Jan 08, 2022 at 01:43:32PM -0700, Ted Bullock wrote: > > > > The manpage incorrectly describes the behaviour and usage of > > > > pci_mapreg_probe(9). This function does not return 0 for success and !0 > > > > for failure as described in the manual, see the diff below for a > > > > possible re-wording and corrected description. > > > > > > I agree the return value is wrong but your description introduces an > > > error in that type bits can contain PCI_MAPREG_MEM_TYPE_64BIT for example. > > > Which is something that the pci_mapreg_type text also gets wrong. > > > > > > How about this to start with? > > > > It certainly corrects the mechanical issue with the returns, it doesn't help > > though with understanding how to use the function correctly though. Under > > what circumstances will the type bits get set as you indicate? I wasn't at > > all clear about that from peering through the call tree. > > as the define would suggest, when it is a 64-bit mem bar expanding on this some more Index: pci_mapreg_map.9 === RCS file: /cvs/src/share/man/man9/pci_mapreg_map.9,v retrieving revision 1.1 diff -u -p -r1.1 pci_mapreg_map.9 --- pci_mapreg_map.923 Feb 2019 04:54:25 - 1.1 +++ pci_mapreg_map.910 Jan 2022 10:51:36 - @@ -128,6 +128,11 @@ pointers. attempts to determine if the BAR referenced by .Fa reg describes a valid register mapping. +If it does +.Fa typep +will point to a value with flags matching those described for the return value +of +.Nm pci_mapreg_type . .Pp .Nm pci_mapreg_type returns the type of register access for the registers at the BAR @@ -135,18 +140,22 @@ referenced by .Fa reg . .Sh RETURN VALUES .Nm pci_mapreg_map , -.Nm pci_mapreg_info , and -.Nm pci_mapreg_probe +.Nm pci_mapreg_info return 0 on success, or an .Xr errno 2 style value on failure. .Pp +.Nm pci_mapreg_probe +returns 1 if if the BAR describes a valid mapping, 0 if not. +.Pp .Nm pci_mapreg_type -returns either -.Dv PCI_MAPREG_TYPE_IO -or -.Dv PCI_MAPREG_TYPE_MEM . +returns a value with flags for type information. +.Dv PCI_MAPREG_TYPE_IO , +.Dv PCI_MAPREG_TYPE_MEM , +.Dv PCI_MAPREG_MEM_TYPE_32BIT , +.Dv PCI_MAPREG_MEM_TYPE_32BIT_1M and +.Dv PCI_MAPREG_MEM_TYPE_64BIT . .Sh SEE ALSO .Xr pci 4 , .Xr bus_space 9 ,
Re: Documentation error in pci_mapreg_probe(9)
On Mon, Jan 10, 2022 at 02:24:05AM -0700, Ted Bullock wrote:
>
> On 2022-01-10 2:18 a.m., Jonathan Gray wrote:
> > On Sat, Jan 08, 2022 at 01:43:32PM -0700, Ted Bullock wrote:
> > > The manpage incorrectly describes the behaviour and usage of
> > > pci_mapreg_probe(9). This function does not return 0 for success and !0
> > > for failure as described in the manual, see the diff below for a
> > > possible re-wording and corrected description.
> >
> > I agree the return value is wrong but your description introduces an
> > error in that type bits can contain PCI_MAPREG_MEM_TYPE_64BIT for example.
> > Which is something that the pci_mapreg_type text also gets wrong.
> >
> > How about this to start with?
>
> It certainly corrects the mechanical issue with the returns, it doesn't help
> though with understanding how to use the function correctly though. Under
> what circumstances will the type bits get set as you indicate? I wasn't at
> all clear about that from peering through the call tree.
as the define would suggest, when it is a 64-bit mem bar
for example
0x0010: BAR mem 64bit addr: 0xf122/0x0001
xhci0 at pci0 dev 20 function 0 "Intel 9 Series xHCI" rev 0x03: bar 0x10 type
0x4 : msi, xHCI 1.0
Index: xhci_pci.c
===
RCS file: /cvs/src/sys/dev/pci/xhci_pci.c,v
retrieving revision 1.10
diff -u -p -r1.10 xhci_pci.c
--- xhci_pci.c 18 Nov 2018 08:46:57 - 1.10
+++ xhci_pci.c 10 Jan 2022 09:54:09 -
@@ -134,6 +134,7 @@ xhci_pci_attach(struct device *parent, s
pci_intr_handle_t ih;
pcireg_t reg;
int error;
+ pcireg_t type;
reg = pci_mapreg_type(pa->pa_pc, pa->pa_tag, PCI_CBMEM);
if (pci_mapreg_map(pa, PCI_CBMEM, reg, 0, >sc.iot, >sc.ioh,
@@ -141,6 +142,9 @@ xhci_pci_attach(struct device *parent, s
printf(": can't map mem space\n");
return;
}
+
+ if (pci_mapreg_probe(pa->pa_pc, pa->pa_tag, 0x10, ))
+ printf(": bar 0x10 type 0x%x ", type);
psc->sc_pc = pa->pa_pc;
psc->sc_tag = pa->pa_tag;
from pcireg.h:
#define PCI_MAPREG_TYPE(mr) \
((mr) & PCI_MAPREG_TYPE_MASK)
#define PCI_MAPREG_TYPE_MASK0x0001
#define PCI_MAPREG_TYPE_MEM 0x
#define PCI_MAPREG_TYPE_IO 0x0001
#define PCI_MAPREG_MEM_TYPE(mr) \
((mr) & PCI_MAPREG_MEM_TYPE_MASK)
#define PCI_MAPREG_MEM_TYPE_MASK0x0006
#define PCI_MAPREG_MEM_TYPE_32BIT 0x
#define PCI_MAPREG_MEM_TYPE_32BIT_1M0x0002
#define PCI_MAPREG_MEM_TYPE_64BIT 0x0004
#define _PCI_MAPREG_TYPEBITS(reg) \
(PCI_MAPREG_TYPE(reg) == PCI_MAPREG_TYPE_IO ? \
reg & PCI_MAPREG_TYPE_MASK : \
reg & (PCI_MAPREG_TYPE_MASK|PCI_MAPREG_MEM_TYPE_MASK))
>
> Anyway it's certainly a weakness of the document. This at least stops the
> error from the return values.
>
> >
> > Index: pci_mapreg_map.9
> > ===
> > RCS file: /cvs/src/share/man/man9/pci_mapreg_map.9,v
> > retrieving revision 1.1
> > diff -u -p -r1.1 pci_mapreg_map.9
> > --- pci_mapreg_map.923 Feb 2019 04:54:25 - 1.1
> > +++ pci_mapreg_map.910 Jan 2022 09:13:30 -
> > @@ -135,12 +135,14 @@ referenced by
> > .Fa reg .
> > .Sh RETURN VALUES
> > .Nm pci_mapreg_map ,
> > -.Nm pci_mapreg_info ,
> > and
> > -.Nm pci_mapreg_probe
> > +.Nm pci_mapreg_info
> > return 0 on success, or an
> > .Xr errno 2
> > style value on failure.
> > +.Pp
> > +.Nm pci_mapreg_probe
> > +returns 1 if if the BAR is implemented, 0 if not.
> > .Pp
> > .Nm pci_mapreg_type
> > returns either
>
> --
> Ted Bullock
>
>
Re: Documentation error in pci_mapreg_probe(9)
On 2022-01-10 2:18 a.m., Jonathan Gray wrote: On Sat, Jan 08, 2022 at 01:43:32PM -0700, Ted Bullock wrote: The manpage incorrectly describes the behaviour and usage of pci_mapreg_probe(9). This function does not return 0 for success and !0 for failure as described in the manual, see the diff below for a possible re-wording and corrected description. I agree the return value is wrong but your description introduces an error in that type bits can contain PCI_MAPREG_MEM_TYPE_64BIT for example. Which is something that the pci_mapreg_type text also gets wrong. How about this to start with? It certainly corrects the mechanical issue with the returns, it doesn't help though with understanding how to use the function correctly though. Under what circumstances will the type bits get set as you indicate? I wasn't at all clear about that from peering through the call tree. Anyway it's certainly a weakness of the document. This at least stops the error from the return values. Index: pci_mapreg_map.9 === RCS file: /cvs/src/share/man/man9/pci_mapreg_map.9,v retrieving revision 1.1 diff -u -p -r1.1 pci_mapreg_map.9 --- pci_mapreg_map.923 Feb 2019 04:54:25 - 1.1 +++ pci_mapreg_map.910 Jan 2022 09:13:30 - @@ -135,12 +135,14 @@ referenced by .Fa reg . .Sh RETURN VALUES .Nm pci_mapreg_map , -.Nm pci_mapreg_info , and -.Nm pci_mapreg_probe +.Nm pci_mapreg_info return 0 on success, or an .Xr errno 2 style value on failure. +.Pp +.Nm pci_mapreg_probe +returns 1 if if the BAR is implemented, 0 if not. .Pp .Nm pci_mapreg_type returns either -- Ted Bullock
Re: Documentation error in pci_mapreg_probe(9)
On Sat, Jan 08, 2022 at 01:43:32PM -0700, Ted Bullock wrote: > The manpage incorrectly describes the behaviour and usage of > pci_mapreg_probe(9). This function does not return 0 for success and !0 > for failure as described in the manual, see the diff below for a > possible re-wording and corrected description. I agree the return value is wrong but your description introduces an error in that type bits can contain PCI_MAPREG_MEM_TYPE_64BIT for example. Which is something that the pci_mapreg_type text also gets wrong. How about this to start with? Index: pci_mapreg_map.9 === RCS file: /cvs/src/share/man/man9/pci_mapreg_map.9,v retrieving revision 1.1 diff -u -p -r1.1 pci_mapreg_map.9 --- pci_mapreg_map.923 Feb 2019 04:54:25 - 1.1 +++ pci_mapreg_map.910 Jan 2022 09:13:30 - @@ -135,12 +135,14 @@ referenced by .Fa reg . .Sh RETURN VALUES .Nm pci_mapreg_map , -.Nm pci_mapreg_info , and -.Nm pci_mapreg_probe +.Nm pci_mapreg_info return 0 on success, or an .Xr errno 2 style value on failure. +.Pp +.Nm pci_mapreg_probe +returns 1 if if the BAR is implemented, 0 if not. .Pp .Nm pci_mapreg_type returns either
