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: <e59cb465-3f0d-4d11-8585-f0716cb09061@amd.com>
Date:   Mon, 23 Jan 2023 18:30:39 -0800
From:   Lizhi Hou <lizhi.hou@....com>
To:     Rob Herring <robh@...nel.org>,
        Vincent Whitchurch <vincent.whitchurch@...s.com>
CC:     Lee Jones <lee@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        kernel <kernel@...s.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Zhen, Max" <Max.Zhen@....com>,
        "Santan, Sonal" <sonal.santan@....com>
Subject: Re: [PATCH] mfd: Add Simple PCI MFD driver


On 1/23/23 08:36, Rob Herring wrote:
> On Mon, Jan 23, 2023 at 10:02 AM Vincent Whitchurch
> <vincent.whitchurch@...s.com> wrote:
>> On Mon, Jan 23, 2023 at 04:32:55PM +0100, Lee Jones wrote:
>>> On Mon, 23 Jan 2023, Vincent Whitchurch wrote:
>>>> Add a PCI driver which registers all child nodes specified in the
>>>> devicetree.  It will allow platform devices to be used on virtual
>>>> systems which already support PCI and devicetree, such as UML with
>>>> virt-pci.
>>>>
>>>> The driver has no id_table by default; user space needs to provide one
>>>> using the new_id mechanism in sysfs.
>>> This feels wrong for several reasons.
>>>
>>> Firstly, I think Greg (Cc:ed) will have something to say about this.
>>>
>>> Secondly, this driver does literally nothing.
>> Well, it does do what the commit message says.  If there's another way
>> of accomplishing that, I'm all ears.
>>
>>> Why can't you use of of the other, pre-existing "also register my
>>> children" compatibles?
>>>
>>> See: drivers/bus/simple-pm-bus.c
>>>       drivers/of/platform.c
>> simple-pm-bus registers a platform driver, and drivers/of/platform.c
>> works on the platform bus.  The driver added by this patch is a PCI
>> driver.  So I don't understand how the files you mention could be used
>> here?
>>
>> In case it helps, the relevant nodes in my UML devicetree look something
>> like this:
>>
>>      virtio@2 {
> dtc should complain about this...
>
>>          compatible = "virtio,uml";
> Binding?
>
>>          virtio-device-id = <1234>;
>>          ranges;
>>
>>          pci {
>>                  #address-cells = <3>;
>>                  #size-cells = <2>;
>>                  ranges = <0x0000000 0 0 0 0xf0000000 0 0x20000>;
>>                  compatible = "virtio,device4d2", "pci";
> "pci" is not a valid compatible string.
>
>>                  device_type = "pci";
>>                  bus-range = <0 0>;
>>
>>                  platform_parent: device@0,0 {
>>                          compatible = "pci494f,dc8";
>>                          reg = <0x00000 0 0 0x0 0x10000>;
>>                          ranges;
>>
>>                          uart@...00 {
>>                                  compatible = "google,goldfish-tty";
>>                                  reg = <0x00000 0 0x10000 0 0x10000>;
> This is not a PCI device, so it shouldn't be using PCI addressing.
> 'ranges' needs an entry (for each BAR) to translate to just a normal
> MMIO bus with 1 or 2 address/size cells. Maybe we want a 'simple-bus'
> node for each BAR. The FPGA series needs the same things, but that
> aspect hasn't really been addressed as the first issue is populating
> the PCI devices dynamically.
>
> The DT address translation code should support all this
> (MMIO->PCI->MMIO), but I don't think there's any existing examples. An
> example (that I can test) would be great. If the unittest had that
> example, I'd be thrilled.
(I tried to reply with my comment this morning, but it did not post to
kernel mail alias. I am re-sending it. Please ignore if you already
received my reply)

I have proposed the address format for hardware apertures on PCI BAR.
The address consists of BAR index and offset. It uses the following
encoding:

     0xIooooooo 0xoooooooo

   Where:

     I = BAR index
     oooooo oooooooo = BAR offset

   The endpoint is compatible with 'simple-bus' and contains 'ranges'
   property for translating aperture address to CPU address.

Ref: 
https://lore.kernel.org/lkml/20220305052304.726050-3-lizhi.hou@xilinx.com/
The 64-bit address can use the default translator defined of/address.c

I implemented an example of top of my latest patch
https://lore.kernel.org/lkml/1674183732-5157-1-git-send-email-lizhi.hou@amd.com/
to verify the address translation. The test device tree nodes are defined
to present two hardware apertures on our alveo PCI device:

   pr-isolation:  PCI BAR 0, offset 0x41000, len 0x1000
   xdma: PCI BAR 2, offset 0x0, len 0x40000

     / {
         fragment@0 {
             target-path="";
             __overlay__ {
                 pci-ep-bus@0 {
                     compatible = "simple-bus";
                     #address-cells = <2>;
                     #size-cells = <2>;
                     ranges = <0x0 0x0 0x0 0x0 0x0 0x2000000>;
                     pr_isolate_ulp@...00 {
                         compatible = "xlnx,alveo-pr-isolation";
                         reg = <0x0 0x41000 0x0 0x1000>;
                     };
                 };
                 pci-ep-bus@2 {
                     compatible = "simple-bus";
                     #address-cells = <2>;
                     #size-cells = <2>;
                     ranges = <0x0 0x0 0x20000000 0x0 0x0 0x40000>;
                     alveo-xdma@0 {
                         compatible = "xlnx,alveo-xdma";
                         reg = <0x0 0x0 0x0 0x40000>;
                     };
                 };
             };
         };
     };

Overall, the test is as below

1) added code to generate 'ranges' for PCI endpoint dt node

    00000000 00000000 C30B0000 00000080 10000000 00000000 02000000 (BAR 0)

    20000000 00000000 C30B0000 00000080 14000000 00000000 00040000 (BAR 2)

    ^ bar index                ^^^^^^^^^^^^^BAR IOMEM address

    code link: 
https://github.com/houlz0507/linux-xoclv2/compare/29031e597fd6272f825dd04ba41a38defb77a99a...pci-dt-v6?diff=unified#diff-bf1b86155c18e04c439b74f5a02bad99c91a8c04f3c21243afce996c2174be56

2) overlay the test device tree fragment under PCI endpoint.

3) Alveo pci device driver probe function calls 
of_platform_default_populate().

I can see the BAR index+offset is translated to IO address correctly.

# ls /sys/bus/platform/devices/
0.flash
3f000000.pcie
3f000000.pcie:pci@2,0:pci@0,0:pci@0,0:dev@0,0:pci-ep-bus@0
3f000000.pcie:pci@2,0:pci@0,0:pci@0,0:dev@0,0:pci-ep-bus@2
8010041000.pr_isolate_ulp
8014000000.alveo-xdma

# lspci -vd 10ee:5020 | grep Memory
     Memory at 8010000000 (64-bit, prefetchable) [disabled] [size=32M]
     Memory at 8014000000 (64-bit, prefetchable) [disabled] [size=256K]

This test needs our Alveo PCI device. It might be a reference to create 
unittest.

Thanks,
Lizhi

>
> Rob

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ