Re: ftrace use of pci_resource_to_user()

[Bjorn Helgaas]

On Fri, May 6, 2016 at 5:33 AM, karol herbst <karolherbst@xxxxxxxxx> wrote:
> 2016-05-06 12:16 GMT+02:00 Pekka Paalanen <ppaalanen@xxxxxxxxx>:
>> On Wed, 4 May 2016 14:17:13 -0500
>> Bjorn Helgaas <helgaas@xxxxxxxxxx> 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