UIO memmap of PCi devices not working?

UIO memmap of PCi devices not working?

[Joakim Tjernlund]

Having problems to mmap PCI UIO devices and stumbeled over this page:
 http://billfarrow.blogspot.se/2010/09/userspace-access-to-pci-memory.html
it claims some adjustments are needed for UIO mmap over PCI to work.
These are #if 0 ATM and trying to enable them fails build.

Can this be fixed to at least build again ?
The reason for having #if 0 in the first place appears to be old X servers,
is that still true? Can the special casing be removed now?

 Jocke=

Re: UIO memmap of PCi devices not working?

[Benjamin Herrenschmidt]

On Wed, 2017-09-06 at 15:20 +0000, Joakim Tjernlund wrote:
> Having problems to mmap PCI UIO devices and stumbeled over this page:
>  http://billfarrow.blogspot.se/2010/09/userspace-access-to-pci-memory.html
> it claims some adjustments are needed for UIO mmap over PCI to work.
> These are #if 0 ATM and trying to enable them fails build.
> 
> Can this be fixed to at least build again ?
> The reason for having #if 0 in the first place appears to be old X servers,
> is that still true? Can the special casing be removed now?

This article seems out of date... I *think* things should work without
change by just mmap'ing the appropriate sysfs files. I'm not sure why
the author thought that had to be ifdef'ed out...

Let me know if you have problems.

As far as I know, the generic code will call pci_resource_to_user()
which on powerpc will return a physical address that already includes
the offset, which is why we don't later add it.

Now we could probably tear all that out and use the new generic code
instead as I *think* X has (very) long been fixed but I'd have to spend
some time triple checking and testing on old HW which I don't have the
bandwidth for right now. 

Cheers,
Ben.

Re: UIO memmap of PCi devices not working?

[Joakim Tjernlund]

On Thu, 2017-09-07 at 17:16 +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2017-09-06 at 15:20 +0000, Joakim Tjernlund wrote:
> > Having problems to mmap PCI UIO devices and stumbeled over this page:
> >  http://billfarrow.blogspot.se/2010/09/userspace-access-to-pci-memory.h=
tml
> > it claims some adjustments are needed for UIO mmap over PCI to work.
> > These are #if 0 ATM and trying to enable them fails build.
> >=20
> > Can this be fixed to at least build again ?
> > The reason for having #if 0 in the first place appears to be old X serv=
ers,
> > is that still true? Can the special casing be removed now?
>=20
> This article seems out of date... I *think* things should work without
> change by just mmap'ing the appropriate sysfs files. I'm not sure why
> the author thought that had to be ifdef'ed out...

Isn't that what the article is doing(mmaping sysfs files)?
And the article author is #ifdefing it back, not out.

>=20
> Let me know if you have problems.

Sure, we still are looking=20

>=20
> As far as I know, the generic code will call pci_resource_to_user()
> which on powerpc will return a physical address that already includes
> the offset, which is why we don't later add it.
>=20
> Now we could probably tear all that out and use the new generic code
> instead as I *think* X has (very) long been fixed but I'd have to spend
> some time triple checking and testing on old HW which I don't have the
> bandwidth for right now.=20

Could you fixup the code which is now #if 0 ? I wan't to test the
difference and I not sure how to fix the build problem after changing
those two #if 0 to #if 1
Even better if they could be a CONFIG option instead.

 Jocke=

Re: UIO memmap of PCi devices not working?

[Benjamin Herrenschmidt]

On Thu, 2017-09-07 at 07:22 +0000, Joakim Tjernlund wrote:
> On Thu, 2017-09-07 at 17:16 +1000, Benjamin Herrenschmidt wrote:
> > On Wed, 2017-09-06 at 15:20 +0000, Joakim Tjernlund wrote:
> > > Having problems to mmap PCI UIO devices and stumbeled over this page:
> > >  http://billfarrow.blogspot.se/2010/09/userspace-access-to-pci-memory.html
> > > it claims some adjustments are needed for UIO mmap over PCI to work.
> > > These are #if 0 ATM and trying to enable them fails build.
> > > 
> > > Can this be fixed to at least build again ?
> > > The reason for having #if 0 in the first place appears to be old X servers,
> > > is that still true? Can the special casing be removed now?
> > 
> > This article seems out of date... I *think* things should work without
> > change by just mmap'ing the appropriate sysfs files. I'm not sure why
> > the author thought that had to be ifdef'ed out...
> 
> Isn't that what the article is doing(mmaping sysfs files)?
> And the article author is #ifdefing it back, not out.

Yes sorry that's what I meant. It should work as-is.

> > 
> > Let me know if you have problems.
> 
> Sure, we still are looking 
> 
> > 
> > As far as I know, the generic code will call pci_resource_to_user()
> > which on powerpc will return a physical address that already includes
> > the offset, which is why we don't later add it.
> > 
> > Now we could probably tear all that out and use the new generic code
> > instead as I *think* X has (very) long been fixed but I'd have to spend
> > some time triple checking and testing on old HW which I don't have the
> > bandwidth for right now. 
> 
> Could you fixup the code which is now #if 0 ? I wan't to test the
> difference and I not sure how to fix the build problem after changing
> those two #if 0 to #if 1
> Even better if they could be a CONFIG option instead.

Hrm it's tricky, you shouldn't just turn that ifdef back on without
also changing pci_resource_to_user(). I'm not sure exactly what's going
on in your case, if you have a problem can you add printk to instrument
?

Cheers,
Ben.

Re: UIO memmap of PCi devices not working?

[Joakim Tjernlund]

On Thu, 2017-09-07 at 18:33 +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2017-09-07 at 07:22 +0000, Joakim Tjernlund wrote:
> > On Thu, 2017-09-07 at 17:16 +1000, Benjamin Herrenschmidt wrote:
> > > On Wed, 2017-09-06 at 15:20 +0000, Joakim Tjernlund wrote:
> > > > Having problems to mmap PCI UIO devices and stumbeled over this pag=
e:
> > > >  http://billfarrow.blogspot.se/2010/09/userspace-access-to-pci-memo=
ry.html
> > > > it claims some adjustments are needed for UIO mmap over PCI to work=
.
> > > > These are #if 0 ATM and trying to enable them fails build.
> > > >=20
> > > > Can this be fixed to at least build again ?
> > > > The reason for having #if 0 in the first place appears to be old X =
servers,
> > > > is that still true? Can the special casing be removed now?
> > >=20
> > > This article seems out of date... I *think* things should work withou=
t
> > > change by just mmap'ing the appropriate sysfs files. I'm not sure why
> > > the author thought that had to be ifdef'ed out...
> >=20
> > Isn't that what the article is doing(mmaping sysfs files)?
> > And the article author is #ifdefing it back, not out.
>=20
> Yes sorry that's what I meant. It should work as-is.
>=20
> > >=20
> > > Let me know if you have problems.
> >=20
> > Sure, we still are looking=20
> >=20
> > >=20
> > > As far as I know, the generic code will call pci_resource_to_user()
> > > which on powerpc will return a physical address that already includes
> > > the offset, which is why we don't later add it.
> > >=20
> > > Now we could probably tear all that out and use the new generic code
> > > instead as I *think* X has (very) long been fixed but I'd have to spe=
nd
> > > some time triple checking and testing on old HW which I don't have th=
e
> > > bandwidth for right now.=20
> >=20
> > Could you fixup the code which is now #if 0 ? I wan't to test the
> > difference and I not sure how to fix the build problem after changing
> > those two #if 0 to #if 1
> > Even better if they could be a CONFIG option instead.
>=20
> Hrm it's tricky, you shouldn't just turn that ifdef back on without
> also changing pci_resource_to_user().

There are two ifdef to change:
__pci_mmap_make_offset():
#if 0 /* See comment in pci_resource_to_user() for why this is disabled */
		*offset +=3D hose->pci_mem_offset;
#endif

and

pci_resource_to_user()
	/* We pass a fully fixed up address to userland for MMIO instead of
	 * a BAR value because X is lame and expects to be able to use that
	 * to pass to /dev/mem !
	 *
	 * That means that we'll have potentially 64 bits values where some
	 * userland apps only expect 32 (like X itself since it thinks only
	 * Sparc has 64 bits MMIO) but if we don't do that, we break it on
	 * 32 bits CHRPs :-(
	 *
	 * Hopefully, the sysfs insterface is immune to that gunk. Once X
	 * has been fixed (and the fix spread enough), we can re-enable the
	 * 2 lines below and pass down a BAR value to userland. In that case
	 * we'll also have to re-enable the matching code in
	 * __pci_mmap_make_offset().
	 *
	 * BenH.
	 */
#if 0
	else if (rsrc->flags & IORESOURCE_MEM)
		offset =3D hose->pci_mem_offset;
#endif

Problem is that pci_mem_offset is gone, the closed I can find is mem_offset
but that is an array,maybe just mem_offset[0] ?

> I'm not sure exactly what's going
> on in your case, if you have a problem can you add printk to instrument
> ?
Seems to be something else going on in out board. Anyhow, the mem_offset sh=
ould
be fixed to compile, nice to have it behind a CONFIG option. Then
one can start the process to remove the special casing easier.

 Jocke=

Re: UIO memmap of PCi devices not working?

[Joakim Tjernlund]

On Thu, 2017-09-07 at 10:59 +0200, Joakim Tjernlund wrote:
> On Thu, 2017-09-07 at 18:33 +1000, Benjamin Herrenschmidt wrote:
> > On Thu, 2017-09-07 at 07:22 +0000, Joakim Tjernlund wrote:
> > > On Thu, 2017-09-07 at 17:16 +1000, Benjamin Herrenschmidt wrote:
> > > > On Wed, 2017-09-06 at 15:20 +0000, Joakim Tjernlund wrote:
> > > > > Having problems to mmap PCI UIO devices and stumbeled over this p=
age:
> > > > >  http://billfarrow.blogspot.se/2010/09/userspace-access-to-pci-me=
mory.html
> > > > > it claims some adjustments are needed for UIO mmap over PCI to wo=
rk.
> > > > > These are #if 0 ATM and trying to enable them fails build.
> > > > >=20
> > > > > Can this be fixed to at least build again ?
> > > > > The reason for having #if 0 in the first place appears to be old =
X servers,
> > > > > is that still true? Can the special casing be removed now?
> > > >=20
> > > > This article seems out of date... I *think* things should work with=
out
> > > > change by just mmap'ing the appropriate sysfs files. I'm not sure w=
hy
> > > > the author thought that had to be ifdef'ed out...
> > >=20
> > > Isn't that what the article is doing(mmaping sysfs files)?
> > > And the article author is #ifdefing it back, not out.
> >=20
> > Yes sorry that's what I meant. It should work as-is.
> >=20
> > > >=20
> > > > Let me know if you have problems.
> > >=20
> > > Sure, we still are looking=20
> > >=20
> > > >=20
> > > > As far as I know, the generic code will call pci_resource_to_user()
> > > > which on powerpc will return a physical address that already includ=
es
> > > > the offset, which is why we don't later add it.
> > > >=20
> > > > Now we could probably tear all that out and use the new generic cod=
e
> > > > instead as I *think* X has (very) long been fixed but I'd have to s=
pend
> > > > some time triple checking and testing on old HW which I don't have =
the
> > > > bandwidth for right now.=20
> > >=20
> > > Could you fixup the code which is now #if 0 ? I wan't to test the
> > > difference and I not sure how to fix the build problem after changing
> > > those two #if 0 to #if 1
> > > Even better if they could be a CONFIG option instead.
> >=20
> > Hrm it's tricky, you shouldn't just turn that ifdef back on without
> > also changing pci_resource_to_user().
>=20
> There are two ifdef to change:
> __pci_mmap_make_offset():
> #if 0 /* See comment in pci_resource_to_user() for why this is disabled *=
/
> 		*offset +=3D hose->pci_mem_offset;
> #endif
>=20
> and
>=20
> pci_resource_to_user()
> 	/* We pass a fully fixed up address to userland for MMIO instead of
> 	 * a BAR value because X is lame and expects to be able to use that
> 	 * to pass to /dev/mem !
> 	 *
> 	 * That means that we'll have potentially 64 bits values where some
> 	 * userland apps only expect 32 (like X itself since it thinks only
> 	 * Sparc has 64 bits MMIO) but if we don't do that, we break it on
> 	 * 32 bits CHRPs :-(
> 	 *
> 	 * Hopefully, the sysfs insterface is immune to that gunk. Once X
> 	 * has been fixed (and the fix spread enough), we can re-enable the
> 	 * 2 lines below and pass down a BAR value to userland. In that case
> 	 * we'll also have to re-enable the matching code in
> 	 * __pci_mmap_make_offset().
> 	 *
> 	 * BenH.
> 	 */
> #if 0
> 	else if (rsrc->flags & IORESOURCE_MEM)
> 		offset =3D hose->pci_mem_offset;
> #endif

This seems to work, just a hack though:
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -314,8 +314,8 @@ static struct resource *__pci_mmap_make_offset(struct p=
ci_dev *dev,
=20
        /* If memory, add on the PCI bridge address offset */
        if (mmap_state =3D=3D pci_mmap_mem) {
-#if 0 /* See comment in pci_resource_to_user() for why this is disabled */
-               *offset +=3D hose->pci_mem_offset;
+#if 1  /* See comment in pci_resource_to_user() for why this is disabled *=
/
+               *offset +=3D hose->mem_offset[0];
 #endif
                res_bit =3D IORESOURCE_MEM;
        } else {
@@ -634,9 +634,9 @@ void pci_resource_to_user(const struct pci_dev *dev, in=
t bar,
         *
         * BenH.
         */
-#if 0
+#if 1
        else if (rsrc->flags & IORESOURCE_MEM)
-               offset =3D hose->pci_mem_offset;
+               offset =3D hose->mem_offset[0];
 #endif
=20
        *start =3D rsrc->start - offset;

>=20
> Problem is that pci_mem_offset is gone, the closed I can find is mem_offs=
et
> but that is an array,maybe just mem_offset[0] ?
>=20
> > I'm not sure exactly what's going
> > on in your case, if you have a problem can you add printk to instrument
> > ?
>=20
> Seems to be something else going on in out board. Anyhow, the mem_offset =
should
> be fixed to compile, nice to have it behind a CONFIG option. Then
> one can start the process to remove the special casing easier.

After sorting the bugs in our app, it works with and without above patch.

 Jocke=

Re: UIO memmap of PCi devices not working?

[Benjamin Herrenschmidt]

On Thu, 2017-09-07 at 10:19 +0000, Joakim Tjernlund wrote:
> > Problem is that pci_mem_offset is gone, the closed I can find is mem_offset
> > but that is an array,maybe just mem_offset[0] ?
> > 
> > > I'm not sure exactly what's going
> > > on in your case, if you have a problem can you add printk to instrument
> > > ?
> > 
> > Seems to be something else going on in out board. Anyhow, the mem_offset should
> > be fixed to compile, nice to have it behind a CONFIG option. Then
> > one can start the process to remove the special casing easier.
> 
> After sorting the bugs in our app, it works with and without above patch.

Ok. I don't see a pressing need to change what we are doing in the
kernel then.

Cheers,
Ben.

Re: UIO memmap of PCi devices not working?

[Benjamin Herrenschmidt]

On Thu, 2017-09-07 at 08:59 +0000, Joakim Tjernlund wrote:
> 
> > Hrm it's tricky, you shouldn't just turn that ifdef back on without
> > also changing pci_resource_to_user().
> 
> There are two ifdef to change:
> __pci_mmap_make_offset():
> #if 0 /* See comment in pci_resource_to_user() for why this is disabled */
> 		*offset += hose->pci_mem_offset;
> #endif
> 
> and
> 
> pci_resource_to_user()
> 	/* We pass a fully fixed up address to userland for MMIO instead of
> 	 * a BAR value because X is lame and expects to be able to use that
> 	 * to pass to /dev/mem !
> 	 *
> 	 * That means that we'll have potentially 64 bits values where some
> 	 * userland apps only expect 32 (like X itself since it thinks only
> 	 * Sparc has 64 bits MMIO) but if we don't do that, we break it on
> 	 * 32 bits CHRPs :-(
> 	 *
> 	 * Hopefully, the sysfs insterface is immune to that gunk. Once X
> 	 * has been fixed (and the fix spread enough), we can re-enable the
> 	 * 2 lines below and pass down a BAR value to userland. In that case
> 	 * we'll also have to re-enable the matching code in
> 	 * __pci_mmap_make_offset().
> 	 *
> 	 * BenH.
> 	 */
> #if 0
> 	else if (rsrc->flags & IORESOURCE_MEM)
> 		offset = hose->pci_mem_offset;
> #endif
> 
> Problem is that pci_mem_offset is gone, the closed I can find is mem_offset
> but that is an array,maybe just mem_offset[0] ?

No, you'd have to scan the array of resources to find which offset
applies.

> > I'm not sure exactly what's going
> > on in your case, if you have a problem can you add printk to instrument
> > ?
> 
> Seems to be something else going on in out board. Anyhow, the mem_offset should
> be fixed to compile, nice to have it behind a CONFIG option. Then
> one can start the process to remove the special casing easier.

Again, why do you need to remove it ? Can you find anything with the
existing code (with its #if'0) that is broken ?

Cheers,
Ben.