linux-kernel - Re: [PATCH v3] PCI: of: Avoid pci_remap_iospace() when PCI

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] [day] [month] [year] [list]

Message-ID: <CAMhs-H-JKPpWBAURD3NrTtxcXqqB1TM=o4n32+7sjoUYMpZyuQ@mail.gmail.com>
Date:   Fri, 24 Sep 2021 19:15:13 +0200
From:   Sergio Paracuellos <sergio.paracuellos@...il.com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     linux-pci <linux-pci@...r.kernel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-staging@...ts.linux.dev, gregkh <gregkh@...uxfoundation.org>,
        Liviu Dudau <Liviu.Dudau@....com>,
        Rob Herring <robh@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>
Subject: Re: [PATCH v3] PCI: of: Avoid pci_remap_iospace() when PCI_IOBASE not defined

Hi Arnd,

On Fri, Sep 24, 2021 at 7:04 PM Arnd Bergmann <arnd@...db.de> wrote:
>
> On Fri, Sep 24, 2021 at 6:45 PM Sergio Paracuellos
> <sergio.paracuellos@...il.com> wrote:
> > On Fri, Sep 24, 2021 at 3:28 PM Arnd Bergmann <arnd@...db.de> wrote:
> > > On Fri, Sep 24, 2021 at 2:46 PM Sergio Paracuellos
> > >
> > > Ok, sounds good. I would still suggest using
> > > "#define PCI_IOBASE mips_io_port_base", so it has the same meaning
> > > as on other architectures (the virtual address of port 0), and replace
> > > the hardcoded base with the CPU address you read from DT to
> > > make that code more portable. As a general rule, DT-enabled drivers
> > > should contain no hardcoded addresses.
> >
> > Yes, it must be cleaned. I was only explaining a possible way to proceed.
>
> Ok
>
> > So, the changes would be:
> > 1) Reverting already added two commits in staging-tree [0] and [1].
> > (two revert patches)
> > 2) Setting PCI_IOBASE to 'mips_io_port_base' so the spaces.h become: (one patch)
> >
> > $ cat arch/mips/include/asm/mach-ralink/spaces.h
> > /* SPDX-License-Identifier: GPL-2.0 */
> > #ifndef __ASM_MACH_RALINK_SPACES_H_
> > #define __ASM_MACH_RALINK_SPACES_H_
> >
> > #define PCI_IOBASE    mips_io_port_base
> > #define PCI_IOSIZE    SZ_16M
> > #define IO_SPACE_LIMIT  (PCI_IOSIZE - 1)
>
> As a minor comment, I would make the PCI_IOSIZE only 64KB in this
> case, unless plan to support ralink/mediatek SoCs that have a multiple
> PCIe domains with distinct 64KB windows.

Ok, I will adjust to SZ_64K , then.

>
> > 3) Change the value written in RALINK_PCI_IOBASE to be sure the value
> > written takes into account address before linux port translation (one
> > patch):
> >
> > pcie_write(pcie, entry->res->start - entry->offset, RALINK_PCI_IOBASE);
>
> ok
>
> > 4) Virtually Map cpu physical address 0x1e160000 and set
> > 'mips_io_port_base' to that virtual address. Something like the
> > following (one patch):
> >
> > static int mt7621_set_io(struct device *dev)
> > {
> >     struct device_node *node = dev->of_node;
> >     struct of_pci_range_parser parser;
> >     struct of_pci_range range;
> >     unsigned long vaddr;
> >     int ret = -EINVAL;
> >
> >     ret = of_pci_range_parser_init(&parser, node);
> >     if (ret)
> >         return ret;
> >
> >     for_each_of_pci_range(&parser, &range) {
> >         switch (range.flags & IORESOURCE_TYPE_BITS) {
> >         case IORESOURCE_IO:
> >             vaddr = (unsigned long)ioremap(range.cpu_addr, range.size);
> >             set_io_port_base(vaddr);
> >             ret = 0;
> >             break;
> >         }
> >     }
> >
> >     return ret;
> > }
>
> This looks like it does the right thing, but conceptually this would belong into
> the mips specific pci_remap_iospace() as we discussed a few emails back,
> not inside the driver. pci_remap_iospace() does get the CPU address
> as an argument, so it just needs to ioremap()/set_io_port_base() it.

Ok, this is what I had in mind, but since this change also needs to
add the #ifdef to the whole 'pci_remap_iospace' function of core apis,
just to be sure that would be a valid approach. Thanks for
clarification.

>
> > And now my concerns:
> > 1) We have to read DT range IO values in the driver and those values
> > will be also parsed by core apis but converting them to linux io
> > ports. Should this be done by the driver or is there a better way to
> > abstract this to don't do things twice?
> > 2) 'set_io_port_base()' function does what we want but it is only
> > mips. We already have the iocu stuff there and the driver is mips
> > anyway, but it is worth to comment this just in case.
>
> I think in both cases the core APIs should do what we need, with the
> change to the mips pci_remap_iospace() I mention. If there is anything
> missing in the core API that you need, we can discuss extending those,
> e.g. to store additional data in the pci_host_bridge structure.

Understood. I'll try to test all of these changes during this weekend
and hopefully send these patches during next week.

Thank you very much for your time and support.

Best regards,
    Sergio Paracuellos

>
>          Arnd