| Message ID | 20220820003125.353570-3-bhe@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: ioremap: Convert architectures to take GENERIC_IOREMAP way | expand |
> -void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot); > +void __iomem * > +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val); It seems a bit odd to do this in two steps vs just doing the entire change in the first patch. Any good reason for that?
Le 20/08/2022 à 02:31, Baoquan He a écrit : > On some architectures, the physical address need be fixed up before > doing mapping, e.g, parisc. And on architectures, e.g arc, the > parameter 'prot' passed into ioremap_prot() need be adjusted too. > > In oder to convert them to take GENERIC_IOREMAP method, we need wrap > the address fixing up code and page prot adjusting code into arch_ioremap() > and pass the new address and 'prot' out for ioremap_prot() handling. Is it really the best approach ? Wouldn't it be better to have helpers to do that, those helpers being called by the ioremap_prot(), instead of doing it inside the arch_ioremap() function ? > > This is a preparation patch, no functionality change. Could this be squashed into previous patch ? > > Signed-off-by: Baoquan He <bhe@redhat.com> > --- > arch/arm64/include/asm/io.h | 3 ++- > arch/arm64/mm/ioremap.c | 5 +++-- > include/asm-generic/io.h | 4 ++-- > mm/ioremap.c | 2 +- > 4 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h > index dd7e1c2dc86c..6a5578ddbbf6 100644 > --- a/arch/arm64/include/asm/io.h > +++ b/arch/arm64/include/asm/io.h > @@ -139,7 +139,8 @@ extern void __memset_io(volatile void __iomem *, int, size_t); > * I/O memory mapping functions. > */ > > -void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot); > +void __iomem * > +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val); > #define arch_ioremap arch_ioremap > > #define _PAGE_IOREMAP PROT_DEVICE_nGnRE > diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c > index b0f4cea86f0e..ef75ffef4dbc 100644 > --- a/arch/arm64/mm/ioremap.c > +++ b/arch/arm64/mm/ioremap.c > @@ -3,9 +3,10 @@ > #include <linux/mm.h> > #include <linux/io.h> > > -void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot) > +void __iomem * > +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val) > { > - unsigned long last_addr, offset; > + unsigned long last_addr, offset, phys_addr = *paddr; > > offset = phys_addr & (~PAGE_MASK); > phys_addr -= offset; > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > index 7b6bfb62ef80..fb9bda2be8ed 100644 > --- a/include/asm-generic/io.h > +++ b/include/asm-generic/io.h > @@ -1059,8 +1059,8 @@ static inline void iounmap(volatile void __iomem *addr) > */ > #ifndef arch_ioremap > #define arch_ioremap arch_ioremap > -static inline void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, > - unsigned long prot) > +static inline void __iomem *arch_ioremap(phys_addr_t *paddr, size_t size, > + unsigned long *prot_val) > { > return NULL; > } > diff --git a/mm/ioremap.c b/mm/ioremap.c > index 99fde69becc7..7914b5cf5b78 100644 > --- a/mm/ioremap.c > +++ b/mm/ioremap.c > @@ -19,7 +19,7 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, > struct vm_struct *area; > void __iomem *ioaddr; > > - ioaddr = arch_ioremap(phys_addr, size, prot); > + ioaddr = arch_ioremap(&phys_addr, size, &prot); > if (IS_ERR(ioaddr)) > return NULL; > else if (ioaddr)
On 08/20/22 at 11:54pm, Christoph Hellwig wrote: > > -void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot); > > +void __iomem * > > +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val); > > It seems a bit odd to do this in two steps vs just doing the entire > change in the first patch. Any good reason for that? I will merge patch 1~2 or 1~3 into patch 1. Earlier, I wrote the reason as below paragraph in cover letter of v1 post. Now it seems not so hard to tell and understand. ==== For patch 1~3, I don't merge them because I made them in different rounds of changing. And splitting them makes me easily describe the intention and make review easier. I can merge them after v1 reviewing if anyone thinks they should be merged. ====
On 08/22/22 at 06:30am, Christophe Leroy wrote: > > > Le 20/08/2022 à 02:31, Baoquan He a écrit : > > On some architectures, the physical address need be fixed up before > > doing mapping, e.g, parisc. And on architectures, e.g arc, the > > parameter 'prot' passed into ioremap_prot() need be adjusted too. > > > > In oder to convert them to take GENERIC_IOREMAP method, we need wrap > > the address fixing up code and page prot adjusting code into arch_ioremap() > > and pass the new address and 'prot' out for ioremap_prot() handling. > > Is it really the best approach ? Wouldn't it be better to have helpers > to do that, those helpers being called by the ioremap_prot(), instead of > doing it inside the arch_ioremap() function ? This is suggested too by Alexander during his v1 reviewing. I tried, but feel the current way taken in this patchset is better. Because not all architecutres need the address fix up, only parisc, and only few need adjust the 'prot'. Introducing other helpers seems too much, that only increases the complexity of of ioremap() and the generic GENERIC_IOREMAP method for people to understand and take. > > > > > This is a preparation patch, no functionality change. > > Could this be squashed into previous patch ? Yep, will do. Thanks.
Le 23/08/2022 à 03:19, Baoquan He a écrit : > On 08/22/22 at 06:30am, Christophe Leroy wrote: >> >> >> Le 20/08/2022 à 02:31, Baoquan He a écrit : >>> On some architectures, the physical address need be fixed up before >>> doing mapping, e.g, parisc. And on architectures, e.g arc, the >>> parameter 'prot' passed into ioremap_prot() need be adjusted too. >>> >>> In oder to convert them to take GENERIC_IOREMAP method, we need wrap >>> the address fixing up code and page prot adjusting code into arch_ioremap() >>> and pass the new address and 'prot' out for ioremap_prot() handling. >> >> Is it really the best approach ? Wouldn't it be better to have helpers >> to do that, those helpers being called by the ioremap_prot(), instead of >> doing it inside the arch_ioremap() function ? > > This is suggested too by Alexander during his v1 reviewing. I tried, but > feel the current way taken in this patchset is better. Because not all > architecutres need the address fix up, only parisc, and only few need > adjust the 'prot'. Introducing other helpers seems too much, that only > increases the complexity of of ioremap() and the generic GENERIC_IOREMAP > method for people to understand and take. I can't understand. Why is it difficult to do something like: #ifndef ioremap_adjust_prot static inline unsigned long ioremap_adjust_prot(unsigned long flags) { return flags; } #endif Then for arc you do static inline unsigned long ioremap_adjust_prot(unsigned long flags) { return pgprot_val(pgprot_noncached(__pgprot(flags))); } #define ioremap_adjust_prot ioremap_adjust_prot By the way, could be a good opportunity to change ioremap_prot() flags type from unsigned long to pgprot_t > >> >>> >>> This is a preparation patch, no functionality change. >> >> Could this be squashed into previous patch ? > > Yep, will do. Thanks. >
On 08/23/22 at 05:33am, Christophe Leroy wrote: > > > Le 23/08/2022 à 03:19, Baoquan He a écrit : > > On 08/22/22 at 06:30am, Christophe Leroy wrote: > >> > >> > >> Le 20/08/2022 à 02:31, Baoquan He a écrit : > >>> On some architectures, the physical address need be fixed up before > >>> doing mapping, e.g, parisc. And on architectures, e.g arc, the > >>> parameter 'prot' passed into ioremap_prot() need be adjusted too. > >>> > >>> In oder to convert them to take GENERIC_IOREMAP method, we need wrap > >>> the address fixing up code and page prot adjusting code into arch_ioremap() > >>> and pass the new address and 'prot' out for ioremap_prot() handling. > >> > >> Is it really the best approach ? Wouldn't it be better to have helpers > >> to do that, those helpers being called by the ioremap_prot(), instead of > >> doing it inside the arch_ioremap() function ? > > > > This is suggested too by Alexander during his v1 reviewing. I tried, but > > feel the current way taken in this patchset is better. Because not all > > architecutres need the address fix up, only parisc, and only few need > > adjust the 'prot'. Introducing other helpers seems too much, that only > > increases the complexity of of ioremap() and the generic GENERIC_IOREMAP > > method for people to understand and take. > > I can't understand. Why is it difficult to do something like: > > #ifndef ioremap_adjust_prot > static inline unsigned long ioremap_adjust_prot(unsigned long flags) > { > return flags; > } > #endif > > Then for arc you do > > static inline unsigned long ioremap_adjust_prot(unsigned long flags) > { > return pgprot_val(pgprot_noncached(__pgprot(flags))); > } > #define ioremap_adjust_prot ioremap_adjust_prot My thinking is we have four things to do in the added hookers. 1) check if we should do ioremap on ARCHes. If not, return NULL from ioremap_prot(); 2) handling the mapping io address specifically on ARCHes, e.g arc, ia64, s390; 3) the original physical address passed into ioremap_prot() need be fixed up, e.g arc; 4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc and xtensa. With Kefeng's patches, the case 1) is handled with introduced ioremap_allowed()/iounmap_allowed(). In this patchset, what I do is rename the hooks as arch_ioremap() and arch_iounmap(), then put case 1), 2), 3), 4) handling into arch_ioremap(). Adding helpers to cover each case is not difficult from my side. I worry that as time goes by, those several hooks my cause issue, e.g if a new adjustment need be done, should we introduce a new helper or make do with the existed hook; how When I investigated this, one arch_ioremap() looks not complicated since not all ARCHes need cover all above 4 cases. That's why I finally choose one hook. I am open to new idea, please let me know if we should change it to introduce several different helpers. > > > By the way, could be a good opportunity to change ioremap_prot() flags > type from unsigned long to pgprot_t Tend to agree, I will give it a shot.
Le 23/08/2022 à 14:32, Baoquan He a écrit : > On 08/23/22 at 05:33am, Christophe Leroy wrote: >> >> >> Le 23/08/2022 à 03:19, Baoquan He a écrit : >>> On 08/22/22 at 06:30am, Christophe Leroy wrote: >>>> >>>> >>>> Le 20/08/2022 à 02:31, Baoquan He a écrit : >>>>> On some architectures, the physical address need be fixed up before >>>>> doing mapping, e.g, parisc. And on architectures, e.g arc, the >>>>> parameter 'prot' passed into ioremap_prot() need be adjusted too. >>>>> >>>>> In oder to convert them to take GENERIC_IOREMAP method, we need wrap >>>>> the address fixing up code and page prot adjusting code into arch_ioremap() >>>>> and pass the new address and 'prot' out for ioremap_prot() handling. >>>> >>>> Is it really the best approach ? Wouldn't it be better to have helpers >>>> to do that, those helpers being called by the ioremap_prot(), instead of >>>> doing it inside the arch_ioremap() function ? >>> >>> This is suggested too by Alexander during his v1 reviewing. I tried, but >>> feel the current way taken in this patchset is better. Because not all >>> architecutres need the address fix up, only parisc, and only few need >>> adjust the 'prot'. Introducing other helpers seems too much, that only >>> increases the complexity of of ioremap() and the generic GENERIC_IOREMAP >>> method for people to understand and take. >> >> I can't understand. Why is it difficult to do something like: >> >> #ifndef ioremap_adjust_prot >> static inline unsigned long ioremap_adjust_prot(unsigned long flags) >> { >> return flags; >> } >> #endif >> >> Then for arc you do >> >> static inline unsigned long ioremap_adjust_prot(unsigned long flags) >> { >> return pgprot_val(pgprot_noncached(__pgprot(flags))); >> } >> #define ioremap_adjust_prot ioremap_adjust_prot > > My thinking is we have four things to do in the added hookers. > 1) check if we should do ioremap on ARCHes. If not, return NULL from > ioremap_prot(); > 2) handling the mapping io address specifically on ARCHes, e.g arc, > ia64, s390; > 3) the original physical address passed into ioremap_prot() need be > fixed up, e.g arc; > 4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc > and xtensa. > > With Kefeng's patches, the case 1) is handled with introduced > ioremap_allowed()/iounmap_allowed(). In this patchset, what I do is > rename the hooks as arch_ioremap() and arch_iounmap(), then put case 1), > 2), 3), 4) handling into arch_ioremap(). Adding helpers to cover each > case is not difficult from my side. I worry that as time goes by, those > several hooks my cause issue, e.g if a new adjustment need be done, > should we introduce a new helper or make do with the existed hook; how > > When I investigated this, one arch_ioremap() looks not complicated > since not all ARCHes need cover all above 4 cases. That's why I finally > choose one hook. I am open to new idea, please let me know if we should > change it to introduce several different helpers. > A new idea that would have my preference would be to do just like we did with arch_get_unmapped_area(). Look at https://elixir.bootlin.com/linux/v6.0-rc1/source/arch/powerpc/mm/book3s64/slice.c#L638 and https://elixir.bootlin.com/linux/v6.0-rc1/source/mm/mmap.c#L2131 Instead of having the generic that calls the arch specific, make it the other way round, have the arch specific call the generic after doing its specialties. >> >> >> By the way, could be a good opportunity to change ioremap_prot() flags >> type from unsigned long to pgprot_t > > Tend to agree, I will give it a shot. >
On 08/23/22 at 07:03pm, Christophe Leroy wrote: > > > Le 23/08/2022 à 14:32, Baoquan He a écrit : > > On 08/23/22 at 05:33am, Christophe Leroy wrote: > >> > >> > >> Le 23/08/2022 à 03:19, Baoquan He a écrit : > >>> On 08/22/22 at 06:30am, Christophe Leroy wrote: > >>>> > >>>> > >>>> Le 20/08/2022 à 02:31, Baoquan He a écrit : > >>>>> On some architectures, the physical address need be fixed up before > >>>>> doing mapping, e.g, parisc. And on architectures, e.g arc, the > >>>>> parameter 'prot' passed into ioremap_prot() need be adjusted too. > >>>>> > >>>>> In oder to convert them to take GENERIC_IOREMAP method, we need wrap > >>>>> the address fixing up code and page prot adjusting code into arch_ioremap() > >>>>> and pass the new address and 'prot' out for ioremap_prot() handling. > >>>> > >>>> Is it really the best approach ? Wouldn't it be better to have helpers > >>>> to do that, those helpers being called by the ioremap_prot(), instead of > >>>> doing it inside the arch_ioremap() function ? > >>> > >>> This is suggested too by Alexander during his v1 reviewing. I tried, but > >>> feel the current way taken in this patchset is better. Because not all > >>> architecutres need the address fix up, only parisc, and only few need > >>> adjust the 'prot'. Introducing other helpers seems too much, that only > >>> increases the complexity of of ioremap() and the generic GENERIC_IOREMAP > >>> method for people to understand and take. > >> > >> I can't understand. Why is it difficult to do something like: > >> > >> #ifndef ioremap_adjust_prot > >> static inline unsigned long ioremap_adjust_prot(unsigned long flags) > >> { > >> return flags; > >> } > >> #endif > >> > >> Then for arc you do > >> > >> static inline unsigned long ioremap_adjust_prot(unsigned long flags) > >> { > >> return pgprot_val(pgprot_noncached(__pgprot(flags))); > >> } > >> #define ioremap_adjust_prot ioremap_adjust_prot > > > > My thinking is we have four things to do in the added hookers. > > 1) check if we should do ioremap on ARCHes. If not, return NULL from > > ioremap_prot(); > > 2) handling the mapping io address specifically on ARCHes, e.g arc, > > ia64, s390; > > 3) the original physical address passed into ioremap_prot() need be > > fixed up, e.g arc; > > 4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc > > and xtensa. > > > > With Kefeng's patches, the case 1) is handled with introduced > > ioremap_allowed()/iounmap_allowed(). In this patchset, what I do is > > rename the hooks as arch_ioremap() and arch_iounmap(), then put case 1), > > 2), 3), 4) handling into arch_ioremap(). Adding helpers to cover each > > case is not difficult from my side. I worry that as time goes by, those > > several hooks my cause issue, e.g if a new adjustment need be done, > > should we introduce a new helper or make do with the existed hook; how > > > > When I investigated this, one arch_ioremap() looks not complicated > > since not all ARCHes need cover all above 4 cases. That's why I finally > > choose one hook. I am open to new idea, please let me know if we should > > change it to introduce several different helpers. > > > > A new idea that would have my preference would be to do just like we did > with arch_get_unmapped_area(). Look at > https://elixir.bootlin.com/linux/v6.0-rc1/source/arch/powerpc/mm/book3s64/slice.c#L638 > and https://elixir.bootlin.com/linux/v6.0-rc1/source/mm/mmap.c#L2131 > > Instead of having the generic that calls the arch specific, make it the > other way round, have the arch specific call the generic after doing its > specialties. This sounds good. I made a draft patch to change code in generic code part, just showing what it looks like. Both arch_ioremap() way and the arch sepcific call the generic way look good to me. Just it will take less effort for me to continue the arch_ioremap() way. I would like to hear Christoph's opinion since he introduced the GENERIC_IOREMAP method and suggested the earlier arch_ioremap() way and change in this patchset. diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h index 68a8117b30fa..4bc3e18c475f 100644 --- a/include/asm-generic/io.h +++ b/include/asm-generic/io.h @@ -1047,35 +1047,18 @@ static inline void iounmap(volatile void __iomem *addr) #elif defined(CONFIG_GENERIC_IOREMAP) #include <linux/pgtable.h> -/* - * Arch code can implement the following two hooks when using GENERIC_IOREMAP - * arch_ioremap() return a bool, - * - IS_ERR means return an error - * - NULL means continue to remap - * - a non-NULL, non-IS_ERR pointer is returned directly - * arch_iounmap() return a bool, - * - 0 means continue to vunmap - * - error code means skip vunmap and return directly - */ -#ifndef arch_ioremap -#define arch_ioremap arch_ioremap -static inline void __iomem *arch_ioremap(phys_addr_t *paddr, size_t size, - unsigned long *prot_val) -{ - return NULL; -} -#endif +void __iomem * +generic_ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long prot); -#ifndef arch_iounmap -#define arch_iounmap arch_iounmap -static inline int arch_iounmap(void __iomem *addr) +#ifndef ioremap_prot +#define ioremap_prot ioremap_prot +void __iomem * +ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long prot); { - return 0; + return generic_ioremap_prot(phys_addr, size, prot); } #endif -void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, - unsigned long prot); void iounmap(volatile void __iomem *addr); #ifndef ioremap diff --git a/mm/ioremap.c b/mm/ioremap.c index 7914b5cf5b78..87d51003dee6 100644 --- a/mm/ioremap.c +++ b/mm/ioremap.c @@ -11,8 +11,8 @@ #include <linux/io.h> #include <linux/export.h> -void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, - unsigned long prot) +void __iomem * +generic_ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long prot) { unsigned long offset, vaddr; phys_addr_t last_addr;
Hi Christophe, On 08/28/22 at 07:10pm, Baoquan He wrote: > On 08/23/22 at 07:03pm, Christophe Leroy wrote: ...... > > >>>> Is it really the best approach ? Wouldn't it be better to have helpers > > >>>> to do that, those helpers being called by the ioremap_prot(), instead of > > >>>> doing it inside the arch_ioremap() function ? > > >>> > > >>> This is suggested too by Alexander during his v1 reviewing. I tried, but > > >>> feel the current way taken in this patchset is better. Because not all > > >>> architecutres need the address fix up, only parisc, and only few need > > >>> adjust the 'prot'. Introducing other helpers seems too much, that only > > >>> increases the complexity of of ioremap() and the generic GENERIC_IOREMAP > > >>> method for people to understand and take. > > >> > > >> I can't understand. Why is it difficult to do something like: > > >> > > >> #ifndef ioremap_adjust_prot > > >> static inline unsigned long ioremap_adjust_prot(unsigned long flags) > > >> { > > >> return flags; > > >> } > > >> #endif > > >> > > >> Then for arc you do > > >> > > >> static inline unsigned long ioremap_adjust_prot(unsigned long flags) > > >> { > > >> return pgprot_val(pgprot_noncached(__pgprot(flags))); > > >> } > > >> #define ioremap_adjust_prot ioremap_adjust_prot > > > > > > My thinking is we have four things to do in the added hookers. > > > 1) check if we should do ioremap on ARCHes. If not, return NULL from > > > ioremap_prot(); > > > 2) handling the mapping io address specifically on ARCHes, e.g arc, > > > ia64, s390; > > > 3) the original physical address passed into ioremap_prot() need be > > > fixed up, e.g arc; > > > 4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc > > > and xtensa. > > > > > > With Kefeng's patches, the case 1) is handled with introduced > > > ioremap_allowed()/iounmap_allowed(). In this patchset, what I do is > > > rename the hooks as arch_ioremap() and arch_iounmap(), then put case 1), > > > 2), 3), 4) handling into arch_ioremap(). Adding helpers to cover each > > > case is not difficult from my side. I worry that as time goes by, those > > > several hooks my cause issue, e.g if a new adjustment need be done, > > > should we introduce a new helper or make do with the existed hook; how > > > > > > When I investigated this, one arch_ioremap() looks not complicated > > > since not all ARCHes need cover all above 4 cases. That's why I finally > > > choose one hook. I am open to new idea, please let me know if we should > > > change it to introduce several different helpers. > > > > > > > A new idea that would have my preference would be to do just like we did > > with arch_get_unmapped_area(). Look at > > https://elixir.bootlin.com/linux/v6.0-rc1/source/arch/powerpc/mm/book3s64/slice.c#L638 > > and https://elixir.bootlin.com/linux/v6.0-rc1/source/mm/mmap.c#L2131 > > > > Instead of having the generic that calls the arch specific, make it the > > other way round, have the arch specific call the generic after doing its > > specialties. > > This sounds good. I made a draft patch to change code in generic code > part, just showing what it looks like. > > Both arch_ioremap() way and the arch sepcific call the generic way look > good to me. Just it will take less effort for me to continue the > arch_ioremap() way. I would like to hear Christoph's opinion since he > introduced the GENERIC_IOREMAP method and suggested the earlier > arch_ioremap() way and change in this patchset. I will make another round change and post. Since Christoph doesn't reply, I would like to continue with the existing arch_ioremap/arch_iounmap() hooks way if you don't have strong opinion on the new idea to reintroduce ioremap(). > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > index 68a8117b30fa..4bc3e18c475f 100644 > --- a/include/asm-generic/io.h > +++ b/include/asm-generic/io.h > @@ -1047,35 +1047,18 @@ static inline void iounmap(volatile void __iomem *addr) > #elif defined(CONFIG_GENERIC_IOREMAP) > #include <linux/pgtable.h> > > -/* > - * Arch code can implement the following two hooks when using GENERIC_IOREMAP > - * arch_ioremap() return a bool, > - * - IS_ERR means return an error > - * - NULL means continue to remap > - * - a non-NULL, non-IS_ERR pointer is returned directly > - * arch_iounmap() return a bool, > - * - 0 means continue to vunmap > - * - error code means skip vunmap and return directly > - */ > -#ifndef arch_ioremap > -#define arch_ioremap arch_ioremap > -static inline void __iomem *arch_ioremap(phys_addr_t *paddr, size_t size, > - unsigned long *prot_val) > -{ > - return NULL; > -} > -#endif > +void __iomem * > +generic_ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long prot); > > -#ifndef arch_iounmap > -#define arch_iounmap arch_iounmap > -static inline int arch_iounmap(void __iomem *addr) > +#ifndef ioremap_prot > +#define ioremap_prot ioremap_prot > +void __iomem * > +ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long prot); > { > - return 0; > + return generic_ioremap_prot(phys_addr, size, prot); > } > #endif > > -void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, > - unsigned long prot); > void iounmap(volatile void __iomem *addr); > > #ifndef ioremap > diff --git a/mm/ioremap.c b/mm/ioremap.c > index 7914b5cf5b78..87d51003dee6 100644 > --- a/mm/ioremap.c > +++ b/mm/ioremap.c > @@ -11,8 +11,8 @@ > #include <linux/io.h> > #include <linux/export.h> > > -void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, > - unsigned long prot) > +void __iomem * > +generic_ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long prot) > { > unsigned long offset, vaddr; > phys_addr_t last_addr; >
Hi Baoquan, Le 12/09/2022 à 04:55, Baoquan He a écrit : > Hi Christophe, > > On 08/28/22 at 07:10pm, Baoquan He wrote: >> On 08/23/22 at 07:03pm, Christophe Leroy wrote: > ...... >>>>>>> Is it really the best approach ? Wouldn't it be better to have helpers >>>>>>> to do that, those helpers being called by the ioremap_prot(), instead of >>>>>>> doing it inside the arch_ioremap() function ? >>>>>> >>>>>> This is suggested too by Alexander during his v1 reviewing. I tried, but >>>>>> feel the current way taken in this patchset is better. Because not all >>>>>> architecutres need the address fix up, only parisc, and only few need >>>>>> adjust the 'prot'. Introducing other helpers seems too much, that only >>>>>> increases the complexity of of ioremap() and the generic GENERIC_IOREMAP >>>>>> method for people to understand and take. >>>>> >>>>> I can't understand. Why is it difficult to do something like: >>>>> >>>>> #ifndef ioremap_adjust_prot >>>>> static inline unsigned long ioremap_adjust_prot(unsigned long flags) >>>>> { >>>>> return flags; >>>>> } >>>>> #endif >>>>> >>>>> Then for arc you do >>>>> >>>>> static inline unsigned long ioremap_adjust_prot(unsigned long flags) >>>>> { >>>>> return pgprot_val(pgprot_noncached(__pgprot(flags))); >>>>> } >>>>> #define ioremap_adjust_prot ioremap_adjust_prot >>>> >>>> My thinking is we have four things to do in the added hookers. >>>> 1) check if we should do ioremap on ARCHes. If not, return NULL from >>>> ioremap_prot(); >>>> 2) handling the mapping io address specifically on ARCHes, e.g arc, >>>> ia64, s390; >>>> 3) the original physical address passed into ioremap_prot() need be >>>> fixed up, e.g arc; >>>> 4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc >>>> and xtensa. >>>> >>>> With Kefeng's patches, the case 1) is handled with introduced >>>> ioremap_allowed()/iounmap_allowed(). In this patchset, what I do is >>>> rename the hooks as arch_ioremap() and arch_iounmap(), then put case 1), >>>> 2), 3), 4) handling into arch_ioremap(). Adding helpers to cover each >>>> case is not difficult from my side. I worry that as time goes by, those >>>> several hooks my cause issue, e.g if a new adjustment need be done, >>>> should we introduce a new helper or make do with the existed hook; how >>>> >>>> When I investigated this, one arch_ioremap() looks not complicated >>>> since not all ARCHes need cover all above 4 cases. That's why I finally >>>> choose one hook. I am open to new idea, please let me know if we should >>>> change it to introduce several different helpers. >>>> >>> >>> A new idea that would have my preference would be to do just like we did >>> with arch_get_unmapped_area(). Look at >>> https://elixir.bootlin.com/linux/v6.0-rc1/source/arch/powerpc/mm/book3s64/slice.c#L638 >>> and https://elixir.bootlin.com/linux/v6.0-rc1/source/mm/mmap.c#L2131 >>> >>> Instead of having the generic that calls the arch specific, make it the >>> other way round, have the arch specific call the generic after doing its >>> specialties. >> >> This sounds good. I made a draft patch to change code in generic code >> part, just showing what it looks like. >> >> Both arch_ioremap() way and the arch sepcific call the generic way look >> good to me. Just it will take less effort for me to continue the >> arch_ioremap() way. I would like to hear Christoph's opinion since he >> introduced the GENERIC_IOREMAP method and suggested the earlier >> arch_ioremap() way and change in this patchset. > > I will make another round change and post. Since Christoph doesn't > reply, I would like to continue with the existing > arch_ioremap/arch_iounmap() hooks way if you don't have strong opinion > on the new idea to reintroduce ioremap(). > I still dislike you approach with the architectures modifying local vars by reference, and as you said earlier I'm not the only one : "This is suggested too by Alexander during his v1 reviewing". So I'd really prefer you to reconsider your approach and avoid passign pointers to local vars to architecture helpers. Thanks Christophe
On 09/12/22 at 07:10am, Christophe Leroy wrote: > Hi Baoquan, > > Le 12/09/2022 à 04:55, Baoquan He a écrit : > > Hi Christophe, > > > > On 08/28/22 at 07:10pm, Baoquan He wrote: > >> On 08/23/22 at 07:03pm, Christophe Leroy wrote: > > ...... > >>>>>>> Is it really the best approach ? Wouldn't it be better to have helpers > >>>>>>> to do that, those helpers being called by the ioremap_prot(), instead of > >>>>>>> doing it inside the arch_ioremap() function ? > >>>>>> > >>>>>> This is suggested too by Alexander during his v1 reviewing. I tried, but > >>>>>> feel the current way taken in this patchset is better. Because not all > >>>>>> architecutres need the address fix up, only parisc, and only few need > >>>>>> adjust the 'prot'. Introducing other helpers seems too much, that only > >>>>>> increases the complexity of of ioremap() and the generic GENERIC_IOREMAP > >>>>>> method for people to understand and take. > >>>>> > >>>>> I can't understand. Why is it difficult to do something like: > >>>>> > >>>>> #ifndef ioremap_adjust_prot > >>>>> static inline unsigned long ioremap_adjust_prot(unsigned long flags) > >>>>> { > >>>>> return flags; > >>>>> } > >>>>> #endif > >>>>> > >>>>> Then for arc you do > >>>>> > >>>>> static inline unsigned long ioremap_adjust_prot(unsigned long flags) > >>>>> { > >>>>> return pgprot_val(pgprot_noncached(__pgprot(flags))); > >>>>> } > >>>>> #define ioremap_adjust_prot ioremap_adjust_prot > >>>> > >>>> My thinking is we have four things to do in the added hookers. > >>>> 1) check if we should do ioremap on ARCHes. If not, return NULL from > >>>> ioremap_prot(); > >>>> 2) handling the mapping io address specifically on ARCHes, e.g arc, > >>>> ia64, s390; > >>>> 3) the original physical address passed into ioremap_prot() need be > >>>> fixed up, e.g arc; > >>>> 4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc > >>>> and xtensa. > >>>> > >>>> With Kefeng's patches, the case 1) is handled with introduced > >>>> ioremap_allowed()/iounmap_allowed(). In this patchset, what I do is > >>>> rename the hooks as arch_ioremap() and arch_iounmap(), then put case 1), > >>>> 2), 3), 4) handling into arch_ioremap(). Adding helpers to cover each > >>>> case is not difficult from my side. I worry that as time goes by, those > >>>> several hooks my cause issue, e.g if a new adjustment need be done, > >>>> should we introduce a new helper or make do with the existed hook; how > >>>> > >>>> When I investigated this, one arch_ioremap() looks not complicated > >>>> since not all ARCHes need cover all above 4 cases. That's why I finally > >>>> choose one hook. I am open to new idea, please let me know if we should > >>>> change it to introduce several different helpers. > >>>> > >>> > >>> A new idea that would have my preference would be to do just like we did > >>> with arch_get_unmapped_area(). Look at > >>> https://elixir.bootlin.com/linux/v6.0-rc1/source/arch/powerpc/mm/book3s64/slice.c#L638 > >>> and https://elixir.bootlin.com/linux/v6.0-rc1/source/mm/mmap.c#L2131 > >>> > >>> Instead of having the generic that calls the arch specific, make it the > >>> other way round, have the arch specific call the generic after doing its > >>> specialties. > >> > >> This sounds good. I made a draft patch to change code in generic code > >> part, just showing what it looks like. > >> > >> Both arch_ioremap() way and the arch sepcific call the generic way look > >> good to me. Just it will take less effort for me to continue the > >> arch_ioremap() way. I would like to hear Christoph's opinion since he > >> introduced the GENERIC_IOREMAP method and suggested the earlier > >> arch_ioremap() way and change in this patchset. > > > > I will make another round change and post. Since Christoph doesn't > > reply, I would like to continue with the existing > > arch_ioremap/arch_iounmap() hooks way if you don't have strong opinion > > on the new idea to reintroduce ioremap(). > > > > I still dislike you approach with the architectures modifying local vars > by reference, and as you said earlier I'm not the only one : "This is > suggested too by Alexander during his v1 reviewing". Alexander suggested several helpers, as I have explained earlier, that will cause at least four helpers currently. And could be more later if new change is introduced. And the address fixup and prot modifcation are related in few architecutures. Adding all of them is is not so necessary. > > So I'd really prefer you to reconsider your approach and avoid passign > pointers to local vars to architecture helpers. If only passing pointers to local vars is disliked, I can explain why I did so. Let me take arch_ioremap() of a64 as example. I can derefence pointer in arch_ioremap() to avoid assigning pointers to local vars. Please see below two version for comparing, and please tell which one is better. To me, assigning pointers to local vars make code simple and clean, honestly. ****** The version in which pointers assigned to local vars looks like below. The old phys_addr is passed in, the assigning can decrease changed line. diff --git a/arch/ia64/mm/ioremap.c b/arch/ia64/mm/ioremap.c index 55fd3eb753ff..8a085fc660e3 100644 --- a/arch/ia64/mm/ioremap.c +++ b/arch/ia64/mm/ioremap.c @@ -30,15 +30,12 @@ early_ioremap (unsigned long phys_addr, unsigned long size) } void __iomem * -ioremap (unsigned long phys_addr, unsigned long size) +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val) { - void __iomem *addr; - struct vm_struct *area; - unsigned long offset; - pgprot_t prot; - u64 attr; + phys_addr_t phys_addr = *paddr; unsigned long gran_base, gran_size; unsigned long page_base; + u64 attr; /* * For things in kern_memmap, we must use the same attribute @@ -69,35 +66,18 @@ ioremap (unsigned long phys_addr, unsigned long size) page_base = phys_addr & PAGE_MASK; size = PAGE_ALIGN(phys_addr + size) - page_base; if (efi_mem_attribute(page_base, size) & EFI_MEMORY_WB) { - prot = PAGE_KERNEL; - - /* - * Mappings have to be page-aligned - */ - offset = phys_addr & ~PAGE_MASK; - phys_addr &= PAGE_MASK; - - /* - * Ok, go for it.. - */ - area = get_vm_area(size, VM_IOREMAP); - if (!area) - return NULL; - - area->phys_addr = phys_addr; - addr = (void __iomem *) area->addr; - if (ioremap_page_range((unsigned long) addr, - (unsigned long) addr + size, phys_addr, prot)) { - vunmap((void __force *) addr); - return NULL; - } - - return (void __iomem *) (offset + (char __iomem *)addr); + return NULL; } return __ioremap_uc(phys_addr); } -EXPORT_SYMBOL(ioremap); + +int arch_iounmap(void __iomem *addr) +{ + if (REGION_NUMBER(addr) != RGN_GATE) + return -EINVAL; + return 0; +} void __iomem * ioremap_uc(unsigned long phys_addr, unsigned long size) ********* The version in which pointer is dereferenced directly in place. Then more lines of code are involved. And some pointer derefencing takes place in macro, so bracket is needed. diff --git a/arch/ia64/mm/ioremap.c b/arch/ia64/mm/ioremap.c index 55fd3eb753ff..e1b991dc2347 100644 --- a/arch/ia64/mm/ioremap.c +++ b/arch/ia64/mm/ioremap.c @@ -30,35 +30,32 @@ early_ioremap (unsigned long phys_addr, unsigned long size) } void __iomem * -ioremap (unsigned long phys_addr, unsigned long size) +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val) { - void __iomem *addr; - struct vm_struct *area; - unsigned long offset; - pgprot_t prot; - u64 attr; + phys_addr_t phys_addr = *paddr; unsigned long gran_base, gran_size; unsigned long page_base; + u64 attr; /* * For things in kern_memmap, we must use the same attribute * as the rest of the kernel. For more details, see * Documentation/ia64/aliasing.rst. */ - attr = kern_mem_attribute(phys_addr, size); + attr = kern_mem_attribute(*paddr, size); if (attr & EFI_MEMORY_WB) - return (void __iomem *) phys_to_virt(phys_addr); + return (void __iomem *) phys_to_virt(*paddr); else if (attr & EFI_MEMORY_UC) - return __ioremap_uc(phys_addr); + return __ioremap_uc(*paddr); /* * Some chipsets don't support UC access to memory. If * WB is supported for the whole granule, we prefer that. */ - gran_base = GRANULEROUNDDOWN(phys_addr); - gran_size = GRANULEROUNDUP(phys_addr + size) - gran_base; + gran_base = GRANULEROUNDDOWN(*paddr); + gran_size = GRANULEROUNDUP(*paddr + size) - gran_base; if (efi_mem_attribute(gran_base, gran_size) & EFI_MEMORY_WB) - return (void __iomem *) phys_to_virt(phys_addr); + return (void __iomem *) phys_to_virt(*paddr); /* * WB is not supported for the whole granule, so we can't use @@ -66,38 +63,21 @@ ioremap (unsigned long phys_addr, unsigned long size) * area with kernel page table mappings, we can use those * instead. */ - page_base = phys_addr & PAGE_MASK; - size = PAGE_ALIGN(phys_addr + size) - page_base; + page_base = (*paddr) & PAGE_MASK; + size = PAGE_ALIGN(*paddr + size) - page_base; if (efi_mem_attribute(page_base, size) & EFI_MEMORY_WB) { - prot = PAGE_KERNEL; - - /* - * Mappings have to be page-aligned - */ - offset = phys_addr & ~PAGE_MASK; - phys_addr &= PAGE_MASK; - - /* - * Ok, go for it.. - */ - area = get_vm_area(size, VM_IOREMAP); - if (!area) - return NULL; - - area->phys_addr = phys_addr; - addr = (void __iomem *) area->addr; - if (ioremap_page_range((unsigned long) addr, - (unsigned long) addr + size, phys_addr, prot)) { - vunmap((void __force *) addr); - return NULL; - } - - return (void __iomem *) (offset + (char __iomem *)addr); + return NULL; } - return __ioremap_uc(phys_addr); + return __ioremap_uc(*paddr); +} + +int arch_iounmap(void __iomem *addr) +{ + if (REGION_NUMBER(addr) != RGN_GATE) + return -EINVAL; + return 0; } -EXPORT_SYMBOL(ioremap); void __iomem * ioremap_uc(unsigned long phys_addr, unsigned long size)
Le 13/09/2022 à 17:11, Baoquan He a écrit : > On 09/12/22 at 07:10am, Christophe Leroy wrote: >> Hi Baoquan, >> >> Le 12/09/2022 à 04:55, Baoquan He a écrit : >>> Hi Christophe, >>> >>> On 08/28/22 at 07:10pm, Baoquan He wrote: >>>> On 08/23/22 at 07:03pm, Christophe Leroy wrote: >>> ...... >>>>>>>>> Is it really the best approach ? Wouldn't it be better to have helpers >>>>>>>>> to do that, those helpers being called by the ioremap_prot(), instead of >>>>>>>>> doing it inside the arch_ioremap() function ? >>>>>>>> >>>>>>>> This is suggested too by Alexander during his v1 reviewing. I tried, but >>>>>>>> feel the current way taken in this patchset is better. Because not all >>>>>>>> architecutres need the address fix up, only parisc, and only few need >>>>>>>> adjust the 'prot'. Introducing other helpers seems too much, that only >>>>>>>> increases the complexity of of ioremap() and the generic GENERIC_IOREMAP >>>>>>>> method for people to understand and take. >>>>>>> >>>>>>> I can't understand. Why is it difficult to do something like: >>>>>>> >>>>>>> #ifndef ioremap_adjust_prot >>>>>>> static inline unsigned long ioremap_adjust_prot(unsigned long flags) >>>>>>> { >>>>>>> return flags; >>>>>>> } >>>>>>> #endif >>>>>>> >>>>>>> Then for arc you do >>>>>>> >>>>>>> static inline unsigned long ioremap_adjust_prot(unsigned long flags) >>>>>>> { >>>>>>> return pgprot_val(pgprot_noncached(__pgprot(flags))); >>>>>>> } >>>>>>> #define ioremap_adjust_prot ioremap_adjust_prot >>>>>> >>>>>> My thinking is we have four things to do in the added hookers. >>>>>> 1) check if we should do ioremap on ARCHes. If not, return NULL from >>>>>> ioremap_prot(); >>>>>> 2) handling the mapping io address specifically on ARCHes, e.g arc, >>>>>> ia64, s390; >>>>>> 3) the original physical address passed into ioremap_prot() need be >>>>>> fixed up, e.g arc; >>>>>> 4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc >>>>>> and xtensa. >>>>>> >>>>>> With Kefeng's patches, the case 1) is handled with introduced >>>>>> ioremap_allowed()/iounmap_allowed(). In this patchset, what I do is >>>>>> rename the hooks as arch_ioremap() and arch_iounmap(), then put case 1), >>>>>> 2), 3), 4) handling into arch_ioremap(). Adding helpers to cover each >>>>>> case is not difficult from my side. I worry that as time goes by, those >>>>>> several hooks my cause issue, e.g if a new adjustment need be done, >>>>>> should we introduce a new helper or make do with the existed hook; how >>>>>> >>>>>> When I investigated this, one arch_ioremap() looks not complicated >>>>>> since not all ARCHes need cover all above 4 cases. That's why I finally >>>>>> choose one hook. I am open to new idea, please let me know if we should >>>>>> change it to introduce several different helpers. >>>>>> >>>>> >>>>> A new idea that would have my preference would be to do just like we did >>>>> with arch_get_unmapped_area(). Look at >>>>> https://elixir.bootlin.com/linux/v6.0-rc1/source/arch/powerpc/mm/book3s64/slice.c#L638 >>>>> and https://elixir.bootlin.com/linux/v6.0-rc1/source/mm/mmap.c#L2131 >>>>> >>>>> Instead of having the generic that calls the arch specific, make it the >>>>> other way round, have the arch specific call the generic after doing its >>>>> specialties. >>>> >>>> This sounds good. I made a draft patch to change code in generic code >>>> part, just showing what it looks like. >>>> >>>> Both arch_ioremap() way and the arch sepcific call the generic way look >>>> good to me. Just it will take less effort for me to continue the >>>> arch_ioremap() way. I would like to hear Christoph's opinion since he >>>> introduced the GENERIC_IOREMAP method and suggested the earlier >>>> arch_ioremap() way and change in this patchset. >>> >>> I will make another round change and post. Since Christoph doesn't >>> reply, I would like to continue with the existing >>> arch_ioremap/arch_iounmap() hooks way if you don't have strong opinion >>> on the new idea to reintroduce ioremap(). >>> >> >> I still dislike you approach with the architectures modifying local vars >> by reference, and as you said earlier I'm not the only one : "This is >> suggested too by Alexander during his v1 reviewing". > > Alexander suggested several helpers, as I have explained earlier, that > will cause at least four helpers currently. And could be more later if > new change is introduced. And the address fixup and prot modifcation > are related in few architecutures. Adding all of them is is not so > necessary. > >> >> So I'd really prefer you to reconsider your approach and avoid passign >> pointers to local vars to architecture helpers. > > If only passing pointers to local vars is disliked, I can explain why I > did so. Let me take arch_ioremap() of a64 as example. I can derefence > pointer in arch_ioremap() to avoid assigning pointers to local vars. > Please see below two version for comparing, and please tell which one is > better. Ok, yes I overlooked and didn't remember it right. > > To me, assigning pointers to local vars make code simple and clean, > honestly. Well, for me it looks ood, not intellectually natural. If I understand correctly, you do ioremap() --> Call arch_ioremap() --> If the arch doesn't want to handle ioremap itself, it returns NULL --> Then you fallback on generic handling. The arch may say "I don't want to handle it", but at the same time it blindly modifies the parameters so that the generic handling is not exacly the generic handling. Not easy to follow for the reader. Do you have any exemple in the kernel that works more or less with the same approach ? What I propose is Arch specific ioremap() --> do proper preparation --> call generic_ioremap() And the generic fallback implementation when the arch doesn't have a specific ioremap() __weak ioremap() --> call generic_ioremap(). The above looks a lot more natural and easier to follow, it is clear for the reader which function does what. Christophe > > ****** > The version in which pointers assigned to local vars looks like below. > The old phys_addr is passed in, the assigning can decrease changed line. > diff --git a/arch/ia64/mm/ioremap.c b/arch/ia64/mm/ioremap.c > index 55fd3eb753ff..8a085fc660e3 100644 > --- a/arch/ia64/mm/ioremap.c > +++ b/arch/ia64/mm/ioremap.c > @@ -30,15 +30,12 @@ early_ioremap (unsigned long phys_addr, unsigned long size) > } > > void __iomem * > -ioremap (unsigned long phys_addr, unsigned long size) > +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val) > { > - void __iomem *addr; > - struct vm_struct *area; > - unsigned long offset; > - pgprot_t prot; > - u64 attr; > + phys_addr_t phys_addr = *paddr; > unsigned long gran_base, gran_size; > unsigned long page_base; > + u64 attr; > > /* > * For things in kern_memmap, we must use the same attribute > @@ -69,35 +66,18 @@ ioremap (unsigned long phys_addr, unsigned long size) > page_base = phys_addr & PAGE_MASK; > size = PAGE_ALIGN(phys_addr + size) - page_base; > if (efi_mem_attribute(page_base, size) & EFI_MEMORY_WB) { > - prot = PAGE_KERNEL; > - > - /* > - * Mappings have to be page-aligned > - */ > - offset = phys_addr & ~PAGE_MASK; > - phys_addr &= PAGE_MASK; > - > - /* > - * Ok, go for it.. > - */ > - area = get_vm_area(size, VM_IOREMAP); > - if (!area) > - return NULL; > - > - area->phys_addr = phys_addr; > - addr = (void __iomem *) area->addr; > - if (ioremap_page_range((unsigned long) addr, > - (unsigned long) addr + size, phys_addr, prot)) { > - vunmap((void __force *) addr); > - return NULL; > - } > - > - return (void __iomem *) (offset + (char __iomem *)addr); > + return NULL; > } > > return __ioremap_uc(phys_addr); > } > -EXPORT_SYMBOL(ioremap); > + > +int arch_iounmap(void __iomem *addr) > +{ > + if (REGION_NUMBER(addr) != RGN_GATE) > + return -EINVAL; > + return 0; > +} > > void __iomem * > ioremap_uc(unsigned long phys_addr, unsigned long size) > > > ********* > The version in which pointer is dereferenced directly in place. Then > more lines of code are involved. And some pointer derefencing takes > place in macro, so bracket is needed. > diff --git a/arch/ia64/mm/ioremap.c b/arch/ia64/mm/ioremap.c > index 55fd3eb753ff..e1b991dc2347 100644 > --- a/arch/ia64/mm/ioremap.c > +++ b/arch/ia64/mm/ioremap.c > @@ -30,35 +30,32 @@ early_ioremap (unsigned long phys_addr, unsigned long size) > } > > void __iomem * > -ioremap (unsigned long phys_addr, unsigned long size) > +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val) > { > - void __iomem *addr; > - struct vm_struct *area; > - unsigned long offset; > - pgprot_t prot; > - u64 attr; > + phys_addr_t phys_addr = *paddr; > unsigned long gran_base, gran_size; > unsigned long page_base; > + u64 attr; > > /* > * For things in kern_memmap, we must use the same attribute > * as the rest of the kernel. For more details, see > * Documentation/ia64/aliasing.rst. > */ > - attr = kern_mem_attribute(phys_addr, size); > + attr = kern_mem_attribute(*paddr, size); > if (attr & EFI_MEMORY_WB) > - return (void __iomem *) phys_to_virt(phys_addr); > + return (void __iomem *) phys_to_virt(*paddr); > else if (attr & EFI_MEMORY_UC) > - return __ioremap_uc(phys_addr); > + return __ioremap_uc(*paddr); > > /* > * Some chipsets don't support UC access to memory. If > * WB is supported for the whole granule, we prefer that. > */ > - gran_base = GRANULEROUNDDOWN(phys_addr); > - gran_size = GRANULEROUNDUP(phys_addr + size) - gran_base; > + gran_base = GRANULEROUNDDOWN(*paddr); > + gran_size = GRANULEROUNDUP(*paddr + size) - gran_base; > if (efi_mem_attribute(gran_base, gran_size) & EFI_MEMORY_WB) > - return (void __iomem *) phys_to_virt(phys_addr); > + return (void __iomem *) phys_to_virt(*paddr); > > /* > * WB is not supported for the whole granule, so we can't use > @@ -66,38 +63,21 @@ ioremap (unsigned long phys_addr, unsigned long size) > * area with kernel page table mappings, we can use those > * instead. > */ > - page_base = phys_addr & PAGE_MASK; > - size = PAGE_ALIGN(phys_addr + size) - page_base; > + page_base = (*paddr) & PAGE_MASK; > + size = PAGE_ALIGN(*paddr + size) - page_base; > if (efi_mem_attribute(page_base, size) & EFI_MEMORY_WB) { > - prot = PAGE_KERNEL; > - > - /* > - * Mappings have to be page-aligned > - */ > - offset = phys_addr & ~PAGE_MASK; > - phys_addr &= PAGE_MASK; > - > - /* > - * Ok, go for it.. > - */ > - area = get_vm_area(size, VM_IOREMAP); > - if (!area) > - return NULL; > - > - area->phys_addr = phys_addr; > - addr = (void __iomem *) area->addr; > - if (ioremap_page_range((unsigned long) addr, > - (unsigned long) addr + size, phys_addr, prot)) { > - vunmap((void __force *) addr); > - return NULL; > - } > - > - return (void __iomem *) (offset + (char __iomem *)addr); > + return NULL; > } > > - return __ioremap_uc(phys_addr); > + return __ioremap_uc(*paddr); > +} > + > +int arch_iounmap(void __iomem *addr) > +{ > + if (REGION_NUMBER(addr) != RGN_GATE) > + return -EINVAL; > + return 0; > } > -EXPORT_SYMBOL(ioremap); > > void __iomem * > ioremap_uc(unsigned long phys_addr, unsigned long size) >
On 09/21/22 at 04:40pm, Christophe Leroy wrote: > > > Le 13/09/2022 à 17:11, Baoquan He a écrit : > > On 09/12/22 at 07:10am, Christophe Leroy wrote: > >> Hi Baoquan, > >> > >> Le 12/09/2022 à 04:55, Baoquan He a écrit : > >>> Hi Christophe, > >>> > >>> On 08/28/22 at 07:10pm, Baoquan He wrote: > >>>> On 08/23/22 at 07:03pm, Christophe Leroy wrote: > >>> ...... > >>>>>>>>> Is it really the best approach ? Wouldn't it be better to have helpers > >>>>>>>>> to do that, those helpers being called by the ioremap_prot(), instead of > >>>>>>>>> doing it inside the arch_ioremap() function ? > >>>>>>>> > >>>>>>>> This is suggested too by Alexander during his v1 reviewing. I tried, but > >>>>>>>> feel the current way taken in this patchset is better. Because not all > >>>>>>>> architecutres need the address fix up, only parisc, and only few need > >>>>>>>> adjust the 'prot'. Introducing other helpers seems too much, that only > >>>>>>>> increases the complexity of of ioremap() and the generic GENERIC_IOREMAP > >>>>>>>> method for people to understand and take. > >>>>>>> > >>>>>>> I can't understand. Why is it difficult to do something like: > >>>>>>> > >>>>>>> #ifndef ioremap_adjust_prot > >>>>>>> static inline unsigned long ioremap_adjust_prot(unsigned long flags) > >>>>>>> { > >>>>>>> return flags; > >>>>>>> } > >>>>>>> #endif > >>>>>>> > >>>>>>> Then for arc you do > >>>>>>> > >>>>>>> static inline unsigned long ioremap_adjust_prot(unsigned long flags) > >>>>>>> { > >>>>>>> return pgprot_val(pgprot_noncached(__pgprot(flags))); > >>>>>>> } > >>>>>>> #define ioremap_adjust_prot ioremap_adjust_prot > >>>>>> > >>>>>> My thinking is we have four things to do in the added hookers. > >>>>>> 1) check if we should do ioremap on ARCHes. If not, return NULL from > >>>>>> ioremap_prot(); > >>>>>> 2) handling the mapping io address specifically on ARCHes, e.g arc, > >>>>>> ia64, s390; > >>>>>> 3) the original physical address passed into ioremap_prot() need be > >>>>>> fixed up, e.g arc; > >>>>>> 4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc > >>>>>> and xtensa. > >>>>>> > >>>>>> With Kefeng's patches, the case 1) is handled with introduced > >>>>>> ioremap_allowed()/iounmap_allowed(). In this patchset, what I do is > >>>>>> rename the hooks as arch_ioremap() and arch_iounmap(), then put case 1), > >>>>>> 2), 3), 4) handling into arch_ioremap(). Adding helpers to cover each > >>>>>> case is not difficult from my side. I worry that as time goes by, those > >>>>>> several hooks my cause issue, e.g if a new adjustment need be done, > >>>>>> should we introduce a new helper or make do with the existed hook; how > >>>>>> > >>>>>> When I investigated this, one arch_ioremap() looks not complicated > >>>>>> since not all ARCHes need cover all above 4 cases. That's why I finally > >>>>>> choose one hook. I am open to new idea, please let me know if we should > >>>>>> change it to introduce several different helpers. > >>>>>> > >>>>> > >>>>> A new idea that would have my preference would be to do just like we did > >>>>> with arch_get_unmapped_area(). Look at > >>>>> https://elixir.bootlin.com/linux/v6.0-rc1/source/arch/powerpc/mm/book3s64/slice.c#L638 > >>>>> and https://elixir.bootlin.com/linux/v6.0-rc1/source/mm/mmap.c#L2131 > >>>>> > >>>>> Instead of having the generic that calls the arch specific, make it the > >>>>> other way round, have the arch specific call the generic after doing its > >>>>> specialties. > >>>> > >>>> This sounds good. I made a draft patch to change code in generic code > >>>> part, just showing what it looks like. > >>>> > >>>> Both arch_ioremap() way and the arch sepcific call the generic way look > >>>> good to me. Just it will take less effort for me to continue the > >>>> arch_ioremap() way. I would like to hear Christoph's opinion since he > >>>> introduced the GENERIC_IOREMAP method and suggested the earlier > >>>> arch_ioremap() way and change in this patchset. > >>> > >>> I will make another round change and post. Since Christoph doesn't > >>> reply, I would like to continue with the existing > >>> arch_ioremap/arch_iounmap() hooks way if you don't have strong opinion > >>> on the new idea to reintroduce ioremap(). > >>> > >> > >> I still dislike you approach with the architectures modifying local vars > >> by reference, and as you said earlier I'm not the only one : "This is > >> suggested too by Alexander during his v1 reviewing". > > > > Alexander suggested several helpers, as I have explained earlier, that > > will cause at least four helpers currently. And could be more later if > > new change is introduced. And the address fixup and prot modifcation > > are related in few architecutures. Adding all of them is is not so > > necessary. > > > >> > >> So I'd really prefer you to reconsider your approach and avoid passign > >> pointers to local vars to architecture helpers. > > > > If only passing pointers to local vars is disliked, I can explain why I > > did so. Let me take arch_ioremap() of a64 as example. I can derefence > > pointer in arch_ioremap() to avoid assigning pointers to local vars. > > Please see below two version for comparing, and please tell which one is > > better. > > Ok, yes I overlooked and didn't remember it right. > > > > > To me, assigning pointers to local vars make code simple and clean, > > honestly. > > Well, for me it looks ood, not intellectually natural. > > If I understand correctly, you do > > ioremap() > --> Call arch_ioremap() > --> If the arch doesn't want to handle ioremap itself, it returns NULL > --> Then you fallback on generic handling. > > The arch may say "I don't want to handle it", but at the same time it > blindly modifies the parameters so that the generic handling is not > exacly the generic handling. No, it's not like that. Arch_ioremap() provides a place where arch specific handling can be done. While it doesn't mean arch has to do all of the follow four things. From the current change I made listed as below, arch_ioremap() in each architecture is quite simple to do one or two things. People only need to provide arch_ioremap() if needed, or do not provide arch_ioremap() at all if not needed. No such thing could hapen like you said that arch don't want to handle, but it blindly modifes the parameter. arm64: - address checking; hexagon: removes the ioremap() directly since it has the same code as standard; - N/A arc: - address checking - modify the prot flag; ia64: - specific io address mapping openrisc: removes ioremap() directly after its early ioremap is cleared away; - N/A parisc: - physical address fixup - address checking; s390: - specific io address mapping sh: - specific io address mapping xtensa: - address checking; Four optional things could be done in arch_ioremap(): 1) check if we should do ioremap on ARCHes. If not, return NULL from ioremap_prot(); 2) handling the mapping io address specifically on ARCHes, e.g arc, ia64, s390; 3) the original physical address passed into ioremap_prot() need be fixed up, e.g arc; 4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc and xtensa. > Not easy to follow for the reader. Do you have any exemple in the kernel > that works more or less with the same approach ? > > > What I propose is > > Arch specific ioremap() > --> do proper preparation > --> call generic_ioremap() > > And the generic fallback implementation when the arch doesn't have a > specific ioremap() > > __weak ioremap() > --> call generic_ioremap(). Hmm, __weak is not suggested any more in kernel. Please see below thread. [PATCH] kexec_file: Drop weak attribute from arch_kexec_apply_relocations[_add] https://lore.kernel.org/all/20220518181828.645877-1-naveen.n.rao@linux.vnet.ibm.com/T/#u So with arch_ioremap() hook, now in each architecture it is: arch_ioremap() is provided and called or no hookd is needed at all in arch It's simpler, isn't it? > > The above looks a lot more natural and easier to follow, it is clear for > the reader which function does what. Before, there's only arm64 taking GENERIC_IOREMAP way and having arch_ioremap() as an exmaple. Now after this patchset, there are so many architectures taking the way, it's very easy to refer to and study, right? Thanks Baoquan
diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h index dd7e1c2dc86c..6a5578ddbbf6 100644 --- a/arch/arm64/include/asm/io.h +++ b/arch/arm64/include/asm/io.h @@ -139,7 +139,8 @@ extern void __memset_io(volatile void __iomem *, int, size_t); * I/O memory mapping functions. */ -void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot); +void __iomem * +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val); #define arch_ioremap arch_ioremap #define _PAGE_IOREMAP PROT_DEVICE_nGnRE diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c index b0f4cea86f0e..ef75ffef4dbc 100644 --- a/arch/arm64/mm/ioremap.c +++ b/arch/arm64/mm/ioremap.c @@ -3,9 +3,10 @@ #include <linux/mm.h> #include <linux/io.h> -void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot) +void __iomem * +arch_ioremap(phys_addr_t *paddr, size_t size, unsigned long *prot_val) { - unsigned long last_addr, offset; + unsigned long last_addr, offset, phys_addr = *paddr; offset = phys_addr & (~PAGE_MASK); phys_addr -= offset; diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h index 7b6bfb62ef80..fb9bda2be8ed 100644 --- a/include/asm-generic/io.h +++ b/include/asm-generic/io.h @@ -1059,8 +1059,8 @@ static inline void iounmap(volatile void __iomem *addr) */ #ifndef arch_ioremap #define arch_ioremap arch_ioremap -static inline void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, - unsigned long prot) +static inline void __iomem *arch_ioremap(phys_addr_t *paddr, size_t size, + unsigned long *prot_val) { return NULL; } diff --git a/mm/ioremap.c b/mm/ioremap.c index 99fde69becc7..7914b5cf5b78 100644 --- a/mm/ioremap.c +++ b/mm/ioremap.c @@ -19,7 +19,7 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, struct vm_struct *area; void __iomem *ioaddr; - ioaddr = arch_ioremap(phys_addr, size, prot); + ioaddr = arch_ioremap(&phys_addr, size, &prot); if (IS_ERR(ioaddr)) return NULL; else if (ioaddr)
On some architectures, the physical address need be fixed up before doing mapping, e.g, parisc. And on architectures, e.g arc, the parameter 'prot' passed into ioremap_prot() need be adjusted too. In oder to convert them to take GENERIC_IOREMAP method, we need wrap the address fixing up code and page prot adjusting code into arch_ioremap() and pass the new address and 'prot' out for ioremap_prot() handling. This is a preparation patch, no functionality change. Signed-off-by: Baoquan He <bhe@redhat.com> --- arch/arm64/include/asm/io.h | 3 ++- arch/arm64/mm/ioremap.c | 5 +++-- include/asm-generic/io.h | 4 ++-- mm/ioremap.c | 2 +- 4 files changed, 8 insertions(+), 6 deletions(-)