Re: PCI BAR's prefetchable bit and pci_mapreg_map()

[Michael <macallan%netbsd.org@localhost>]

Hello,

On Wed, 29 Mar 2017 15:19:39 +0900
Masanobu SAITOH <msaitoh%execsw.org@localhost> wrote:

> On 2017/03/24 8:13, Michael wrote:
> > Hello,
> >
> > On Wed, 22 Mar 2017 16:35:21 +0900
> > Masanobu SAITOH <msaitoh%execsw.org@localhost> wrote:
> >  
> >>   A) modify pci_mapreg_map(). Stop setting BUS_SPACE_MAP_PREFETCHABLE
> >>      by defalut when prefetchable bit is set. If a driver really know
> >>      the whole area of the BAR is prefetchable, set BUS_SPACE_MAP_PREFETCHABLE
> >>      in the 4th argument(busflags) of pci_mapreg_map(). pci_mapreg_map()
> >>      check for both the 4th argument and the prefetchable bit, it sets
> >>      BUS_SPACE_MAP_PREFETCHABLE only when both bits are set.  
> >
> > On 2nd thought, I think we should ignore the prefetchable bit in the
> > BAR here  
> 
>   in pci_mapreg_map()?
> 
> > and just go with whatever the driver wants,  
> 
>   You mean:
> 
>      For pci_mapreg_map():
> 
> 	Don't check prefetchable bit and don't set BUS_SPACE_MAP_PREFETCHABLE
> 	in pci_mapreg_map() like OpenBSD.
> 
>      For each driver side:
> 
> 	Don't use pci_mapreg_map() and use pci_mapreg_info() and
> 	bus_space_map() to use an area with BUS_SPACE_MAP_PREFETCHABLE
> 	(Plan D in my original mail)
> 
> Right? Let call this plan E.

I don't know what OpenBSD does - I think the driver should decide
wether to map something with write combining, prefetching  or similar
techniques applied, simply by passing BUS_SPACE_MAP_PREFETCHABLE,
wether the BAR in question is marked as PREFETCHABLE or not. Mostly
because the bit is not a reliable indicator - you brought examples
where it's set but shouldn't, I brought some where it should be set but
isn't.

> The plan E is OK, but it makes code duplication and the come won't
> be simple. To make code simple,

How? The driver knows the hardware's capabilities, no need for the
pci_mapreg_info() dance. Wether BUS_SPACE_MAP_PREFETCHABLE actually
does anything is MD anyway. In some cases, like MIPS, it's even CPU
model dependent.

> >  A) modify pci_mapreg_map(). Stop setting BUS_SPACE_MAP_PREFETCHABLE
> >     by defalut when prefetchable bit is set. If a driver really know
> >     the whole area of the BAR is prefetchable, set
> > BUS_SPACE_MAP_PREFETCHABLE in the 4th argument(busflags) of
> > pci_mapreg_map(). pci_mapreg_map() check for both the 4th argument
> > and the prefetchable bit, it sets BUS_SPACE_MAP_PREFETCHABLE only
> > when both bits are set.
> >
> >  B) Don't modify pci_mapreg_ma(). Add new API like FreeBSD's
> > pmap_change_attr() and use it when a driver want.
> >
> >  C) make new function pci_mapreg_map_wc()
> >
> >  D) Keep. If a driver know a BAR has prefetchable bit and not the
> >     whole area is prefetchable, do pci_mem_find() -> drop
> > BUS_SPACE_MAP_PREFETCHABLE and do bus_space_map().  
> 
>     E) Don't check prefetchable bit and don't set
> BUS_SPACE_MAP_PREFETCHABLE in any case in pci_mapreg_map() like
> OpenBSD. Don't use pci_mapreg_map() and use pci_mapreg_info() and
>        bus_space_map() if a driver want to use an area with
>        BUS_SPACE_MAP_PREFETCHABLE.
>   
>     F) Any other solution(s).
> 
> Plan A, B and C are useful than D and E to make code simple.
> 
> > since you ran into
> > devices which have the bit set and shouldn't, and I have seen
> > graphics chips which don't set it for their video memory
> > apertures.  
> 
> Even if we choose A or E, we have to check all driver
> which already use pci_mapreg_map() and modify some of
> them.
> 
>      For A:
>      - BAR has no prefetchable bit:
> 	not required to modify
>      - BAR has prefetchable bit and BUS_SPACE_MAP_PREFETCHABLE can't
> be used: not required to modify
>      - really prefetchable:
> 	set BUS_SPACE_MAP_PREFETCHABLE in the 4th argument
> 
>      For D:
>      - BAR has no prefetchable bit:
> 	not required to modify
>      - BAR has prefetchable bit and BUS_SPACE_MAP_PREFETCHABLE can't
> be used: not required to modify
>      - really prefetchable:
> 	Extract it with pci_mapreg_info() and bus_space_map()

For E:
- for drivers that don't set BUS_SPACE_MAP_PREFETCHABLE and don't want
  it - nothing to do here
- drivers that would benefit from BUS_SPACE_MAP_PREFETCHABLE need to be
  changed to explicitly set it, but they will continue to function if not
- drivers that set BUS_SPACE_MAP_PREFETCHABLE but really shouldn't - do
  they actually exist?

have fun
Michael