Skip to contentSkip to conversation

ftrace use of pci_resource_to_user()

DISCUSSION Bjorn Helgaas 5 participants 5 messages May 4, 2016 – May 11, 2016

lore
Share & tools
lore

Bjorn Helgaas

author
#0 lore raw

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.

Bjorn

#1 lore raw
Quoted 30 lines · On Wed, 4 May 2016 14:17:13 -0500 Bjorn Helgaas <[email protected]> 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/....

Quoted 7 lines

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.

Currently this code requires the arch to define HAVE_MMIOTRACE_SUPPORT, and so far as I can tell, only x86 does this.

-- Steve

#2 lore raw
In reply to Bjorn Helgaas
Quoted 30 lines · On Wed, 4 May 2016 14:17:13 -0500 Bjorn Helgaas <[email protected]> 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/....

Quoted 7 lines

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

#3 lore raw

2016-05-06 12:16 GMT+02:00 Pekka Paalanen <[email protected]>:

Quoted 55 lines
Quoted 30 lines · On Wed, 4 May 2016 14:17:13 -0500 Bjorn Helgaas <[email protected]> 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/....

Quoted 7 lines

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.

Quoted 2 lines

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.

Thanks,

Karol

#4 lore raw
Quoted 75 lines · karol herbst

2016-05-06 12:16 GMT+02:00 Pekka Paalanen <[email protected]>:

Quoted 55 lines
Quoted 30 lines · On Wed, 4 May 2016 14:17:13 -0500 Bjorn Helgaas <[email protected]> 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/....

Quoted 7 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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