| Message ID | 20190614203717.75479-4-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Read-only memremap() | expand |
On Fri, Jun 14, 2019 at 01:37:15PM -0700, Stephen Boyd wrote: > Sometimes we have memories that are supposed to be read-only, but when > we map these regions the best we can do is map them as write-back with > MEMREMAP_WB. Introduce a read-only memory mapping (MEMREMAP_RO) that > allows us to map reserved memory regions as read-only. This way, we're > less likely to see these special memory regions become corrupted by > stray writes to them. > > Cc: Evan Green <evgreen@chromium.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > Cc: Andy Gross <agross@kernel.org> > Cc: Will Deacon <will.deacon@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > --- > include/linux/io.h | 1 + > kernel/iomem.c | 15 +++++++++++++-- > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/include/linux/io.h b/include/linux/io.h > index 32e30e8fb9db..16c7f4498869 100644 > --- a/include/linux/io.h > +++ b/include/linux/io.h > @@ -159,6 +159,7 @@ enum { > MEMREMAP_WC = 1 << 2, > MEMREMAP_ENC = 1 << 3, > MEMREMAP_DEC = 1 << 4, > + MEMREMAP_RO = 1 << 5, > }; > > void *memremap(resource_size_t offset, size_t size, unsigned long flags); > diff --git a/kernel/iomem.c b/kernel/iomem.c > index 93c264444510..10d5ef0ff09e 100644 > --- a/kernel/iomem.c > +++ b/kernel/iomem.c > @@ -19,6 +19,13 @@ static void *arch_memremap_wb(resource_size_t offset, unsigned long size) > } > #endif > > +#ifndef arch_memremap_ro > +static void *arch_memremap_ro(resource_size_t offset, unsigned long size) > +{ > + return NULL; > +} > +#endif > + > #ifndef arch_memremap_can_ram_remap > static bool arch_memremap_can_ram_remap(resource_size_t offset, size_t size, > unsigned long flags) > @@ -84,7 +91,10 @@ void *memremap(resource_size_t offset, size_t size, unsigned long flags) > } > > /* Try all mapping types requested until one returns non-NULL */ > - if (flags & MEMREMAP_WB) { > + if ((flags & MEMREMAP_RO) && is_ram != REGION_INTERSECTS) > + addr = arch_memremap_ro(offset, size); > + > + if (!addr && (flags & MEMREMAP_WB)) { > /* > * MEMREMAP_WB is special in that it can be satisfied > * from the direct map. Some archs depend on the > @@ -103,7 +113,8 @@ void *memremap(resource_size_t offset, size_t size, unsigned long flags) > * address mapping. Enforce that this mapping is not aliasing > * System RAM. > */ > - if (!addr && is_ram == REGION_INTERSECTS && flags != MEMREMAP_WB) { > + if (!addr && is_ram == REGION_INTERSECTS && > + (flags != MEMREMAP_WB || flags != MEMREMAP_RO)) { > WARN_ONCE(1, "memremap attempted on ram %pa size: %#lx\n", > &offset, (unsigned long) size); > return NULL; This function seems a little confused about whether 'flags' is really a bitmap of flags, or whether it is equal to exactly one entry in the enum. Given that I think it's sensible for somebody to specify 'MEMREMAP_RO | MEMREMAP_WT', then we probably need to start checking these things a bit more thoroughly so we can reject unsupported combinations at the very least. Will
Quoting Will Deacon (2019-07-10 07:14:34) > On Fri, Jun 14, 2019 at 01:37:15PM -0700, Stephen Boyd wrote: > > @@ -84,7 +91,10 @@ void *memremap(resource_size_t offset, size_t size, unsigned long flags) > > } > > > > /* Try all mapping types requested until one returns non-NULL */ > > - if (flags & MEMREMAP_WB) { > > + if ((flags & MEMREMAP_RO) && is_ram != REGION_INTERSECTS) > > + addr = arch_memremap_ro(offset, size); > > + > > + if (!addr && (flags & MEMREMAP_WB)) { > > /* > > * MEMREMAP_WB is special in that it can be satisfied > > * from the direct map. Some archs depend on the > > @@ -103,7 +113,8 @@ void *memremap(resource_size_t offset, size_t size, unsigned long flags) > > * address mapping. Enforce that this mapping is not aliasing > > * System RAM. > > */ > > - if (!addr && is_ram == REGION_INTERSECTS && flags != MEMREMAP_WB) { > > + if (!addr && is_ram == REGION_INTERSECTS && > > + (flags != MEMREMAP_WB || flags != MEMREMAP_RO)) { > > WARN_ONCE(1, "memremap attempted on ram %pa size: %#lx\n", > > &offset, (unsigned long) size); > > return NULL; > > This function seems a little confused about whether 'flags' is really a > bitmap of flags, or whether it is equal to exactly one entry in the enum. > Given that I think it's sensible for somebody to specify 'MEMREMAP_RO | > MEMREMAP_WT', then we probably need to start checking these things a bit > more thoroughly so we can reject unsupported combinations at the very least. > I'm also confused about the same thing. I thought it was a "getting worse via best effort" type of thing based on the comment above the function. * In the case of multiple flags, the different * mapping types will be attempted in the order listed below until one of * them succeeds. (I now realize I should have documented the new flag so that this order would be known. I'll resend this series again with the documentation fix.) I also thought that the combination of read-only and write through would be OK because the flags are more of a best effort approach to making a mapping. Given that, is there anything to reject? Or do we just keep trying until we can't try anymore?
diff --git a/include/linux/io.h b/include/linux/io.h index 32e30e8fb9db..16c7f4498869 100644 --- a/include/linux/io.h +++ b/include/linux/io.h @@ -159,6 +159,7 @@ enum { MEMREMAP_WC = 1 << 2, MEMREMAP_ENC = 1 << 3, MEMREMAP_DEC = 1 << 4, + MEMREMAP_RO = 1 << 5, }; void *memremap(resource_size_t offset, size_t size, unsigned long flags); diff --git a/kernel/iomem.c b/kernel/iomem.c index 93c264444510..10d5ef0ff09e 100644 --- a/kernel/iomem.c +++ b/kernel/iomem.c @@ -19,6 +19,13 @@ static void *arch_memremap_wb(resource_size_t offset, unsigned long size) } #endif +#ifndef arch_memremap_ro +static void *arch_memremap_ro(resource_size_t offset, unsigned long size) +{ + return NULL; +} +#endif + #ifndef arch_memremap_can_ram_remap static bool arch_memremap_can_ram_remap(resource_size_t offset, size_t size, unsigned long flags) @@ -84,7 +91,10 @@ void *memremap(resource_size_t offset, size_t size, unsigned long flags) } /* Try all mapping types requested until one returns non-NULL */ - if (flags & MEMREMAP_WB) { + if ((flags & MEMREMAP_RO) && is_ram != REGION_INTERSECTS) + addr = arch_memremap_ro(offset, size); + + if (!addr && (flags & MEMREMAP_WB)) { /* * MEMREMAP_WB is special in that it can be satisfied * from the direct map. Some archs depend on the @@ -103,7 +113,8 @@ void *memremap(resource_size_t offset, size_t size, unsigned long flags) * address mapping. Enforce that this mapping is not aliasing * System RAM. */ - if (!addr && is_ram == REGION_INTERSECTS && flags != MEMREMAP_WB) { + if (!addr && is_ram == REGION_INTERSECTS && + (flags != MEMREMAP_WB || flags != MEMREMAP_RO)) { WARN_ONCE(1, "memremap attempted on ram %pa size: %#lx\n", &offset, (unsigned long) size); return NULL;