| Message ID | 20191209191346.5197-6-logang@deltatee.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allow setting caching mode in arch_add_memory() for P2PDMA | expand |
On 09.12.19 20:13, Logan Gunthorpe wrote: > devm_memremap_pages() is currently used by the PCI P2PDMA code to create > struct page mappings for IO memory. At present, these mappings are created > with PAGE_KERNEL which implies setting the PAT bits to be WB. However, on > x86, an mtrr register will typically override this and force the cache > type to be UC-. In the case firmware doesn't set this register it is > effectively WB and will typically result in a machine check exception > when it's accessed. > > Other arches are not currently likely to function correctly seeing they > don't have any MTRR registers to fall back on. > > To solve this, add an argument to arch_add_memory() to explicitly > set the pgprot value to a specific value. > > Of the arches that support MEMORY_HOTPLUG: x86_64, s390 and arm64 is a > simple change to pass the pgprot_t down to their respective functions > which set up the page tables. For x86_32, set the page tables explicitly > using _set_memory_prot() (seeing they are already mapped). For sh, reject > anything but PAGE_KERNEL settings -- this should be fine, for now, seeing > sh doesn't support ZONE_DEVICE anyway. > > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: David Hildenbrand <david@redhat.com> > Cc: Michal Hocko <mhocko@suse.com> > Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > --- > arch/arm64/mm/mmu.c | 4 ++-- > arch/ia64/mm/init.c | 5 ++++- > arch/powerpc/mm/mem.c | 4 ++-- > arch/s390/mm/init.c | 4 ++-- > arch/sh/mm/init.c | 5 ++++- > arch/x86/mm/init_32.c | 7 ++++++- > arch/x86/mm/init_64.c | 4 ++-- > include/linux/memory_hotplug.h | 2 +- > mm/memory_hotplug.c | 2 +- > mm/memremap.c | 2 +- > 10 files changed, 25 insertions(+), 14 deletions(-) > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > index 60c929f3683b..48b65272df15 100644 > --- a/arch/arm64/mm/mmu.c > +++ b/arch/arm64/mm/mmu.c > @@ -1050,7 +1050,7 @@ int p4d_free_pud_page(p4d_t *p4d, unsigned long addr) > } > > #ifdef CONFIG_MEMORY_HOTPLUG > -int arch_add_memory(int nid, u64 start, u64 size, > +int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, > struct mhp_restrictions *restrictions) Can we fiddle that into "struct mhp_restrictions" instead?
On 2019-12-09 12:23 p.m., David Hildenbrand wrote: > On 09.12.19 20:13, Logan Gunthorpe wrote: >> devm_memremap_pages() is currently used by the PCI P2PDMA code to create >> struct page mappings for IO memory. At present, these mappings are created >> with PAGE_KERNEL which implies setting the PAT bits to be WB. However, on >> x86, an mtrr register will typically override this and force the cache >> type to be UC-. In the case firmware doesn't set this register it is >> effectively WB and will typically result in a machine check exception >> when it's accessed. >> >> Other arches are not currently likely to function correctly seeing they >> don't have any MTRR registers to fall back on. >> >> To solve this, add an argument to arch_add_memory() to explicitly >> set the pgprot value to a specific value. >> >> Of the arches that support MEMORY_HOTPLUG: x86_64, s390 and arm64 is a >> simple change to pass the pgprot_t down to their respective functions >> which set up the page tables. For x86_32, set the page tables explicitly >> using _set_memory_prot() (seeing they are already mapped). For sh, reject >> anything but PAGE_KERNEL settings -- this should be fine, for now, seeing >> sh doesn't support ZONE_DEVICE anyway. >> >> Cc: Dan Williams <dan.j.williams@intel.com> >> Cc: David Hildenbrand <david@redhat.com> >> Cc: Michal Hocko <mhocko@suse.com> >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >> --- >> arch/arm64/mm/mmu.c | 4 ++-- >> arch/ia64/mm/init.c | 5 ++++- >> arch/powerpc/mm/mem.c | 4 ++-- >> arch/s390/mm/init.c | 4 ++-- >> arch/sh/mm/init.c | 5 ++++- >> arch/x86/mm/init_32.c | 7 ++++++- >> arch/x86/mm/init_64.c | 4 ++-- >> include/linux/memory_hotplug.h | 2 +- >> mm/memory_hotplug.c | 2 +- >> mm/memremap.c | 2 +- >> 10 files changed, 25 insertions(+), 14 deletions(-) >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c >> index 60c929f3683b..48b65272df15 100644 >> --- a/arch/arm64/mm/mmu.c >> +++ b/arch/arm64/mm/mmu.c >> @@ -1050,7 +1050,7 @@ int p4d_free_pud_page(p4d_t *p4d, unsigned long addr) >> } >> >> #ifdef CONFIG_MEMORY_HOTPLUG >> -int arch_add_memory(int nid, u64 start, u64 size, >> +int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, >> struct mhp_restrictions *restrictions) > > Can we fiddle that into "struct mhp_restrictions" instead? Yes, if that's what people want, it's pretty trivial to do. I chose not to do it that way because it doesn't get passed down to add_pages() and it's not really a "restriction". If I don't hear any objections, I will do that for v2. Thanks, Logan
On Mon 09-12-19 13:24:19, Logan Gunthorpe wrote: > > > On 2019-12-09 12:23 p.m., David Hildenbrand wrote: > > On 09.12.19 20:13, Logan Gunthorpe wrote: > >> devm_memremap_pages() is currently used by the PCI P2PDMA code to create > >> struct page mappings for IO memory. At present, these mappings are created > >> with PAGE_KERNEL which implies setting the PAT bits to be WB. However, on > >> x86, an mtrr register will typically override this and force the cache > >> type to be UC-. In the case firmware doesn't set this register it is > >> effectively WB and will typically result in a machine check exception > >> when it's accessed. > >> > >> Other arches are not currently likely to function correctly seeing they > >> don't have any MTRR registers to fall back on. > >> > >> To solve this, add an argument to arch_add_memory() to explicitly > >> set the pgprot value to a specific value. > >> > >> Of the arches that support MEMORY_HOTPLUG: x86_64, s390 and arm64 is a > >> simple change to pass the pgprot_t down to their respective functions > >> which set up the page tables. For x86_32, set the page tables explicitly > >> using _set_memory_prot() (seeing they are already mapped). For sh, reject > >> anything but PAGE_KERNEL settings -- this should be fine, for now, seeing > >> sh doesn't support ZONE_DEVICE anyway. > >> > >> Cc: Dan Williams <dan.j.williams@intel.com> > >> Cc: David Hildenbrand <david@redhat.com> > >> Cc: Michal Hocko <mhocko@suse.com> > >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > >> --- > >> arch/arm64/mm/mmu.c | 4 ++-- > >> arch/ia64/mm/init.c | 5 ++++- > >> arch/powerpc/mm/mem.c | 4 ++-- > >> arch/s390/mm/init.c | 4 ++-- > >> arch/sh/mm/init.c | 5 ++++- > >> arch/x86/mm/init_32.c | 7 ++++++- > >> arch/x86/mm/init_64.c | 4 ++-- > >> include/linux/memory_hotplug.h | 2 +- > >> mm/memory_hotplug.c | 2 +- > >> mm/memremap.c | 2 +- > >> 10 files changed, 25 insertions(+), 14 deletions(-) > >> > >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > >> index 60c929f3683b..48b65272df15 100644 > >> --- a/arch/arm64/mm/mmu.c > >> +++ b/arch/arm64/mm/mmu.c > >> @@ -1050,7 +1050,7 @@ int p4d_free_pud_page(p4d_t *p4d, unsigned long addr) > >> } > >> > >> #ifdef CONFIG_MEMORY_HOTPLUG > >> -int arch_add_memory(int nid, u64 start, u64 size, > >> +int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, > >> struct mhp_restrictions *restrictions) > > > > Can we fiddle that into "struct mhp_restrictions" instead? > > Yes, if that's what people want, it's pretty trivial to do. I chose not > to do it that way because it doesn't get passed down to add_pages() and > it's not really a "restriction". If I don't hear any objections, I will > do that for v2. I do agree that restriction is not the best fit. But I consider prot argument to complicate the API to all users even though it is not really clear whether we are going to have many users really benefiting from it. Look at the vmalloc API and try to find how many users of __vmalloc do not use PAGE_KERNEL. So I can see two options. One of them is to add arch_add_memory_prot that would allow to have give and extra prot argument or simply call an arch independent API to change the protection after arch_add_memory. The later sounds like much less code. The memory shouldn't be in use by anybody at that stage yet AFAIU. Maybe there even is an API like that.
On Mon, Dec 9, 2019 at 12:24 PM Logan Gunthorpe <logang@deltatee.com> wrote: > > > > On 2019-12-09 12:23 p.m., David Hildenbrand wrote: > > On 09.12.19 20:13, Logan Gunthorpe wrote: > >> devm_memremap_pages() is currently used by the PCI P2PDMA code to create > >> struct page mappings for IO memory. At present, these mappings are created > >> with PAGE_KERNEL which implies setting the PAT bits to be WB. However, on > >> x86, an mtrr register will typically override this and force the cache > >> type to be UC-. In the case firmware doesn't set this register it is > >> effectively WB and will typically result in a machine check exception > >> when it's accessed. > >> > >> Other arches are not currently likely to function correctly seeing they > >> don't have any MTRR registers to fall back on. > >> > >> To solve this, add an argument to arch_add_memory() to explicitly > >> set the pgprot value to a specific value. > >> > >> Of the arches that support MEMORY_HOTPLUG: x86_64, s390 and arm64 is a > >> simple change to pass the pgprot_t down to their respective functions > >> which set up the page tables. For x86_32, set the page tables explicitly > >> using _set_memory_prot() (seeing they are already mapped). For sh, reject > >> anything but PAGE_KERNEL settings -- this should be fine, for now, seeing > >> sh doesn't support ZONE_DEVICE anyway. > >> > >> Cc: Dan Williams <dan.j.williams@intel.com> > >> Cc: David Hildenbrand <david@redhat.com> > >> Cc: Michal Hocko <mhocko@suse.com> > >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > >> --- > >> arch/arm64/mm/mmu.c | 4 ++-- > >> arch/ia64/mm/init.c | 5 ++++- > >> arch/powerpc/mm/mem.c | 4 ++-- > >> arch/s390/mm/init.c | 4 ++-- > >> arch/sh/mm/init.c | 5 ++++- > >> arch/x86/mm/init_32.c | 7 ++++++- > >> arch/x86/mm/init_64.c | 4 ++-- > >> include/linux/memory_hotplug.h | 2 +- > >> mm/memory_hotplug.c | 2 +- > >> mm/memremap.c | 2 +- > >> 10 files changed, 25 insertions(+), 14 deletions(-) > >> > >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > >> index 60c929f3683b..48b65272df15 100644 > >> --- a/arch/arm64/mm/mmu.c > >> +++ b/arch/arm64/mm/mmu.c > >> @@ -1050,7 +1050,7 @@ int p4d_free_pud_page(p4d_t *p4d, unsigned long addr) > >> } > >> > >> #ifdef CONFIG_MEMORY_HOTPLUG > >> -int arch_add_memory(int nid, u64 start, u64 size, > >> +int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, > >> struct mhp_restrictions *restrictions) > > > > Can we fiddle that into "struct mhp_restrictions" instead? > > Yes, if that's what people want, it's pretty trivial to do. I chose not > to do it that way because it doesn't get passed down to add_pages() and > it's not really a "restriction". If I don't hear any objections, I will > do that for v2. +1 to storing this information alongside the altmap in that structure. However, I agree struct mhp_restrictions, with the MHP_MEMBLOCK_API flag now gone, has lost all of its "restrictions". How about dropping the 'flags' property and renaming the struct to 'struct mhp_modifiers'?
> Am 09.12.2019 um 21:43 schrieb Dan Williams <dan.j.williams@intel.com>: > > On Mon, Dec 9, 2019 at 12:24 PM Logan Gunthorpe <logang@deltatee.com> wrote: >> >> >> >>> On 2019-12-09 12:23 p.m., David Hildenbrand wrote: >>> On 09.12.19 20:13, Logan Gunthorpe wrote: >>>> devm_memremap_pages() is currently used by the PCI P2PDMA code to create >>>> struct page mappings for IO memory. At present, these mappings are created >>>> with PAGE_KERNEL which implies setting the PAT bits to be WB. However, on >>>> x86, an mtrr register will typically override this and force the cache >>>> type to be UC-. In the case firmware doesn't set this register it is >>>> effectively WB and will typically result in a machine check exception >>>> when it's accessed. >>>> >>>> Other arches are not currently likely to function correctly seeing they >>>> don't have any MTRR registers to fall back on. >>>> >>>> To solve this, add an argument to arch_add_memory() to explicitly >>>> set the pgprot value to a specific value. >>>> >>>> Of the arches that support MEMORY_HOTPLUG: x86_64, s390 and arm64 is a >>>> simple change to pass the pgprot_t down to their respective functions >>>> which set up the page tables. For x86_32, set the page tables explicitly >>>> using _set_memory_prot() (seeing they are already mapped). For sh, reject >>>> anything but PAGE_KERNEL settings -- this should be fine, for now, seeing >>>> sh doesn't support ZONE_DEVICE anyway. >>>> >>>> Cc: Dan Williams <dan.j.williams@intel.com> >>>> Cc: David Hildenbrand <david@redhat.com> >>>> Cc: Michal Hocko <mhocko@suse.com> >>>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >>>> --- >>>> arch/arm64/mm/mmu.c | 4 ++-- >>>> arch/ia64/mm/init.c | 5 ++++- >>>> arch/powerpc/mm/mem.c | 4 ++-- >>>> arch/s390/mm/init.c | 4 ++-- >>>> arch/sh/mm/init.c | 5 ++++- >>>> arch/x86/mm/init_32.c | 7 ++++++- >>>> arch/x86/mm/init_64.c | 4 ++-- >>>> include/linux/memory_hotplug.h | 2 +- >>>> mm/memory_hotplug.c | 2 +- >>>> mm/memremap.c | 2 +- >>>> 10 files changed, 25 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c >>>> index 60c929f3683b..48b65272df15 100644 >>>> --- a/arch/arm64/mm/mmu.c >>>> +++ b/arch/arm64/mm/mmu.c >>>> @@ -1050,7 +1050,7 @@ int p4d_free_pud_page(p4d_t *p4d, unsigned long addr) >>>> } >>>> >>>> #ifdef CONFIG_MEMORY_HOTPLUG >>>> -int arch_add_memory(int nid, u64 start, u64 size, >>>> +int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, >>>> struct mhp_restrictions *restrictions) >>> >>> Can we fiddle that into "struct mhp_restrictions" instead? >> >> Yes, if that's what people want, it's pretty trivial to do. I chose not >> to do it that way because it doesn't get passed down to add_pages() and >> it's not really a "restriction". If I don't hear any objections, I will >> do that for v2. > > +1 to storing this information alongside the altmap in that structure. > However, I agree struct mhp_restrictions, with the MHP_MEMBLOCK_API > flag now gone, has lost all of its "restrictions". How about dropping > the 'flags' property and renaming the struct to 'struct > mhp_modifiers'? > I‘d prefer that over an arch_add_memory_protected() as suggested by Michal. But if we can change it after adding the memory (as also suggested by Michal), that would also be nice. Cheers!
On Mon, Dec 9, 2019 at 12:47 PM Michal Hocko <mhocko@kernel.org> wrote: > > On Mon 09-12-19 13:24:19, Logan Gunthorpe wrote: > > > > > > On 2019-12-09 12:23 p.m., David Hildenbrand wrote: > > > On 09.12.19 20:13, Logan Gunthorpe wrote: > > >> devm_memremap_pages() is currently used by the PCI P2PDMA code to create > > >> struct page mappings for IO memory. At present, these mappings are created > > >> with PAGE_KERNEL which implies setting the PAT bits to be WB. However, on > > >> x86, an mtrr register will typically override this and force the cache > > >> type to be UC-. In the case firmware doesn't set this register it is > > >> effectively WB and will typically result in a machine check exception > > >> when it's accessed. > > >> > > >> Other arches are not currently likely to function correctly seeing they > > >> don't have any MTRR registers to fall back on. > > >> > > >> To solve this, add an argument to arch_add_memory() to explicitly > > >> set the pgprot value to a specific value. > > >> > > >> Of the arches that support MEMORY_HOTPLUG: x86_64, s390 and arm64 is a > > >> simple change to pass the pgprot_t down to their respective functions > > >> which set up the page tables. For x86_32, set the page tables explicitly > > >> using _set_memory_prot() (seeing they are already mapped). For sh, reject > > >> anything but PAGE_KERNEL settings -- this should be fine, for now, seeing > > >> sh doesn't support ZONE_DEVICE anyway. > > >> > > >> Cc: Dan Williams <dan.j.williams@intel.com> > > >> Cc: David Hildenbrand <david@redhat.com> > > >> Cc: Michal Hocko <mhocko@suse.com> > > >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > > >> --- > > >> arch/arm64/mm/mmu.c | 4 ++-- > > >> arch/ia64/mm/init.c | 5 ++++- > > >> arch/powerpc/mm/mem.c | 4 ++-- > > >> arch/s390/mm/init.c | 4 ++-- > > >> arch/sh/mm/init.c | 5 ++++- > > >> arch/x86/mm/init_32.c | 7 ++++++- > > >> arch/x86/mm/init_64.c | 4 ++-- > > >> include/linux/memory_hotplug.h | 2 +- > > >> mm/memory_hotplug.c | 2 +- > > >> mm/memremap.c | 2 +- > > >> 10 files changed, 25 insertions(+), 14 deletions(-) > > >> > > >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > > >> index 60c929f3683b..48b65272df15 100644 > > >> --- a/arch/arm64/mm/mmu.c > > >> +++ b/arch/arm64/mm/mmu.c > > >> @@ -1050,7 +1050,7 @@ int p4d_free_pud_page(p4d_t *p4d, unsigned long addr) > > >> } > > >> > > >> #ifdef CONFIG_MEMORY_HOTPLUG > > >> -int arch_add_memory(int nid, u64 start, u64 size, > > >> +int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, > > >> struct mhp_restrictions *restrictions) > > > > > > Can we fiddle that into "struct mhp_restrictions" instead? > > > > Yes, if that's what people want, it's pretty trivial to do. I chose not > > to do it that way because it doesn't get passed down to add_pages() and > > it's not really a "restriction". If I don't hear any objections, I will > > do that for v2. > > I do agree that restriction is not the best fit. But I consider prot > argument to complicate the API to all users even though it is not really > clear whether we are going to have many users really benefiting from it. > Look at the vmalloc API and try to find how many users of __vmalloc do > not use PAGE_KERNEL. At least for this I can foresee at least one more user in the pipeline, encrypted memory support for persistent memory mappings that will store the key-id in the ptes. > > So I can see two options. One of them is to add arch_add_memory_prot > that would allow to have give and extra prot argument or simply call > an arch independent API to change the protection after arch_add_memory. > The later sounds like much less code. The memory shouldn't be in use by > anybody at that stage yet AFAIU. Maybe there even is an API like that. I'm ok with passing it the same way as altmap or a new arch_add_memory_prot() my only hangup with after the fact changes is the wasted effort it inflicts in the init path for potentially large address ranges.
On 2019-12-09 1:41 p.m., Michal Hocko wrote: > On Mon 09-12-19 13:24:19, Logan Gunthorpe wrote: >> >> >> On 2019-12-09 12:23 p.m., David Hildenbrand wrote: >>> On 09.12.19 20:13, Logan Gunthorpe wrote: >>>> devm_memremap_pages() is currently used by the PCI P2PDMA code to create >>>> struct page mappings for IO memory. At present, these mappings are created >>>> with PAGE_KERNEL which implies setting the PAT bits to be WB. However, on >>>> x86, an mtrr register will typically override this and force the cache >>>> type to be UC-. In the case firmware doesn't set this register it is >>>> effectively WB and will typically result in a machine check exception >>>> when it's accessed. >>>> >>>> Other arches are not currently likely to function correctly seeing they >>>> don't have any MTRR registers to fall back on. >>>> >>>> To solve this, add an argument to arch_add_memory() to explicitly >>>> set the pgprot value to a specific value. >>>> >>>> Of the arches that support MEMORY_HOTPLUG: x86_64, s390 and arm64 is a >>>> simple change to pass the pgprot_t down to their respective functions >>>> which set up the page tables. For x86_32, set the page tables explicitly >>>> using _set_memory_prot() (seeing they are already mapped). For sh, reject >>>> anything but PAGE_KERNEL settings -- this should be fine, for now, seeing >>>> sh doesn't support ZONE_DEVICE anyway. >>>> >>>> Cc: Dan Williams <dan.j.williams@intel.com> >>>> Cc: David Hildenbrand <david@redhat.com> >>>> Cc: Michal Hocko <mhocko@suse.com> >>>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >>>> --- >>>> arch/arm64/mm/mmu.c | 4 ++-- >>>> arch/ia64/mm/init.c | 5 ++++- >>>> arch/powerpc/mm/mem.c | 4 ++-- >>>> arch/s390/mm/init.c | 4 ++-- >>>> arch/sh/mm/init.c | 5 ++++- >>>> arch/x86/mm/init_32.c | 7 ++++++- >>>> arch/x86/mm/init_64.c | 4 ++-- >>>> include/linux/memory_hotplug.h | 2 +- >>>> mm/memory_hotplug.c | 2 +- >>>> mm/memremap.c | 2 +- >>>> 10 files changed, 25 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c >>>> index 60c929f3683b..48b65272df15 100644 >>>> --- a/arch/arm64/mm/mmu.c >>>> +++ b/arch/arm64/mm/mmu.c >>>> @@ -1050,7 +1050,7 @@ int p4d_free_pud_page(p4d_t *p4d, unsigned long addr) >>>> } >>>> >>>> #ifdef CONFIG_MEMORY_HOTPLUG >>>> -int arch_add_memory(int nid, u64 start, u64 size, >>>> +int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, >>>> struct mhp_restrictions *restrictions) >>> >>> Can we fiddle that into "struct mhp_restrictions" instead? >> >> Yes, if that's what people want, it's pretty trivial to do. I chose not >> to do it that way because it doesn't get passed down to add_pages() and >> it's not really a "restriction". If I don't hear any objections, I will >> do that for v2. > > I do agree that restriction is not the best fit. But I consider prot > argument to complicate the API to all users even though it is not really > clear whether we are going to have many users really benefiting from it. > Look at the vmalloc API and try to find how many users of __vmalloc do > not use PAGE_KERNEL. > > So I can see two options. One of them is to add arch_add_memory_prot > that would allow to have give and extra prot argument or simply call > an arch independent API to change the protection after arch_add_memory. > The later sounds like much less code. The memory shouldn't be in use by > anybody at that stage yet AFAIU. Maybe there even is an API like that. Yes, well, we tried something like this by calling set_memory_wc() inside memremap_pages(); but on large bars (tens of GB) it was too slow (taking several seconds to complete) and on some hosts actually hit CPU watchdog errors. So at the very least we'd have to add some cpu_relax() calls to that path. And it's also the case that set_memory_wc() is x86 only right now. So we'd have to create a new general interface to walk and fixup page tables for all arches. But, in my opinion, setting up all those page tables twice is too large of an overhead and it's better to just add them correctly the first time. The changes I propose to do this aren't really a lot of code and probably less than creating a new interface for all arches. Logan
On 2019-12-09 2:00 p.m., Dan Williams wrote: >>>> Can we fiddle that into "struct mhp_restrictions" instead? >>> >>> Yes, if that's what people want, it's pretty trivial to do. I chose not >>> to do it that way because it doesn't get passed down to add_pages() and >>> it's not really a "restriction". If I don't hear any objections, I will >>> do that for v2. >> >> I do agree that restriction is not the best fit. But I consider prot >> argument to complicate the API to all users even though it is not really >> clear whether we are going to have many users really benefiting from it. >> Look at the vmalloc API and try to find how many users of __vmalloc do >> not use PAGE_KERNEL. > > At least for this I can foresee at least one more user in the > pipeline, encrypted memory support for persistent memory mappings that > will store the key-id in the ptes. > >> >> So I can see two options. One of them is to add arch_add_memory_prot >> that would allow to have give and extra prot argument or simply call >> an arch independent API to change the protection after arch_add_memory. >> The later sounds like much less code. The memory shouldn't be in use by >> anybody at that stage yet AFAIU. Maybe there even is an API like that. > > I'm ok with passing it the same way as altmap or a new > arch_add_memory_prot() my only hangup with after the fact changes is > the wasted effort it inflicts in the init path for potentially large > address ranges. Yes, I'll change the way it's passed in for v2 as that seems to be generally agreed upon. I can also add a patch to make the name change. And, yes, given our testing, the wasted effort is quite significant so I'm against changing the prots after the fact. Logan
On Mon 09-12-19 14:24:22, Logan Gunthorpe wrote: > > > On 2019-12-09 1:41 p.m., Michal Hocko wrote: > > On Mon 09-12-19 13:24:19, Logan Gunthorpe wrote: > >> > >> > >> On 2019-12-09 12:23 p.m., David Hildenbrand wrote: > >>> On 09.12.19 20:13, Logan Gunthorpe wrote: > >>>> devm_memremap_pages() is currently used by the PCI P2PDMA code to create > >>>> struct page mappings for IO memory. At present, these mappings are created > >>>> with PAGE_KERNEL which implies setting the PAT bits to be WB. However, on > >>>> x86, an mtrr register will typically override this and force the cache > >>>> type to be UC-. In the case firmware doesn't set this register it is > >>>> effectively WB and will typically result in a machine check exception > >>>> when it's accessed. > >>>> > >>>> Other arches are not currently likely to function correctly seeing they > >>>> don't have any MTRR registers to fall back on. > >>>> > >>>> To solve this, add an argument to arch_add_memory() to explicitly > >>>> set the pgprot value to a specific value. > >>>> > >>>> Of the arches that support MEMORY_HOTPLUG: x86_64, s390 and arm64 is a > >>>> simple change to pass the pgprot_t down to their respective functions > >>>> which set up the page tables. For x86_32, set the page tables explicitly > >>>> using _set_memory_prot() (seeing they are already mapped). For sh, reject > >>>> anything but PAGE_KERNEL settings -- this should be fine, for now, seeing > >>>> sh doesn't support ZONE_DEVICE anyway. > >>>> > >>>> Cc: Dan Williams <dan.j.williams@intel.com> > >>>> Cc: David Hildenbrand <david@redhat.com> > >>>> Cc: Michal Hocko <mhocko@suse.com> > >>>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > >>>> --- > >>>> arch/arm64/mm/mmu.c | 4 ++-- > >>>> arch/ia64/mm/init.c | 5 ++++- > >>>> arch/powerpc/mm/mem.c | 4 ++-- > >>>> arch/s390/mm/init.c | 4 ++-- > >>>> arch/sh/mm/init.c | 5 ++++- > >>>> arch/x86/mm/init_32.c | 7 ++++++- > >>>> arch/x86/mm/init_64.c | 4 ++-- > >>>> include/linux/memory_hotplug.h | 2 +- > >>>> mm/memory_hotplug.c | 2 +- > >>>> mm/memremap.c | 2 +- > >>>> 10 files changed, 25 insertions(+), 14 deletions(-) > >>>> > >>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > >>>> index 60c929f3683b..48b65272df15 100644 > >>>> --- a/arch/arm64/mm/mmu.c > >>>> +++ b/arch/arm64/mm/mmu.c > >>>> @@ -1050,7 +1050,7 @@ int p4d_free_pud_page(p4d_t *p4d, unsigned long addr) > >>>> } > >>>> > >>>> #ifdef CONFIG_MEMORY_HOTPLUG > >>>> -int arch_add_memory(int nid, u64 start, u64 size, > >>>> +int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, > >>>> struct mhp_restrictions *restrictions) > >>> > >>> Can we fiddle that into "struct mhp_restrictions" instead? > >> > >> Yes, if that's what people want, it's pretty trivial to do. I chose not > >> to do it that way because it doesn't get passed down to add_pages() and > >> it's not really a "restriction". If I don't hear any objections, I will > >> do that for v2. > > > > I do agree that restriction is not the best fit. But I consider prot > > argument to complicate the API to all users even though it is not really > > clear whether we are going to have many users really benefiting from it. > > Look at the vmalloc API and try to find how many users of __vmalloc do > > not use PAGE_KERNEL. > > > > So I can see two options. One of them is to add arch_add_memory_prot > > that would allow to have give and extra prot argument or simply call > > an arch independent API to change the protection after arch_add_memory. > > The later sounds like much less code. The memory shouldn't be in use by > > anybody at that stage yet AFAIU. Maybe there even is an API like that. > > Yes, well, we tried something like this by calling set_memory_wc() > inside memremap_pages(); but on large bars (tens of GB) it was too slow > (taking several seconds to complete) and on some hosts actually hit CPU > watchdog errors. Which looks like something to fix independently. > So at the very least we'd have to add some cpu_relax() calls to that > path. And it's also the case that set_memory_wc() is x86 only right now. > So we'd have to create a new general interface to walk and fixup page > tables for all arches. > > But, in my opinion, setting up all those page tables twice is too large > of an overhead and it's better to just add them correctly the first > time. The changes I propose to do this aren't really a lot of code and > probably less than creating a new interface for all arches. OK, fair enough. Then I would suggest going with arch_add_memory_prot then unless there is a wider disagreement witht that.
On Mon 09-12-19 12:43:40, Dan Williams wrote: > On Mon, Dec 9, 2019 at 12:24 PM Logan Gunthorpe <logang@deltatee.com> wrote: > > > > > > > > On 2019-12-09 12:23 p.m., David Hildenbrand wrote: > > > On 09.12.19 20:13, Logan Gunthorpe wrote: [...] > > >> #ifdef CONFIG_MEMORY_HOTPLUG > > >> -int arch_add_memory(int nid, u64 start, u64 size, > > >> +int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, > > >> struct mhp_restrictions *restrictions) > > > > > > Can we fiddle that into "struct mhp_restrictions" instead? > > > > Yes, if that's what people want, it's pretty trivial to do. I chose not > > to do it that way because it doesn't get passed down to add_pages() and > > it's not really a "restriction". If I don't hear any objections, I will > > do that for v2. > > +1 to storing this information alongside the altmap in that structure. > However, I agree struct mhp_restrictions, with the MHP_MEMBLOCK_API > flag now gone, has lost all of its "restrictions". How about dropping > the 'flags' property and renaming the struct to 'struct > mhp_modifiers'? Hmm, this email somehow didn't end up in my inbox so I have missed it before replying. Well, mhp_modifiers makes some sense and it would reduce the API proliferation but how do you expect the prot part to be handled? I really do not want people to think about PAGE_KERNEL or which protection to use because my experience tells that this will get copied without much thinking or simply will break with some odd usecases. So how exactly this would be used?
On 10.12.19 11:04, Michal Hocko wrote: > On Mon 09-12-19 12:43:40, Dan Williams wrote: >> On Mon, Dec 9, 2019 at 12:24 PM Logan Gunthorpe <logang@deltatee.com> wrote: >>> >>> >>> >>> On 2019-12-09 12:23 p.m., David Hildenbrand wrote: >>>> On 09.12.19 20:13, Logan Gunthorpe wrote: > [...] >>>>> #ifdef CONFIG_MEMORY_HOTPLUG >>>>> -int arch_add_memory(int nid, u64 start, u64 size, >>>>> +int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, >>>>> struct mhp_restrictions *restrictions) >>>> >>>> Can we fiddle that into "struct mhp_restrictions" instead? >>> >>> Yes, if that's what people want, it's pretty trivial to do. I chose not >>> to do it that way because it doesn't get passed down to add_pages() and >>> it's not really a "restriction". If I don't hear any objections, I will >>> do that for v2. >> >> +1 to storing this information alongside the altmap in that structure. >> However, I agree struct mhp_restrictions, with the MHP_MEMBLOCK_API >> flag now gone, has lost all of its "restrictions". How about dropping >> the 'flags' property and renaming the struct to 'struct >> mhp_modifiers'? > > Hmm, this email somehow didn't end up in my inbox so I have missed it > before replying. > > Well, mhp_modifiers makes some sense and it would reduce the API > proliferation but how do you expect the prot part to be handled? > I really do not want people to think about PAGE_KERNEL or which > protection to use because my experience tells that this will get copied > without much thinking or simply will break with some odd usecases. > So how exactly this would be used? I was thinking about exactly the same "issue". 1. default initialization via a function memhp_modifier_default_init(&modified); 2. a flag that unlocks the prot field (default:0). Without the flag, it is ignored. We can keep the current initialization then. Other ideas?
On Tue 10-12-19 11:09:46, David Hildenbrand wrote: > On 10.12.19 11:04, Michal Hocko wrote: > > On Mon 09-12-19 12:43:40, Dan Williams wrote: > >> On Mon, Dec 9, 2019 at 12:24 PM Logan Gunthorpe <logang@deltatee.com> wrote: > >>> > >>> > >>> > >>> On 2019-12-09 12:23 p.m., David Hildenbrand wrote: > >>>> On 09.12.19 20:13, Logan Gunthorpe wrote: > > [...] > >>>>> #ifdef CONFIG_MEMORY_HOTPLUG > >>>>> -int arch_add_memory(int nid, u64 start, u64 size, > >>>>> +int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, > >>>>> struct mhp_restrictions *restrictions) > >>>> > >>>> Can we fiddle that into "struct mhp_restrictions" instead? > >>> > >>> Yes, if that's what people want, it's pretty trivial to do. I chose not > >>> to do it that way because it doesn't get passed down to add_pages() and > >>> it's not really a "restriction". If I don't hear any objections, I will > >>> do that for v2. > >> > >> +1 to storing this information alongside the altmap in that structure. > >> However, I agree struct mhp_restrictions, with the MHP_MEMBLOCK_API > >> flag now gone, has lost all of its "restrictions". How about dropping > >> the 'flags' property and renaming the struct to 'struct > >> mhp_modifiers'? > > > > Hmm, this email somehow didn't end up in my inbox so I have missed it > > before replying. > > > > Well, mhp_modifiers makes some sense and it would reduce the API > > proliferation but how do you expect the prot part to be handled? > > I really do not want people to think about PAGE_KERNEL or which > > protection to use because my experience tells that this will get copied > > without much thinking or simply will break with some odd usecases. > > So how exactly this would be used? > > I was thinking about exactly the same "issue". > > 1. default initialization via a function > > memhp_modifier_default_init(&modified); > > 2. a flag that unlocks the prot field (default:0). Without the flag, it > is ignored. We can keep the current initialization then. > > Other ideas? 3. a prot mask to apply on top of PAGE_KERNEL? Or would that be insufficient/clumsy?
On 10.12.19 11:34, Michal Hocko wrote: > On Tue 10-12-19 11:09:46, David Hildenbrand wrote: >> On 10.12.19 11:04, Michal Hocko wrote: >>> On Mon 09-12-19 12:43:40, Dan Williams wrote: >>>> On Mon, Dec 9, 2019 at 12:24 PM Logan Gunthorpe <logang@deltatee.com> wrote: >>>>> >>>>> >>>>> >>>>> On 2019-12-09 12:23 p.m., David Hildenbrand wrote: >>>>>> On 09.12.19 20:13, Logan Gunthorpe wrote: >>> [...] >>>>>>> #ifdef CONFIG_MEMORY_HOTPLUG >>>>>>> -int arch_add_memory(int nid, u64 start, u64 size, >>>>>>> +int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, >>>>>>> struct mhp_restrictions *restrictions) >>>>>> >>>>>> Can we fiddle that into "struct mhp_restrictions" instead? >>>>> >>>>> Yes, if that's what people want, it's pretty trivial to do. I chose not >>>>> to do it that way because it doesn't get passed down to add_pages() and >>>>> it's not really a "restriction". If I don't hear any objections, I will >>>>> do that for v2. >>>> >>>> +1 to storing this information alongside the altmap in that structure. >>>> However, I agree struct mhp_restrictions, with the MHP_MEMBLOCK_API >>>> flag now gone, has lost all of its "restrictions". How about dropping >>>> the 'flags' property and renaming the struct to 'struct >>>> mhp_modifiers'? >>> >>> Hmm, this email somehow didn't end up in my inbox so I have missed it >>> before replying. >>> >>> Well, mhp_modifiers makes some sense and it would reduce the API >>> proliferation but how do you expect the prot part to be handled? >>> I really do not want people to think about PAGE_KERNEL or which >>> protection to use because my experience tells that this will get copied >>> without much thinking or simply will break with some odd usecases. >>> So how exactly this would be used? >> >> I was thinking about exactly the same "issue". >> >> 1. default initialization via a function >> >> memhp_modifier_default_init(&modified); >> >> 2. a flag that unlocks the prot field (default:0). Without the flag, it >> is ignored. We can keep the current initialization then. >> >> Other ideas? > > 3. a prot mask to apply on top of PAGE_KERNEL? Or would that be > insufficient/clumsy? > If it works for the given use case, I guess this would be simple and ok.
On 2019-12-10 4:25 a.m., David Hildenbrand wrote: > On 10.12.19 11:34, Michal Hocko wrote: >> On Tue 10-12-19 11:09:46, David Hildenbrand wrote: >>> On 10.12.19 11:04, Michal Hocko wrote: >>>> On Mon 09-12-19 12:43:40, Dan Williams wrote: >>>>> On Mon, Dec 9, 2019 at 12:24 PM Logan Gunthorpe <logang@deltatee.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 2019-12-09 12:23 p.m., David Hildenbrand wrote: >>>>>>> On 09.12.19 20:13, Logan Gunthorpe wrote: >>>> [...] >>>>>>>> #ifdef CONFIG_MEMORY_HOTPLUG >>>>>>>> -int arch_add_memory(int nid, u64 start, u64 size, >>>>>>>> +int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, >>>>>>>> struct mhp_restrictions *restrictions) >>>>>>> >>>>>>> Can we fiddle that into "struct mhp_restrictions" instead? >>>>>> >>>>>> Yes, if that's what people want, it's pretty trivial to do. I chose not >>>>>> to do it that way because it doesn't get passed down to add_pages() and >>>>>> it's not really a "restriction". If I don't hear any objections, I will >>>>>> do that for v2. >>>>> >>>>> +1 to storing this information alongside the altmap in that structure. >>>>> However, I agree struct mhp_restrictions, with the MHP_MEMBLOCK_API >>>>> flag now gone, has lost all of its "restrictions". How about dropping >>>>> the 'flags' property and renaming the struct to 'struct >>>>> mhp_modifiers'? >>>> >>>> Hmm, this email somehow didn't end up in my inbox so I have missed it >>>> before replying. >>>> >>>> Well, mhp_modifiers makes some sense and it would reduce the API >>>> proliferation but how do you expect the prot part to be handled? >>>> I really do not want people to think about PAGE_KERNEL or which >>>> protection to use because my experience tells that this will get copied >>>> without much thinking or simply will break with some odd usecases. >>>> So how exactly this would be used? >>> >>> I was thinking about exactly the same "issue". >>> >>> 1. default initialization via a function >>> >>> memhp_modifier_default_init(&modified); >>> >>> 2. a flag that unlocks the prot field (default:0). Without the flag, it >>> is ignored. We can keep the current initialization then. >>> >>> Other ideas? >> >> 3. a prot mask to apply on top of PAGE_KERNEL? Or would that be >> insufficient/clumsy? >> > > If it works for the given use case, I guess this would be simple and ok. I don't see how we can do that without a ton of work. The pgport_t is architecture specific so we'd need to add mask functions to every architecture for every page cache type we need to use. I don't think that's a good idea. I think I slightly prefer option 2 over the above. But I'd actually prefer callers have to think about the caching type seeing when we grew the second user (memremap_pages()) and it was paired with track_pfn_remap(), it was actually subtly wrong because it could create a mapping that track_pfn_remap() disagreed with. In fact, PAGE_KERNEL has already been specified in memremap_pages() for ages, it was just ignored when it got to the arch_add_memory() step which is were this issue comes from. In my opinion, having a coder and reviewer see PAGE_KERNEL and ask if that makes sense is a benefit. Having it hidden because we don't want people to think about it is worse, harder to understand and results in bugs that are more difficult to spot. Though, we may be overthinking this: arch_add_memory() is a low level non-exported API that's currently used in exactly two places. I don't think there's going to be many, if any, valid new use cases coming up for it in the future. That's more what memremap_pages() is for. Logan
On Tue 10-12-19 16:52:31, Logan Gunthorpe wrote: [...] > In my opinion, having a coder and reviewer see PAGE_KERNEL and ask if > that makes sense is a benefit. Having it hidden because we don't want > people to think about it is worse, harder to understand and results in > bugs that are more difficult to spot. My experience would disagree here. We have several examples in the MM where an overly complex and versatile APIs led to suble bugs, a lot of copy&pasting and cargo cult programing (just look at the page allocator as a shiny example - e.g. gfp_flags). So I am always trying to be carefull here. > Though, we may be overthinking this: arch_add_memory() is a low level > non-exported API that's currently used in exactly two places. This is a fair argument. Most users are and should be using add_memory(). > I don't > think there's going to be many, if any, valid new use cases coming up > for it in the future. That's more what memremap_pages() is for. OK, fair enough. If this is indeed the simplest way forward then I will not stand in the way.
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c index 60c929f3683b..48b65272df15 100644 --- a/arch/arm64/mm/mmu.c +++ b/arch/arm64/mm/mmu.c @@ -1050,7 +1050,7 @@ int p4d_free_pud_page(p4d_t *p4d, unsigned long addr) } #ifdef CONFIG_MEMORY_HOTPLUG -int arch_add_memory(int nid, u64 start, u64 size, +int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, struct mhp_restrictions *restrictions) { int flags = 0; @@ -1059,7 +1059,7 @@ int arch_add_memory(int nid, u64 start, u64 size, flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS; __create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start), - size, PAGE_KERNEL, __pgd_pgtable_alloc, flags); + size, prot, __pgd_pgtable_alloc, flags); return __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT, restrictions); diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c index bf9df2625bc8..15a1efcecd83 100644 --- a/arch/ia64/mm/init.c +++ b/arch/ia64/mm/init.c @@ -669,13 +669,16 @@ mem_init (void) } #ifdef CONFIG_MEMORY_HOTPLUG -int arch_add_memory(int nid, u64 start, u64 size, +int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, struct mhp_restrictions *restrictions) { unsigned long start_pfn = start >> PAGE_SHIFT; unsigned long nr_pages = size >> PAGE_SHIFT; int ret; + if (prot != PAGE_KERNEL) + return -EINVAL; + ret = __add_pages(nid, start_pfn, nr_pages, restrictions); if (ret) printk("%s: Problem encountered in __add_pages() as ret=%d\n", diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c index 22525d8935ce..a901c2b65801 100644 --- a/arch/powerpc/mm/mem.c +++ b/arch/powerpc/mm/mem.c @@ -105,7 +105,7 @@ int __weak remove_section_mapping(unsigned long start, unsigned long end) return -ENODEV; } -int __ref arch_add_memory(int nid, u64 start, u64 size, +int __ref arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, struct mhp_restrictions *restrictions) { unsigned long start_pfn = start >> PAGE_SHIFT; @@ -115,7 +115,7 @@ int __ref arch_add_memory(int nid, u64 start, u64 size, resize_hpt_for_hotplug(memblock_phys_mem_size()); start = (unsigned long)__va(start); - rc = create_section_mapping(start, start + size, nid, PAGE_KERNEL); + rc = create_section_mapping(start, start + size, nid, prot); if (rc) { pr_warn("Unable to create mapping for hot added memory 0x%llx..0x%llx: %d\n", start, start + size, rc); diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c index 263ebb074cdd..d3a67d8a1317 100644 --- a/arch/s390/mm/init.c +++ b/arch/s390/mm/init.c @@ -266,7 +266,7 @@ device_initcall(s390_cma_mem_init); #endif /* CONFIG_CMA */ -int arch_add_memory(int nid, u64 start, u64 size, +int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, struct mhp_restrictions *restrictions) { unsigned long start_pfn = PFN_DOWN(start); @@ -276,7 +276,7 @@ int arch_add_memory(int nid, u64 start, u64 size, if (WARN_ON_ONCE(restrictions->altmap)) return -EINVAL; - rc = vmem_add_mapping(start, size, PAGE_KERNEL); + rc = vmem_add_mapping(start, size, prot); if (rc) return rc; diff --git a/arch/sh/mm/init.c b/arch/sh/mm/init.c index dfdbaa50946e..cf9f788115ff 100644 --- a/arch/sh/mm/init.c +++ b/arch/sh/mm/init.c @@ -405,13 +405,16 @@ void __init mem_init(void) } #ifdef CONFIG_MEMORY_HOTPLUG -int arch_add_memory(int nid, u64 start, u64 size, +int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, struct mhp_restrictions *restrictions) { unsigned long start_pfn = PFN_DOWN(start); unsigned long nr_pages = size >> PAGE_SHIFT; int ret; + if (prot != PAGE_KERNEL) + return -EINVAL; + /* We only have ZONE_NORMAL, so this is easy.. */ ret = __add_pages(nid, start_pfn, nr_pages, restrictions); if (unlikely(ret)) diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c index d3cdd9137f42..c0fe624eb304 100644 --- a/arch/x86/mm/init_32.c +++ b/arch/x86/mm/init_32.c @@ -852,11 +852,16 @@ void __init mem_init(void) } #ifdef CONFIG_MEMORY_HOTPLUG -int arch_add_memory(int nid, u64 start, u64 size, +int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, struct mhp_restrictions *restrictions) { unsigned long start_pfn = start >> PAGE_SHIFT; unsigned long nr_pages = size >> PAGE_SHIFT; + int ret; + + ret = _set_memory_prot(start, nr_pages, prot); + if (ret) + return ret; return __add_pages(nid, start_pfn, nr_pages, restrictions); } diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c index 65a5093ec97b..c7d170d67b57 100644 --- a/arch/x86/mm/init_64.c +++ b/arch/x86/mm/init_64.c @@ -862,13 +862,13 @@ int add_pages(int nid, unsigned long start_pfn, unsigned long nr_pages, return ret; } -int arch_add_memory(int nid, u64 start, u64 size, +int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, struct mhp_restrictions *restrictions) { unsigned long start_pfn = start >> PAGE_SHIFT; unsigned long nr_pages = size >> PAGE_SHIFT; - init_memory_mapping(start, start + size, PAGE_KERNEL); + init_memory_mapping(start, start + size, prot); return add_pages(nid, start_pfn, nr_pages, restrictions); } diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h index f46ea71b4ffd..82e8b3fcebab 100644 --- a/include/linux/memory_hotplug.h +++ b/include/linux/memory_hotplug.h @@ -111,7 +111,7 @@ extern void __online_page_free(struct page *page); extern int try_online_node(int nid); -extern int arch_add_memory(int nid, u64 start, u64 size, +extern int arch_add_memory(int nid, u64 start, u64 size, pgprot_t prot, struct mhp_restrictions *restrictions); extern u64 max_mem_size; diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index df570e5c71cc..0a581a344a00 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -1035,7 +1035,7 @@ int __ref add_memory_resource(int nid, struct resource *res) new_node = ret; /* call arch's memory hotadd */ - ret = arch_add_memory(nid, start, size, &restrictions); + ret = arch_add_memory(nid, start, size, PAGE_KERNEL, &restrictions); if (ret < 0) goto error; diff --git a/mm/memremap.c b/mm/memremap.c index 03ccbdfeb697..4edcca074e15 100644 --- a/mm/memremap.c +++ b/mm/memremap.c @@ -281,7 +281,7 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid) } error = arch_add_memory(nid, res->start, resource_size(res), - &restrictions); + pgprot, &restrictions); } if (!error) {
devm_memremap_pages() is currently used by the PCI P2PDMA code to create struct page mappings for IO memory. At present, these mappings are created with PAGE_KERNEL which implies setting the PAT bits to be WB. However, on x86, an mtrr register will typically override this and force the cache type to be UC-. In the case firmware doesn't set this register it is effectively WB and will typically result in a machine check exception when it's accessed. Other arches are not currently likely to function correctly seeing they don't have any MTRR registers to fall back on. To solve this, add an argument to arch_add_memory() to explicitly set the pgprot value to a specific value. Of the arches that support MEMORY_HOTPLUG: x86_64, s390 and arm64 is a simple change to pass the pgprot_t down to their respective functions which set up the page tables. For x86_32, set the page tables explicitly using _set_memory_prot() (seeing they are already mapped). For sh, reject anything but PAGE_KERNEL settings -- this should be fine, for now, seeing sh doesn't support ZONE_DEVICE anyway. Cc: Dan Williams <dan.j.williams@intel.com> Cc: David Hildenbrand <david@redhat.com> Cc: Michal Hocko <mhocko@suse.com> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> --- arch/arm64/mm/mmu.c | 4 ++-- arch/ia64/mm/init.c | 5 ++++- arch/powerpc/mm/mem.c | 4 ++-- arch/s390/mm/init.c | 4 ++-- arch/sh/mm/init.c | 5 ++++- arch/x86/mm/init_32.c | 7 ++++++- arch/x86/mm/init_64.c | 4 ++-- include/linux/memory_hotplug.h | 2 +- mm/memory_hotplug.c | 2 +- mm/memremap.c | 2 +- 10 files changed, 25 insertions(+), 14 deletions(-)