Re: About commit "io: change inX() to have their own IO barrier overrides"

Re: About commit "io: change inX() to have their own IO barrier overrides"

[Arnd Bergmann]

On Tue, Mar 3, 2020 at 2:18 PM John Garry <john.garry@huawei.com> wrote:
>
> + linux-arch
>
> For background, see
> https://lore.kernel.org/lkml/2e80d7bc-32a0-cc40-00a9-8a383a1966c2@huawei.com/
>
> >>
> >> So today only ARM64 uses it for this relevant code, above. But maybe
> >> others in future will want to use it - any arch without native IO port
> >> access is a candidate.
> >
> > I'm looking at Arnd here for help.
> >
> >>
> >>>
> >>> As long as the expectations are set, I see no reason why it shouldn't
> >>> but, I'll let Arnd comment on it too.
> >>
> >> ok, so it looks reasonable consider replicating your change for ***, above.
>
> To be clear, I would make this change in lib/logic_pio.c since
> __io_pbr() can be overridden per-arch:
>
>   #define BUILD_LOGIC_IO(bw, type)
>   type logic_in##bw(unsigned long addr)
>   {
>        type ret = (type)~0;
>        if (addr < MMIO_UPPER_LIMIT) {
> -          ret = read##bw(PCI_IOBASE + addr);
> +          __io_pbr();
> +          ret = __raw_read##bw(PCI_IOBASE + addr);
> +          __io_pbr();

__io_par();

>        } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {
>            struct logic_pio_hwaddr *entry = find_io_range(addr);
>
> ...
>
> (forgetting leX_to_cpu for the moment)

Yes, I suppose this is required to get consistent behavior on arm64,
which overrides __io_par() but not __io_ar(), with the current code
the barrier after read is weaker when LOGIC_PIO is enabled than it
is otherwise.

For other architectures, I suppose we would need another indirection
level, as those can also override the default inb() itself to do something
other than readb(PCI_IOBASE + addr), and that is not handled
here either. We can do that if we need LOGIC_PIO on a second
architecture.

       Arnd

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

Re: About commit "io: change inX() to have their own IO barrier overrides"

[John Garry]

On 03/03/2020 16:40, Arnd Bergmann wrote:
> On Tue, Mar 3, 2020 at 2:18 PM John Garry <john.garry@huawei.com> wrote:
>>
>> + linux-arch
>>
>> For background, see
>> https://lore.kernel.org/lkml/2e80d7bc-32a0-cc40-00a9-8a383a1966c2@huawei.com/
>>
>>>>
>>>> So today only ARM64 uses it for this relevant code, above. But maybe
>>>> others in future will want to use it - any arch without native IO port
>>>> access is a candidate.
>>>
>>> I'm looking at Arnd here for help.
>>>
>>>>
>>>>>
>>>>> As long as the expectations are set, I see no reason why it shouldn't
>>>>> but, I'll let Arnd comment on it too.
>>>>
>>>> ok, so it looks reasonable consider replicating your change for ***, above.
>>
>> To be clear, I would make this change in lib/logic_pio.c since
>> __io_pbr() can be overridden per-arch:
>>
>>    #define BUILD_LOGIC_IO(bw, type)
>>    type logic_in##bw(unsigned long addr)
>>    {
>>         type ret = (type)~0;
>>         if (addr < MMIO_UPPER_LIMIT) {
>> -          ret = read##bw(PCI_IOBASE + addr);
>> +          __io_pbr();
>> +          ret = __raw_read##bw(PCI_IOBASE + addr);
>> +          __io_pbr();
> 
> __io_par();
> 
>>         } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {
>>             struct logic_pio_hwaddr *entry = find_io_range(addr);
>>
>> ...
>>
>> (forgetting leX_to_cpu for the moment)
> 
> Yes, I suppose this is required to get consistent behavior on arm64,
> which overrides __io_par() but not __io_ar(), with the current code
> the barrier after read is weaker when LOGIC_PIO is enabled than it
> is otherwise.

Ok.

Apart from that, this code is somewhat hidden. I mean, most people would 
consider generic IO port accessors come from asm-generic/io.h only, 
which is not the case here. Maybe this can be better integrated into 
asm-generic/io.h, the only hint today being the logic_pio.h include half 
way through the file.

> 
> For other architectures, I suppose we would need another indirection
> level, as those can also override the default inb() itself to do something
> other than readb(PCI_IOBASE + addr), and that is not handled
> here either. We can do that if we need LOGIC_PIO on a second
> architecture.

Jiaxun Yang did mention that MIPS may want to move away from its own IO 
space management.

Thanks,
John

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

Re: About commit "io: change inX() to have their own IO barrier overrides"

[Sinan Kaya]

On 3/3/2020 11:40 AM, Arnd Bergmann wrote:
>> -          ret = read##bw(PCI_IOBASE + addr);
>> +          __io_pbr();
>> +          ret = __raw_read##bw(PCI_IOBASE + addr);
>> +          __io_pbr();
> __io_par();
> 

Why do we need to change read##bw above?

read##bw already provides strong ordering guarantees across multiple
architectures.

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

Re: About commit "io: change inX() to have their own IO barrier overrides"

[Arnd Bergmann]

On Fri, Mar 6, 2020 at 4:44 AM Sinan Kaya <okaya@kernel.org> wrote:
>
> On 3/3/2020 11:40 AM, Arnd Bergmann wrote:
> >> -          ret = read##bw(PCI_IOBASE + addr);
> >> +          __io_pbr();
> >> +          ret = __raw_read##bw(PCI_IOBASE + addr);
> >> +          __io_pbr();
> > __io_par();
> >
>
> Why do we need to change read##bw above?
>
> read##bw already provides strong ordering guarantees across multiple
> architectures.

The exact semantics of inl() and readl() are slightly different, so they
have distinct sets of barriers in the asm-generic/io.h implementation.

For instance, the arm64 architectures defines in_par() as '__iormb(v)',
but defines __io_ar() as a  '__rmb()'. Similarly, riscv defines them
as "fence i,ior" and "fence i,r".

You could argue that the definitions are wrong (I have not checked the
history of the definitions), but as long as the inb() in asm-generic/io.h
uses those, the implementation in lib/logic_pio.c uses the same ones
to make the two behave the same way.

       Arnd

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

Re: About commit "io: change inX() to have their own IO barrier overrides"

[John Garry]

On 06/03/2020 07:54, Arnd Bergmann wrote:
> On Fri, Mar 6, 2020 at 4:44 AM Sinan Kaya <okaya@kernel.org> wrote:
>>
>> On 3/3/2020 11:40 AM, Arnd Bergmann wrote:
>>>> -          ret = read##bw(PCI_IOBASE + addr);
>>>> +          __io_pbr();
>>>> +          ret = __raw_read##bw(PCI_IOBASE + addr);
>>>> +          __io_pbr();
>>> __io_par();
>>>
>>
>> Why do we need to change read##bw above?
>>
>> read##bw already provides strong ordering guarantees across multiple
>> architectures.
> 
> The exact semantics of inl() and readl() are slightly different, so they
> have distinct sets of barriers in the asm-generic/io.h implementation.
> 
> For instance, the arm64 architectures defines in_par() as '__iormb(v)',
> but defines __io_ar() as a  '__rmb()'. Similarly, riscv defines them
> as "fence i,ior" and "fence i,r".
> 
> You could argue that the definitions are wrong (I have not checked the
> history of the definitions), but as long as the inb() in asm-generic/io.h
> uses those, the implementation in lib/logic_pio.c uses the same ones
> to make the two behave the same way.
> 

So the change would look like:

-- a/lib/logic_pio.c
+++ b/lib/logic_pio.c
@@ -229,13 +229,21 @@ unsigned long 
logic_pio_trans_cpuaddr(resource_size_t addr)
  }

  #if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE)
+
+#define logic_in_to_cpu_b(x) (x)
+#define logic_in_to_cpu_w(x) __le16_to_cpu(x)
+#define logic_in_to_cpu_l(x) __le32_to_cpu(x)
+
  #define BUILD_LOGIC_IO(bw, type)                                      \
  type logic_in##bw(unsigned long addr)                                 \
  {                                                                     \
         type ret = (type)~0;                                           \
                                                                        \
         if (addr < MMIO_UPPER_LIMIT) {                                 \
-               ret = read##bw(PCI_IOBASE + addr);                     \
+               void __iomem *_addr = PCI_IOBASE + addr;               \
+               __io_pbr();                                            \
+               ret = logic_in_to_cpu_##bw(__raw_read##bw(_addr));     \
+               __io_par(ret);                                         \
         } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {\
                 struct logic_pio_hwaddr *entry = find_io_rang

We could prob combine the le_to_cpu and __raw_read into a single macro.

John

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

Re: About commit "io: change inX() to have their own IO barrier overrides"

[Arnd Bergmann]

On Fri, Mar 6, 2020 at 11:40 AM John Garry <john.garry@huawei.com> wrote:
> On 06/03/2020 07:54, Arnd Bergmann wrote:
> > On Fri, Mar 6, 2020 at 4:44 AM Sinan Kaya <okaya@kernel.org> wrote:
> -- a/lib/logic_pio.c
> +++ b/lib/logic_pio.c
> @@ -229,13 +229,21 @@ unsigned long
> logic_pio_trans_cpuaddr(resource_size_t addr)
>   }
>
>   #if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE)
> +
> +#define logic_in_to_cpu_b(x) (x)
> +#define logic_in_to_cpu_w(x) __le16_to_cpu(x)
> +#define logic_in_to_cpu_l(x) __le32_to_cpu(x)
> +
>   #define BUILD_LOGIC_IO(bw, type)                                      \
>   type logic_in##bw(unsigned long addr)                                 \
>   {                                                                     \
>          type ret = (type)~0;                                           \
>                                                                         \
>          if (addr < MMIO_UPPER_LIMIT) {                                 \
> -               ret = read##bw(PCI_IOBASE + addr);                     \
> +               void __iomem *_addr = PCI_IOBASE + addr;               \
> +               __io_pbr();                                            \
> +               ret = logic_in_to_cpu_##bw(__raw_read##bw(_addr));     \
> +               __io_par(ret);                                         \
>          } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {\
>                  struct logic_pio_hwaddr *entry = find_io_rang
>
> We could prob combine the le_to_cpu and __raw_read into a single macro.

What is the purpose of splitting out the byteswap rather than leaving the
open-coded rather than __le16_to_cpu(__raw_readw(PCI_IOBASE + addr))?

If this is needed to work across architectures, how about adding
an intermediate __raw_inw() etc in asm-generic/io.h like

#ifndef __raw_inw
#define __raw_inw(a) __raw_readw(PCI_IOBASE + addr));
#endif

#include <linux/logic_pio.h>

#ifndef inw
static inline u16 inw(unsigned long addr)
{
        u16 val;

        __io_pbr();
        val = __le16_to_cpu(__raw_inw(addr));
        __io_par(val);
        return val;
}
#endif

       Arnd

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

Re: About commit "io: change inX() to have their own IO barrier overrides"

[John Garry]

On 06/03/2020 15:16, Arnd Bergmann wrote:
> On Fri, Mar 6, 2020 at 11:40 AM John Garry <john.garry@huawei.com> wrote:
>> On 06/03/2020 07:54, Arnd Bergmann wrote:
>>> On Fri, Mar 6, 2020 at 4:44 AM Sinan Kaya <okaya@kernel.org> wrote:
>> -- a/lib/logic_pio.c
>> +++ b/lib/logic_pio.c
>> @@ -229,13 +229,21 @@ unsigned long
>> logic_pio_trans_cpuaddr(resource_size_t addr)
>>    }
>>
>>    #if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE)
>> +
>> +#define logic_in_to_cpu_b(x) (x)
>> +#define logic_in_to_cpu_w(x) __le16_to_cpu(x)
>> +#define logic_in_to_cpu_l(x) __le32_to_cpu(x)
>> +
>>    #define BUILD_LOGIC_IO(bw, type)                                      \

Note: The "bw" argument name could be improved to "bwl", since this 
macro is used for building inl() also.

>>    type logic_in##bw(unsigned long addr)                                 \
>>    {                                                                     \
>>           type ret = (type)~0;                                           \
>>                                                                          \
>>           if (addr < MMIO_UPPER_LIMIT) {                                 \
>> -               ret = read##bw(PCI_IOBASE + addr);                     \
>> +               void __iomem *_addr = PCI_IOBASE + addr;               \
>> +               __io_pbr();                                            \
>> +               ret = logic_in_to_cpu_##bw(__raw_read##bw(_addr));     \
>> +               __io_par(ret);                                         \
>>           } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {\
>>                   struct logic_pio_hwaddr *entry = find_io_rang
>>
>> We could prob combine the le_to_cpu and __raw_read into a single macro.
> 
> What is the purpose of splitting out the byteswap rather than leaving the
> open-coded rather than __le16_to_cpu(__raw_readw(PCI_IOBASE + addr))?

I'm just copying what is in asm-generic io.h, which uses the 16b and 32b 
byteswaps in the w and l variants, respectively.

> 
> If this is needed to work across architectures, how about adding
> an intermediate __raw_inw() etc in asm-generic/io.h like
> 
> #ifndef __raw_inw
> #define __raw_inw(a) __raw_readw(PCI_IOBASE + addr));
> #endif
> 
> #include <linux/logic_pio.h>
> 
> #ifndef inw
> static inline u16 inw(unsigned long addr)
> {
>          u16 val;
> 
>          __io_pbr();
>          val = __le16_to_cpu(__raw_inw(addr));
>          __io_par(val);
>          return val;
> }
> #endif
> 

The idea is good, but it would be nice if we just somehow use a common 
asm-generic io.h definition directly in logic_pio.c, like:

asm-generic io.h:

#ifndef __raw_inw // name?
#define __raw_inw __raw_inw
static inline u16 __raw_inw(unsigned long addr)
{
	u16 val;

	__io_pbr();
	val = __le16_to_cpu(__raw_readw(addr));
	__io_par(val);
	return val;
}
#endif

#include <linux/logic_pio.h>

#ifndef inw
#define inw __raw_inw
#endif

logic_pio.c:

  #define BUILD_LOGIC_IO(bw, type)                                      \
  type logic_in##bw(unsigned long addr)                                 \
  {                                                                     \
         type ret = (type)~0;                                           \
                                                                        \
         if (addr < MMIO_UPPER_LIMIT) {                                 \
-               ret = read##bw(PCI_IOBASE + addr);                     \
+               ret = __raw_in##bw(PCI_IOBASE + addr);                 \
         } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {\
                 struct logic_pio_hwaddr *entry = find_io_rang

Thanks,
John

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

Re: About commit "io: change inX() to have their own IO barrier overrides"

[Arnd Bergmann]

On Fri, Mar 6, 2020 at 5:18 PM John Garry <john.garry@huawei.com> wrote:
> On 06/03/2020 15:16, Arnd Bergmann wrote:
> > On Fri, Mar 6, 2020 at 11:40 AM John Garry <john.garry@huawei.com> wrote:
> >> On 06/03/2020 07:54, Arnd Bergmann wrote:
> >>> On Fri, Mar 6, 2020 at 4:44 AM Sinan Kaya <okaya@kernel.org> wrote:
> >> -- a/lib/logic_pio.c
> >> +++ b/lib/logic_pio.c
> >> @@ -229,13 +229,21 @@ unsigned long
> >> logic_pio_trans_cpuaddr(resource_size_t addr)
> >>    }
> >>
> >>    #if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE)
> >> +
> >> +#define logic_in_to_cpu_b(x) (x)
> >> +#define logic_in_to_cpu_w(x) __le16_to_cpu(x)
> >> +#define logic_in_to_cpu_l(x) __le32_to_cpu(x)
> >> +
> >>    #define BUILD_LOGIC_IO(bw, type)                                      \
>
> Note: The "bw" argument name could be improved to "bwl", since this
> macro is used for building inl() also.
>
> >>    type logic_in##bw(unsigned long addr)                                 \
> >>    {                                                                     \
> >>           type ret = (type)~0;                                           \
> >>                                                                          \
> >>           if (addr < MMIO_UPPER_LIMIT) {                                 \
> >> -               ret = read##bw(PCI_IOBASE + addr);                     \
> >> +               void __iomem *_addr = PCI_IOBASE + addr;               \
> >> +               __io_pbr();                                            \
> >> +               ret = logic_in_to_cpu_##bw(__raw_read##bw(_addr));     \
> >> +               __io_par(ret);                                         \
> >>           } else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {\
> >>                   struct logic_pio_hwaddr *entry = find_io_rang
> >>
> >> We could prob combine the le_to_cpu and __raw_read into a single macro.
> >
> > What is the purpose of splitting out the byteswap rather than leaving the
> > open-coded rather than __le16_to_cpu(__raw_readw(PCI_IOBASE + addr))?
>
> I'm just copying what is in asm-generic io.h, which uses the 16b and 32b
> byteswaps in the w and l variants, respectively.

Sure, but I don't think that needs another macro.

>
> The idea is good, but it would be nice if we just somehow use a common
> asm-generic io.h definition directly in logic_pio.c, like:
>
> asm-generic io.h:
>
> #ifndef __raw_inw // name?
> #define __raw_inw __raw_inw
> static inline u16 __raw_inw(unsigned long addr)
> {
>         u16 val;
>
>         __io_pbr();
>         val = __le16_to_cpu(__raw_readw(addr));
>         __io_par(val);
>         return val;
> }
> #endif
>
> #include <linux/logic_pio.h>
>
> #ifndef inw
> #define inw __raw_inw
> #endif

Yes, makes sense. Maybe __arch_inw() then? Not great either, but I think
that's better than __raw_inw() because __raw_* would sound like it
mirrors __raw_readl() that lacks the barriers and byteswaps.

      Arnd

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

Re: About commit "io: change inX() to have their own IO barrier overrides"

[John Garry]

On 06/03/2020 16:29, Arnd Bergmann wrote:
>> The idea is good, but it would be nice if we just somehow use a common
>> asm-generic io.h definition directly in logic_pio.c, like:
>>
>> asm-generic io.h:
>>
>> #ifndef __raw_inw // name?
>> #define __raw_inw __raw_inw
>> static inline u16 __raw_inw(unsigned long addr)
>> {
>>          u16 val;
>>
>>          __io_pbr();
>>          val = __le16_to_cpu(__raw_readw(addr));
>>          __io_par(val);
>>          return val;
>> }
>> #endif
>>
>> #include <linux/logic_pio.h>
>>
>> #ifndef inw
>> #define inw __raw_inw
>> #endif
> Yes, makes sense. Maybe __arch_inw() then? Not great either, but I think
> that's better than __raw_inw() because __raw_* would sound like it
> mirrors __raw_readl() that lacks the barriers and byteswaps.

Right, I had the same concern. And maybe the "arch" prefix is 
misleading. Just __inw could be ok, and hopefully not conflict with the 
arch/arm/mach-* definitions.

Thanks,
John

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

Re: About commit "io: change inX() to have their own IO barrier overrides"

[John Garry]

On 06/03/2020 16:43, John Garry wrote:
> On 06/03/2020 16:29, Arnd Bergmann wrote:
>>> The idea is good, but it would be nice if we just somehow use a common
>>> asm-generic io.h definition directly in logic_pio.c, like:
>>>
>>> asm-generic io.h:
>>>
>>> #ifndef __raw_inw // name?
>>> #define __raw_inw __raw_inw
>>> static inline u16 __raw_inw(unsigned long addr)
>>> {
>>>          u16 val;
>>>
>>>          __io_pbr();
>>>          val = __le16_to_cpu(__raw_readw(addr));
>>>          __io_par(val);
>>>          return val;
>>> }
>>> #endif
>>>
>>> #include <linux/logic_pio.h>
>>>
>>> #ifndef inw
>>> #define inw __raw_inw
>>> #endif
>> Yes, makes sense. Maybe __arch_inw() then? Not great either, but I think
>> that's better than __raw_inw() because __raw_* would sound like it
>> mirrors __raw_readl() that lacks the barriers and byteswaps.
> 
> Right, I had the same concern. And maybe the "arch" prefix is 
> misleading. Just __inw could be ok, and hopefully not conflict with the 
> arch/arm/mach-* definitions.
> 

I think that it hasn't been mentioned already, but it looks like the 
outX methods also need the same treatment, from a7851aa54c.

thanks,
John

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

Re: About commit "io: change inX() to have their own IO barrier overrides"

[Sinan Kaya]

On 3/6/2020 2:54 AM, Arnd Bergmann wrote:
> The exact semantics of inl() and readl() are slightly different, so they
> have distinct sets of barriers in the asm-generic/io.h implementation.
> 
> For instance, the arm64 architectures defines in_par() as '__iormb(v)',
> but defines __io_ar() as a  '__rmb()'. Similarly, riscv defines them
> as "fence i,ior" and "fence i,r".

makes sense

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