| Message ID | 2d9aa000afe81b45157617664134b871207c2067.1643206612.git.karolinadrobnik@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce memblock simulator | expand |
On Thu, Jan 27, 2022 at 02:21:25PM +0100, Karolina Drobnik wrote:
> Add a dummy io.h header.
Rather begs the question of what memblock.c needs from linux/io.h.
Wouldn't it be better to:
+++ b/mm/memblock.c
@@ -18,7 +18,6 @@
#include <linux/memblock.h>
#include <asm/sections.h>
-#include <linux/io.h>
#include "internal.h"
(allmodconfig on x86-64 builds fine with this; I have not done an
extended sweep of other arches / build options).
On Thu, 2022-01-27 at 14:09 +0000, Matthew Wilcox wrote: > On Thu, Jan 27, 2022 at 02:21:25PM +0100, Karolina Drobnik wrote: > > Add a dummy io.h header. > > Rather begs the question of what memblock.c needs from linux/io.h. > > Wouldn't it be better to: > > +++ b/mm/memblock.c > @@ -18,7 +18,6 @@ > #include <linux/memblock.h> > > #include <asm/sections.h> > -#include <linux/io.h> > > #include "internal.h" > That was something I considered in the very beginning, but didn't have a chance to verify it works for all architectures. I can take a look after I'm finished with other v2 changes. > (allmodconfig on x86-64 builds fine with this; I have not done an > extended sweep of other arches / build options).
On Fri, Jan 28, 2022 at 12:21:59PM +0100, Karolina Drobnik wrote: > On Thu, 2022-01-27 at 14:09 +0000, Matthew Wilcox wrote: > > On Thu, Jan 27, 2022 at 02:21:25PM +0100, Karolina Drobnik wrote: > > > Add a dummy io.h header. > > > > Rather begs the question of what memblock.c needs from linux/io.h. > > > > Wouldn't it be better to: > > > > +++ b/mm/memblock.c > > @@ -18,7 +18,6 @@ > > #include <linux/memblock.h> > > > > #include <asm/sections.h> > > -#include <linux/io.h> > > > > #include "internal.h" > > > > That was something I considered in the very beginning, but didn't have > a chance to verify it works for all architectures. I can take a look > after I'm finished with other v2 changes. > > > (allmodconfig on x86-64 builds fine with this; I have not done an > > extended sweep of other arches / build options). I did a sweep for defconfigs for all arches and all were fine. Karolina, please send the formal patch. Let's see how kbuild bot reacts.
On Sun, Jan 30, 2022 at 06:10:00PM +0200, Mike Rapoport wrote: > On Fri, Jan 28, 2022 at 12:21:59PM +0100, Karolina Drobnik wrote: > > On Thu, 2022-01-27 at 14:09 +0000, Matthew Wilcox wrote: > > > On Thu, Jan 27, 2022 at 02:21:25PM +0100, Karolina Drobnik wrote: > > > > Add a dummy io.h header. > > > > > > Rather begs the question of what memblock.c needs from linux/io.h. > > > > > > Wouldn't it be better to: > > > > > > +++ b/mm/memblock.c > > > @@ -18,7 +18,6 @@ > > > #include <linux/memblock.h> > > > > > > #include <asm/sections.h> > > > -#include <linux/io.h> > > > > > > #include "internal.h" > > > > > > > That was something I considered in the very beginning, but didn't have > > a chance to verify it works for all architectures. I can take a look > > after I'm finished with other v2 changes. > > > > > (allmodconfig on x86-64 builds fine with this; I have not done an > > > extended sweep of other arches / build options). > > I did a sweep for defconfigs for all arches and all were fine. Thanks for doing the sweep, Mike. I think I found a deeper problem which is masked due to our maze of header files: include/asm-generic/io.h:#ifndef virt_to_phys include/asm-generic/io.h:#define virt_to_phys virt_to_phys so there's an assumption that <asm/io.h> defines virt_to_phys(). You can see that in a number of architectures, eg: arch/alpha/include/asm/io.h:static inline unsigned long virt_to_phys(volatile void *address) arch/ia64/include/asm/io.h:#define virt_to_phys virt_to_phys arch/mips/include/asm/io.h:#define virt_to_phys virt_to_phys arch/nios2/include/asm/io.h:#define virt_to_phys(vaddr) \ arch/parisc/include/asm/io.h:#define virt_to_phys(a) ((unsigned long)__pa(a)) arch/powerpc/include/asm/io.h:#define virt_to_phys virt_to_phys arch/sh/include/asm/io.h:#define virt_to_phys(address) ((unsigned long)(address)) arch/x86/include/asm/io.h:#define virt_to_phys virt_to_phys That's clearly not the right place to define it. Two architectures put it in asm/memory.h: arch/arm/include/asm/memory.h:#define virt_to_phys virt_to_phys arch/arm64/include/asm/memory.h:#define virt_to_phys virt_to_phys then: arch/m68k/include/asm/virtconvert.h:#define virt_to_phys virt_to_phys arch/sparc/include/asm/page_32.h:#define virt_to_phys __pa arch/sparc/include/asm/page_64.h:#define virt_to_phys __pa This needs to be properly sorted out, but I don't want to tell Karolina that's now her job as a prerequisite for merging this patchset; that would be unfair. Cc'ing Arnd. This is the kind of awful mess that he loves fixing ;-)
On Sun, Jan 30, 2022 at 05:53:24PM +0000, Matthew Wilcox wrote: > On Sun, Jan 30, 2022 at 06:10:00PM +0200, Mike Rapoport wrote: > > On Fri, Jan 28, 2022 at 12:21:59PM +0100, Karolina Drobnik wrote: > > > On Thu, 2022-01-27 at 14:09 +0000, Matthew Wilcox wrote: > > > > On Thu, Jan 27, 2022 at 02:21:25PM +0100, Karolina Drobnik wrote: > > > > > Add a dummy io.h header. > > > > > > > > Rather begs the question of what memblock.c needs from linux/io.h. > > > > > > > > Wouldn't it be better to: > > > > > > > > +++ b/mm/memblock.c > > > > @@ -18,7 +18,6 @@ > > > > #include <linux/memblock.h> > > > > > > > > #include <asm/sections.h> > > > > -#include <linux/io.h> > > > > > > > > #include "internal.h" > > > > > > > > > > That was something I considered in the very beginning, but didn't have > > > a chance to verify it works for all architectures. I can take a look > > > after I'm finished with other v2 changes. > > > > > > > (allmodconfig on x86-64 builds fine with this; I have not done an > > > > extended sweep of other arches / build options). > > > > I did a sweep for defconfigs for all arches and all were fine. > > Thanks for doing the sweep, Mike. > > I think I found a deeper problem which is masked due to our maze of > header files: > > include/asm-generic/io.h:#ifndef virt_to_phys > include/asm-generic/io.h:#define virt_to_phys virt_to_phys > > so there's an assumption that <asm/io.h> defines virt_to_phys(). > You can see that in a number of architectures, eg: > > arch/alpha/include/asm/io.h:static inline unsigned long virt_to_phys(volatile void *address) > arch/ia64/include/asm/io.h:#define virt_to_phys virt_to_phys > arch/mips/include/asm/io.h:#define virt_to_phys virt_to_phys > arch/nios2/include/asm/io.h:#define virt_to_phys(vaddr) \ > arch/parisc/include/asm/io.h:#define virt_to_phys(a) ((unsigned long)__pa(a)) > arch/powerpc/include/asm/io.h:#define virt_to_phys virt_to_phys > arch/sh/include/asm/io.h:#define virt_to_phys(address) ((unsigned long)(address)) > arch/x86/include/asm/io.h:#define virt_to_phys virt_to_phys > > That's clearly not the right place to define it. Two architectures > put it in asm/memory.h: > > arch/arm/include/asm/memory.h:#define virt_to_phys virt_to_phys > arch/arm64/include/asm/memory.h:#define virt_to_phys virt_to_phys > > then: > > arch/m68k/include/asm/virtconvert.h:#define virt_to_phys virt_to_phys > arch/sparc/include/asm/page_32.h:#define virt_to_phys __pa > arch/sparc/include/asm/page_64.h:#define virt_to_phys __pa I'd say sparc picked the most appropriate header for it. memory.h could also work fine, but it's only present on some arches. I'll take a deeper look, thanks for checking this. > This needs to be properly sorted out, but I don't want to tell Karolina > that's now her job as a prerequisite for merging this patchset; that > would be unfair. Totally agree. > Cc'ing Arnd. This is the kind of awful mess that he loves fixing ;-) Heh, me too :)
Hi Mike, On Sun, 2022-01-30 at 18:10 +0200, Mike Rapoport wrote: > On Fri, Jan 28, 2022 at 12:21:59PM +0100, Karolina Drobnik wrote: > > On Thu, 2022-01-27 at 14:09 +0000, Matthew Wilcox wrote: > > > On Thu, Jan 27, 2022 at 02:21:25PM +0100, Karolina Drobnik wrote: > > > > Add a dummy io.h header. > > > > > > Rather begs the question of what memblock.c needs from > > > linux/io.h. > > > > > > Wouldn't it be better to: > > > > > > +++ b/mm/memblock.c > > > @@ -18,7 +18,6 @@ > > > #include <linux/memblock.h> > > > > > > #include <asm/sections.h> > > > -#include <linux/io.h> > > > > > > #include "internal.h" > > > > > > > That was something I considered in the very beginning, but didn't > > have > > a chance to verify it works for all architectures. I can take a > > look > > after I'm finished with other v2 changes. > > > > > (allmodconfig on x86-64 builds fine with this; I have not done an > > > extended sweep of other arches / build options). > > I did a sweep for defconfigs for all arches and all were fine. Thanks for doing the sweep, it's very helpful. > Karolina, please send the formal patch. Let's see how kbuild bot > reacts. OK, will send something later today. All the best, Karolina
Hi Matthew, On Sun, 2022-01-30 at 17:53 +0000, Matthew Wilcox wrote: > I think I found a deeper problem which is masked due to our maze of > header files: > > include/asm-generic/io.h:#ifndef virt_to_phys > include/asm-generic/io.h:#define virt_to_phys virt_to_phys > > so there's an assumption that <asm/io.h> defines virt_to_phys(). > You can see that in a number of architectures, eg: > > arch/alpha/include/asm/io.h:static inline unsigned long > virt_to_phys(volatile void *address) > arch/ia64/include/asm/io.h:#define virt_to_phys virt_to_phys > arch/mips/include/asm/io.h:#define virt_to_phys virt_to_phys > arch/nios2/include/asm/io.h:#define virt_to_phys(vaddr) \ > arch/parisc/include/asm/io.h:#define virt_to_phys(a) ((unsigned > long)__pa(a)) > arch/powerpc/include/asm/io.h:#define virt_to_phys virt_to_phys > arch/sh/include/asm/io.h:#define virt_to_phys(address) ((unsigned > long)(address)) > arch/x86/include/asm/io.h:#define virt_to_phys virt_to_phys > > That's clearly not the right place to define it. Two architectures > put it in asm/memory.h: > > arch/arm/include/asm/memory.h:#define virt_to_phys virt_to_phys > arch/arm64/include/asm/memory.h:#define virt_to_phys virt_to_phys > > then: > > arch/m68k/include/asm/virtconvert.h:#define virt_to_phys virt_to_phys > arch/sparc/include/asm/page_32.h:#define virt_to_phys __pa > arch/sparc/include/asm/page_64.h:#define virt_to_phys __pa Oh, that's a lot...thank you for checking it. The simulator has its own mm stubs, so it shouldn't be impacted by such a clean up, correct? > This needs to be properly sorted out, but I don't want to tell > Karolina that's now her job as a prerequisite for merging this > patchset; that would be unfair. Thanks. Also, I think I'm not be the best person to (efficiently) sort this out :) All the best, Karolina
On Sun, Jan 30, 2022 at 6:53 PM Matthew Wilcox <willy@infradead.org> wrote: > Thanks for doing the sweep, Mike. > > I think I found a deeper problem which is masked due to our maze of > header files: > > include/asm-generic/io.h:#ifndef virt_to_phys > include/asm-generic/io.h:#define virt_to_phys virt_to_phys > > so there's an assumption that <asm/io.h> defines virt_to_phys(). > You can see that in a number of architectures, eg: > > arch/alpha/include/asm/io.h:static inline unsigned long virt_to_phys(volatile void *address) > arch/ia64/include/asm/io.h:#define virt_to_phys virt_to_phys > arch/mips/include/asm/io.h:#define virt_to_phys virt_to_phys > arch/nios2/include/asm/io.h:#define virt_to_phys(vaddr) \ > arch/parisc/include/asm/io.h:#define virt_to_phys(a) ((unsigned long)__pa(a)) > arch/powerpc/include/asm/io.h:#define virt_to_phys virt_to_phys > arch/sh/include/asm/io.h:#define virt_to_phys(address) ((unsigned long)(address)) > arch/x86/include/asm/io.h:#define virt_to_phys virt_to_phys > > That's clearly not the right place to define it. Two architectures > put it in asm/memory.h: > > arch/arm/include/asm/memory.h:#define virt_to_phys virt_to_phys > arch/arm64/include/asm/memory.h:#define virt_to_phys virt_to_phys > > then: > > arch/m68k/include/asm/virtconvert.h:#define virt_to_phys virt_to_phys > arch/sparc/include/asm/page_32.h:#define virt_to_phys __pa > arch/sparc/include/asm/page_64.h:#define virt_to_phys __pa > > This needs to be properly sorted out, but I don't want to tell Karolina > that's now her job as a prerequisite for merging this patchset; that > would be unfair. > > Cc'ing Arnd. This is the kind of awful mess that he loves fixing ;-) Adding Ingo as well. I'm in the middle of getting his fast-headers tree to work well on a couple of other architectures, and the memory.h/page.h/io.h mess is one of the tricky bits in there, both in his series and in my follow-ups. What makes this bit even worse is that the architectures also not just inconsistent about where they put __va/__pa and virt_to_phys/phys_to_virt, they are also inconsistent about which of the two pairs is based on the other, so any way you touch it means you will break something, and changing it now will likely require a tricky rebase of Ingo's patches. Ingo, do you happen to have patches already that could be isolated from your series to address this? Maybe we can add the linux/mm_page_address.h header first and require that each architecture puts these macros into asm/page_address.h. We need to isolate these anyway, because the page addresses are used in a lot of places that don't need to include any of the remaining headers (page.h, mm.h, memory.h, io.h) that pull in hundreds more. Arnd
On Mon, Jan 31, 2022 at 02:30:32PM +0100, Arnd Bergmann wrote: > On Sun, Jan 30, 2022 at 6:53 PM Matthew Wilcox <willy@infradead.org> wrote: > > > Thanks for doing the sweep, Mike. > > > > I think I found a deeper problem which is masked due to our maze of > > header files: > > > > include/asm-generic/io.h:#ifndef virt_to_phys > > include/asm-generic/io.h:#define virt_to_phys virt_to_phys > > > > so there's an assumption that <asm/io.h> defines virt_to_phys(). > > You can see that in a number of architectures, eg: > > > > arch/alpha/include/asm/io.h:static inline unsigned long virt_to_phys(volatile void *address) > > arch/ia64/include/asm/io.h:#define virt_to_phys virt_to_phys > > arch/mips/include/asm/io.h:#define virt_to_phys virt_to_phys > > arch/nios2/include/asm/io.h:#define virt_to_phys(vaddr) \ > > arch/parisc/include/asm/io.h:#define virt_to_phys(a) ((unsigned long)__pa(a)) > > arch/powerpc/include/asm/io.h:#define virt_to_phys virt_to_phys > > arch/sh/include/asm/io.h:#define virt_to_phys(address) ((unsigned long)(address)) > > arch/x86/include/asm/io.h:#define virt_to_phys virt_to_phys > > > > That's clearly not the right place to define it. Two architectures > > put it in asm/memory.h: > > > > arch/arm/include/asm/memory.h:#define virt_to_phys virt_to_phys > > arch/arm64/include/asm/memory.h:#define virt_to_phys virt_to_phys > > > > then: > > > > arch/m68k/include/asm/virtconvert.h:#define virt_to_phys virt_to_phys > > arch/sparc/include/asm/page_32.h:#define virt_to_phys __pa > > arch/sparc/include/asm/page_64.h:#define virt_to_phys __pa > > > > This needs to be properly sorted out, but I don't want to tell Karolina > > that's now her job as a prerequisite for merging this patchset; that > > would be unfair. > > > > Cc'ing Arnd. This is the kind of awful mess that he loves fixing ;-) > > Adding Ingo as well. I'm in the middle of getting his fast-headers tree > to work well on a couple of other architectures, and the memory.h/page.h/io.h > mess is one of the tricky bits in there, both in his series and in my > follow-ups. > > What makes this bit even worse is that the architectures also not just > inconsistent about where they put __va/__pa and > virt_to_phys/phys_to_virt, they are also inconsistent about which of the > two pairs is based on the other, so any way you touch it means you > will break something, and changing it now will likely require a tricky > rebase of Ingo's patches. Hmm, whatever we'll do with these conversions, it will be tricky for Ingo's tree... > Ingo, do you happen to have patches already that could be isolated > from your series to address this? Maybe we can add the > linux/mm_page_address.h header first and require that each > architecture puts these macros into asm/page_address.h. > We need to isolate these anyway, because the page addresses > are used in a lot of places that don't need to include any of the > remaining headers (page.h, mm.h, memory.h, io.h) that pull in > hundreds more. I peeked at Ingo's tree and there is this: commit 3426911a3f83 (headers/deps: io/arch: Move the address translation APIs from <asm/io.h> to <asm/io_extra.h>) (https://git.kernel.org/pub/scm/linux/kernel/git/mingo/tip.git/commit/?id=3426911a3f833930d92f3aebe349f09a513375d9) It moves virt_to_phys and phys_to_virt on x86 to a new header. I actually liked m68k's name for a header with virt_to_phys/phys_to_virt definitions - virtconvert.h. As an experiment I pulled out address translations from arch/arm/include/memory.h to arch/arm/include/virtconvert.h, it wasn't that bad: arch/arm/include/asm/dma-mapping.h | 2 + arch/arm/include/asm/io.h | 1 + arch/arm/include/asm/memory.h | 244 ---------------------------------- arch/arm/include/asm/pgtable.h | 1 + arch/arm/include/asm/virtconvert.h | 264 +++++++++++++++++++++++++++++++++++++ arch/arm/kernel/psci_smp.c | 1 + 6 files changed, 269 insertions(+), 244 deletions(-) (https://git.kernel.org/rppt/linux/c/4c34ec16319fc85280aad89d7a74df845c1614fc) > Arnd
On Mon, Jan 31, 2022 at 4:18 PM Mike Rapoport <rppt@kernel.org> wrote: > On Mon, Jan 31, 2022 at 02:30:32PM +0100, Arnd Bergmann wrote: > > On Sun, Jan 30, 2022 at 6:53 PM Matthew Wilcox <willy@infradead.org> wrote: > I actually liked m68k's name for a header with virt_to_phys/phys_to_virt > definitions - virtconvert.h. > > As an experiment I pulled out address translations from > arch/arm/include/memory.h to arch/arm/include/virtconvert.h, it wasn't that > bad: > > arch/arm/include/asm/dma-mapping.h | 2 + > arch/arm/include/asm/io.h | 1 + > arch/arm/include/asm/memory.h | 244 ---------------------------------- > arch/arm/include/asm/pgtable.h | 1 + > arch/arm/include/asm/virtconvert.h | 264 +++++++++++++++++++++++++++++++++++++ > arch/arm/kernel/psci_smp.c | 1 + > 6 files changed, 269 insertions(+), 244 deletions(-) > > (https://git.kernel.org/rppt/linux/c/4c34ec16319fc85280aad89d7a74df845c1614fc) Right, that doesn't look too bad, especially since it seems you managed to avoid any further indirect inlcudes. Doing the same consistently for all architectures may end up a bit harder but would be a great help. Arnd
diff --git a/tools/include/linux/io.h b/tools/include/linux/io.h new file mode 100644 index 000000000000..e129871fe661 --- /dev/null +++ b/tools/include/linux/io.h @@ -0,0 +1,5 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _TOOLS_IO_H +#define _TOOLS_IO_H + +#endif
Add a dummy io.h header. Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com> --- tools/include/linux/io.h | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 tools/include/linux/io.h