Messages in this thread | | | Date | Wed, 11 May 2016 14:04:57 -0500 | Subject | Re: ftrace use of pci_resource_to_user() | From | Bjorn Helgaas <> |
| |
On Fri, May 6, 2016 at 5:33 AM, karol herbst <karolherbst@gmail.com> wrote: > 2016-05-06 12:16 GMT+02:00 Pekka Paalanen <ppaalanen@gmail.com>: >> On Wed, 4 May 2016 14:17:13 -0500 >> Bjorn Helgaas <helgaas@kernel.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 > > Hi, > > thanks for adding us here. > > The code in question with which we parse the MMiotraces is demmio: > https://github.com/envytools/envytools/blob/master/rnn/demmio.c > > I never really looked into, though, so it could be partly wrong what I say here. > > In line 261 to 282 is the PCIDEV parsing section. > And this is used to get the relative address from the start of the BAR > regions, so that demmio can look addresses up in rnndb (database of > the mmio registers of Nvidia GPUs) > and prints out which value is written/read at which time and what that > value means for the hardware. > > And because that tools is kind of important for us to properly RE what > the nvidia does with the hardware, we really want to be carefull > changing anything here.
Sorry, for some reason I missed these responses until now.
As far as I can tell, Steve is right, and this code is only compiled for x86, where pci_resource_to_user() does nothing, so removing it should have no user-visible effect.
I'll post a patch to do that and cc: you.
Bjorn
| |