linux-kernel - Re: ftrace use of pci_resource_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160506131604.0067718f@eldfell>
Date:	Fri, 6 May 2016 13:16:04 +0300
From:	Pekka Paalanen <ppaalanen@...il.com>
To:	Bjorn Helgaas <helgaas@...nel.org>
Cc:	Steven Rostedt <rostedt@...dmis.org>,
	Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
	Yinghai Lu <yinghai@...nel.org>, karolherbst@...il.com,
	Ben Skeggs <bskeggs@...hat.com>, koriakin@...4.net
Subject: Re: ftrace use of pci_resource_to_user()

On Wed, 4 May 2016 14:17:13 -0500
Bjorn Helgaas <helgaas@...nel.org> wrote:

> 138295373ccf ("ftrace: mmiotrace update, #2") added this use of
> pci_resource_to_user():
> 
>   +static int mmio_print_pcidev(struct trace_seq *s, const struct pci_dev *dev)
>   +{
>   ...
>   +       /*
>   +        * XXX: is pci_resource_to_user() appropriate, since we are
>   +        * supposed to interpret the __ioremap() phys_addr argument based on
>   +        * these printed values?
>   +        */
>   +       for (i = 0; i < 7; i++) {
>   +               pci_resource_to_user(dev, i, &dev->resource[i], &start, &end);
>   +               ret += trace_seq_printf(s, " %llx",
>   +                       (unsigned long long)(start |
>   +                       (dev->resource[i].flags & PCI_REGION_FLAG_MASK)));
>   +       }
> 
> I think it was a mistake to use pci_resource_to_user() here because it
> adds unnecessary arch dependencies in whatever consumes this output.
> 
> On most arches, pci_resource_to_user() is a no-op and the result is
> normal resource addresses, i.e., CPU physical addresses that match
> things in /proc/iomem and /sys/devices/pci.../resource.
> 
> On microblaze, mips, powerpc, and sparc, the result of
> pci_resource_to_user() is something else, usually a PCI bus address (a
> raw BAR value).  These values are only useful for using mmap on
> files like /proc/bus/pci/....
> 
> I don't know what, if anything, consumes this output.  If things parse
> it, we shouldn't break them.  But those things likely would need
> special cases for microblaze, mips, powerpc, and sparc.
> 
> If it's only for human consumption, I think we should consider
> removing the use of pci_resource_to_user() and printing
> dev->resource[i].start instead.

Hi,

the code in question prints the "PCIDEV" lines in the mmiotrace output.
IIRC, it was initially meant to replicate the contents
of /proc/bus/pci/devices. I do not know it any tools rely on it, I
suppose they might, for mapping MAPs to device and BAR.

I am adding to CC some people that actually work with mmiotrace for
Nouveau. It is used for seeing what the NVIDIA proprieratry driver
does, so if there is no NVIDIA driver for the arch, they probably don't
care. I am not sure if other driver projects use it too, IIRC I heard
something about some wireless drivers in the past.


Thanks,
pq

Content of type "application/pgp-signature" skipped