Re: pci_ioremap_set_mem_type(), pci_remap_iospace()

Re: pci_ioremap_set_mem_type(), pci_remap_iospace()

[Bjorn Helgaas]

[+cc linux-pci, sorry, due to the @google.com DMARC hassles I can't reply
directly to mail sent to bhelgaas@google.com or to helgaas@kernel.org
(which is currently forwarded to bhelgaas@google.com).  I *can* reply to
linux-pci mail because I have a non google.com account subscribed there.
Sorry again, I know this is TMI.  I'm going to manually insert Thomas'
response so I can respond.]

Thomas Petazzoni wrote:
> On Wed, Apr 27, 2016 at 05:58:27PM -0500, Bjorn Helgaas wrote:
>> Hi Thomas & Liviu,
>> 
>> You added pci_ioremap_set_mem_type(int mem_type) with 1c8c3cf0b523
>> ("ARM: 8060/1: mm: allow sub-architectures to override PCI I/O memory
>> type").
>> 
>> I see this patch on the list: "[PATCH 3/3] ARM: mvebu: implement
>> L2/PCIe deadlock workaround"
>> (http://lists.infradead.org/pipermail/linux-arm-kernel/2014-March/242784.html)
>> that does call pci_ioremap_set_mem_type(), but it doesn't look like
>> that patch ever got merged.
>> 
>> Is it still useful to have pci_ioremap_set_mem_type() even though
>> nobody calls it?

> I am actually about to send a patch that makes use of it, I discussed
> it with Arnd a few days ago / last week.
> 
> Essentially the story was the following. On Cortex-A9 based Marvell
> SoCs, when HW I/O coherency is enabled, you need all non-RAM space to
> be mapped strongly ordered. To this effect, I submitted this patch
> series that introduces pci_ioremap_mem_type() to allow
> platform-specific code to override the memory type used to map PCI I/O
> space.
> 
> The reaction of Arnd after this patch series was to suggest that maybe
> I should just change the memory type for all platforms, so I sent
> another patch doing that.
> 
> But in the end, Russell merged the implementation of
> pci_ioremap_mem_type() from the previous version of the patch series.
> And the remaining patch fell through the cracks. Since PCI I/O is
> rarely used these days, it wasn't noticed.
>
> But right now, I have this in my stack of patches to be sent:
> ...
> diff --git a/arch/arm/mach-mvebu/coherency.c b/arch/arm/mach-mvebu/coherency.c
> index 7e989d6..2d1f06d 100644
> --- a/arch/arm/mach-mvebu/coherency.c
> +++ b/arch/arm/mach-mvebu/coherency.c
> ...
> @@ -186,7 +180,8 @@ static void __init armada_375_380_coherency_init(struct device_node *np)
>         struct device_node *cache_dn;
> 
>         coherency_cpu_base = of_iomap(np, 0);
> -       arch_ioremap_caller = armada_pcie_wa_ioremap_caller;
> +       arch_ioremap_caller = armada_wa_ioremap_caller;
> +       pci_ioremap_set_mem_type(MT_UNCACHED);

This makes sense to me because I think this changes ioremap() to do
the right thing.  But my concern is that ioremap_page_range() doesn't
actually use ioremap(), so pci_ioremap_set_mem_type() has no effect on
that path.  pci_remap_iospace() uses ioremap_page_range(), so I
suspect it will use the wrong attributes.

>> I'm looking at the issue of how we ioremap memory-mapped ioport
>> spaces, and pci_ioremap_mem_type is currently used in the arm-specific
>> pci_ioremap_io():
>> 
>>   int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr)
>>   {
>>     BUG_ON(offset + SZ_64K > IO_SPACE_LIMIT);
>> 
>>     return ioremap_page_range(PCI_IO_VIRT_BASE + offset,
>>                               PCI_IO_VIRT_BASE + offset + SZ_64K,
>>                               phys_addr,
>>                               __pgprot(get_mem_type(pci_ioremap_mem_type)->prot_pte));
>>   }
>> 
>> Also, what about pci_remap_iospace(), added by 8b921acfeffd ("PCI: Add
>> pci_remap_iospace() to map bus I/O resources")?
>> 
>>   int __weak pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr)
>>   {
>>   #if defined(PCI_IOBASE) && defined(CONFIG_MMU)
>>     unsigned long vaddr = (unsigned long)PCI_IOBASE + res->start;
>> 
>>     if (!(res->flags & IORESOURCE_IO))
>>       return -EINVAL;
>> 
>>     if (res->end > IO_SPACE_LIMIT)
>>       return -EINVAL;
>> 
>>     return ioremap_page_range(vaddr, vaddr + resource_size(res), phys_addr,
>>                               pgprot_device(PAGE_KERNEL));
>>     ...

> Yes, I was also surprised to see two functions doing pretty much the
> same, this probably needs to be refactored. There are 11 users of
> pci_ioremap_io() (6 of them being Marvell related), and
> pci_ioremap_io() is provided by ARM specific code.
> 
> On the other hand, pci_remap_iospace() is provided by
> architecture-independent code, and use in 6 PCI controller drivers.
> 
> What about:
> 
>  1/ Providing an alternate version of pci_remap_iospace() that allows
>     to pass the memory attribute to be used.

I'd rather fix ioremap_page_range() somehow, because that's an
exported interface, and if we only fix pci_remap_iospace(), we still
have to worry about ioremap_page_range() doing the wrong thing.

>  2/ Convert all users of pci_ioremap_io() to pci_remap_iospace().
> 
>  3/ Get rid of pci_ioremap_io().

Re: pci_ioremap_set_mem_type(), pci_remap_iospace()

[Thomas Petazzoni]

Hello,

On Thu, 28 Apr 2016 09:19:20 -0500, Bjorn Helgaas wrote:

> >         coherency_cpu_base = of_iomap(np, 0);
> > -       arch_ioremap_caller = armada_pcie_wa_ioremap_caller;
> > +       arch_ioremap_caller = armada_wa_ioremap_caller;
> > +       pci_ioremap_set_mem_type(MT_UNCACHED);
> 
> This makes sense to me because I think this changes ioremap() to do
> the right thing.

What changes ioremap() to do the right thing for all mappings *except*
PCI I/O mappings:

   arch_ioremap_caller = armada_wa_ioremap_caller;

>  But my concern is that ioremap_page_range() doesn't
> actually use ioremap(), so pci_ioremap_set_mem_type() has no effect on
> that path.  pci_remap_iospace() uses ioremap_page_range(), so I
> suspect it will use the wrong attributes.

Indeed, ioremap_page_range() doesn't use ioremap(), and that's why
there is this separate pci_ioremap_set_mem_type() to ensure that
mappings done for PCI I/O space are done with MT_UNCACHED.

And indeed, pci_remap_iospace() would not work for me, as it wouldn't
use the memory attributes set by pci_ioremap_set_mem_type().

To be honest, I am wondering why we're not using the same memory
attribute for PCI I/O space as the memory attributes used for anything
else that's ioremapped. In practice, that's what happens on ARM:
ioremap() uses MT_DEVICE by default, and pci_ioremap_io() uses
MT_DEVICE as well.

> >  1/ Providing an alternate version of pci_remap_iospace() that allows
> >     to pass the memory attribute to be used.
> 
> I'd rather fix ioremap_page_range() somehow, because that's an
> exported interface, and if we only fix pci_remap_iospace(), we still
> have to worry about ioremap_page_range() doing the wrong thing.

What do you want to fix in ioremap_page_range() ? It already allows
passing the memory type that you want, so I'm not sure to understand.

It is worth mentioning that on ARM, the arch_ioremap_caller mechanism
allows platform-specific to do its own magic for each mapping. I.e, it
might decide to use some specific memory attributes, or some specific
way of doing the mappings.

It is used on 5 platforms:

 * EBSA110, where it does a 1:1 mapping with the physical address if I
   understand correctly:

static void __iomem *ebsa110_ioremap_caller(phys_addr_t cookie, size_t size,
                                            unsigned int flags, void *caller)
{
        return (void __iomem *)cookie;
}

 * i.MX3, where it forces the memory attribute to be
   MT_DEVICE_NONSHARED, but only for the mappings that are below
   0x80000000 and not within the L2 cache registers. Otherwise, the
   default memory attribute is used.

 * iop13xx, where it really does some magic calculations to get the
   virtual addresses matching some specific physical addresses (and
   falls back to a real ioremap for other areas).

 * ixp4xx, which does a normal ioremap() for non-PCI devices, and a 1:1
   mapping for PCI stuff.

 * mvebu, where we currently override the memory attribute to strongly
   ordered for PCI devices only (but I need to change that so that all
   mappings are strongly ordered)

All in all, the pgprot_device() mechanism added by Liviu is not really
sufficient to address all those customizations, which is why ARM has
this more complicated way of allowing platform specific code to
override the ioremap behavior.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: pci_ioremap_set_mem_type(), pci_remap_iospace()

[Bjorn Helgaas]

On Thu, Apr 28, 2016 at 04:36:46PM +0200, Thomas Petazzoni wrote:
> Hello,
> 
> On Thu, 28 Apr 2016 09:19:20 -0500, Bjorn Helgaas wrote:
> 
> > >         coherency_cpu_base = of_iomap(np, 0);
> > > -       arch_ioremap_caller = armada_pcie_wa_ioremap_caller;
> > > +       arch_ioremap_caller = armada_wa_ioremap_caller;
> > > +       pci_ioremap_set_mem_type(MT_UNCACHED);
> > 
> > This makes sense to me because I think this changes ioremap() to do
> > the right thing.
> 
> What changes ioremap() to do the right thing for all mappings *except*
> PCI I/O mappings:
> 
>    arch_ioremap_caller = armada_wa_ioremap_caller;
> 
> >  But my concern is that ioremap_page_range() doesn't
> > actually use ioremap(), so pci_ioremap_set_mem_type() has no effect on
> > that path.  pci_remap_iospace() uses ioremap_page_range(), so I
> > suspect it will use the wrong attributes.
> 
> Indeed, ioremap_page_range() doesn't use ioremap(), and that's why
> there is this separate pci_ioremap_set_mem_type() to ensure that
> mappings done for PCI I/O space are done with MT_UNCACHED.
> 
> And indeed, pci_remap_iospace() would not work for me, as it wouldn't
> use the memory attributes set by pci_ioremap_set_mem_type().

Right.  pci_remap_iospace() is a generic interface and should work
on all arches.  It *is* declared __weak, but there's no arch-specific
implementation of it, and I think if we made ioremap_page_range()
truly generic, we could make pci_remap_iospace() non-weak.

> To be honest, I am wondering why we're not using the same memory
> attribute for PCI I/O space as the memory attributes used for anything
> else that's ioremapped. In practice, that's what happens on ARM:
> ioremap() uses MT_DEVICE by default, and pci_ioremap_io() uses
> MT_DEVICE as well.
> 
> > >  1/ Providing an alternate version of pci_remap_iospace() that allows
> > >     to pass the memory attribute to be used.
> > 
> > I'd rather fix ioremap_page_range() somehow, because that's an
> > exported interface, and if we only fix pci_remap_iospace(), we still
> > have to worry about ioremap_page_range() doing the wrong thing.
> 
> What do you want to fix in ioremap_page_range() ? It already allows
> passing the memory type that you want, so I'm not sure to understand.

Yes, but in order for ioremap_page_range() to work on arm, we have to
pass in an arm-specific memory type
(__pgprot(get_mem_type(pci_ioremap_mem_type)->prot_pte)).

We can't do that because ioremap_page_range() is a generic function,
implemented in lib/ioremap.c and exported to modules, and generic
callers only know about pgprot_noncached, pgprot_writecombine,
pgprot_writethrough, pgprot_device, etc.

Re: pci_ioremap_set_mem_type(), pci_remap_iospace()

[Bjorn Helgaas]

[+cc linux-pci, manually inserted reply, sorry again]

Liviu wrote:
> On Wed, Apr 27, 2016 at 05:58:27PM -0500, Bjorn Helgaas wrote:
>> Looking at __arm_ioremap_pfn_caller() makes me
>> suspect that pci_remap_iospace() is not safe on arm.

> Hmm, not sure how you reached that conclusion. __arm_ioremap_pfn_caller()
> does call in the end ioremap_page_range() for ARMv7 || ARM_LPAE, which is
> what pci_remap_iospace() does as well. Beside the possibly different
> pgprot_t values, what would make it unsafe for arm?

The different pgprot_t is exactly what I'm worried about.  The callers of
pci_ioremap_io() probably get the right thing (a pgprot tweaked to deal
with this Cortex problem), but other callers of ioremap_page_range(),
pci_remap_iospace() in particular, don't get that tweak:

  pci_ioremap_io
    ioremap_page_range(..., __pgprot(get_mem_type(pci_ioremap_mem_type)->prot_pte))

  pci_remap_iospace
    ioremap_page_range(..., pgprot_device(PAGE_KERNEL))
      ioremap_pud_range(..., pgprot_device(PAGE_KERNEL))
        ioremap_pmd_range(..., pgprot_device(PAGE_KERNEL))
          ioremap_pte_range(..., pgprot_device(PAGE_KERNEL))
            set_pte_at(..., pfn_pte(pfn, pgprot_device(PAGE_KERNEL)))

Re: pci_ioremap_set_mem_type(), pci_remap_iospace()

[Thomas Petazzoni]

Hello,

On Thu, 28 Apr 2016 09:33:32 -0500, Bjorn Helgaas wrote:
> [+cc linux-pci, manually inserted reply, sorry again]
> 
> Liviu wrote:
> > On Wed, Apr 27, 2016 at 05:58:27PM -0500, Bjorn Helgaas wrote:
> >> Looking at __arm_ioremap_pfn_caller() makes me
> >> suspect that pci_remap_iospace() is not safe on arm.
> 
> > Hmm, not sure how you reached that conclusion. __arm_ioremap_pfn_caller()
> > does call in the end ioremap_page_range() for ARMv7 || ARM_LPAE, which is
> > what pci_remap_iospace() does as well. Beside the possibly different
> > pgprot_t values, what would make it unsafe for arm?
> 
> The different pgprot_t is exactly what I'm worried about.  The callers of
> pci_ioremap_io() probably get the right thing (a pgprot tweaked to deal
> with this Cortex problem), but other callers of ioremap_page_range(),
> pci_remap_iospace() in particular, don't get that tweak:
> 
>   pci_ioremap_io
>     ioremap_page_range(..., __pgprot(get_mem_type(pci_ioremap_mem_type)->prot_pte))
> 
>   pci_remap_iospace
>     ioremap_page_range(..., pgprot_device(PAGE_KERNEL))
>       ioremap_pud_range(..., pgprot_device(PAGE_KERNEL))
>         ioremap_pmd_range(..., pgprot_device(PAGE_KERNEL))
>           ioremap_pte_range(..., pgprot_device(PAGE_KERNEL))
>             set_pte_at(..., pfn_pte(pfn, pgprot_device(PAGE_KERNEL)))

Indeed. Which is why we should get rid of one of the two interfaces.
Since pci_remap_iospace() is the generic one, I guess that's the one we
should migrate to.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel