| Message ID | 20190910160903.65694-4-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Read-only memremap() | expand |
On Tue, Sep 10, 2019 at 9:09 AM Stephen Boyd <swboyd@chromium.org> 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 | 20 +++++++++++++++++--- > 2 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/include/linux/io.h b/include/linux/io.h > index accac822336a..15a63efcd153 100644 > --- a/include/linux/io.h > +++ b/include/linux/io.h > @@ -148,6 +148,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 62c92e43aa0d..6d76b7398714 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) > @@ -45,7 +52,7 @@ static void *try_ram_remap(resource_size_t offset, size_t size, > * @offset: iomem resource start address > * @size: size of remap > * @flags: any of MEMREMAP_WB, MEMREMAP_WT, MEMREMAP_WC, > - * MEMREMAP_ENC, MEMREMAP_DEC > + * MEMREMAP_ENC, MEMREMAP_DEC, MEMREMAP_RO > * > * memremap() is "ioremap" for cases where it is known that the resource > * being mapped does not have i/o side effects and the __iomem > @@ -53,6 +60,9 @@ static void *try_ram_remap(resource_size_t offset, size_t size, > * mapping types will be attempted in the order listed below until one of > * them succeeds. > * > + * MEMREMAP_RO - establish a mapping whereby writes are ignored/rejected. > + * Attempts to map System RAM with this mapping type will fail. Why should attempts to map RAM with this flag fail? MEMREMAP_WB will allow RAM and quietly give you back the direct mapping, so it seems like at least some values in this function allow RAM. Oh, I see a comment below about "Enforce that this mapping is not aliasing System RAM". I guess this is worried about cache coloring? But is that a problem with RO mappings? I guess the RO mappings could get partially stale, so if the memory were being updated out from under you, you might see some updates but not others. Was that the rationale? > + * > * MEMREMAP_WB - matches the default mapping for System RAM on > * the architecture. This is usually a read-allocate write-back cache. > * Moreover, if MEMREMAP_WB is specified and the requested remap region is RAM > @@ -84,7 +94,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 +116,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)) { Isn't this condition always true? Did you mean flags != MEM_REMAP_WB && flags != MEMREMAP_RO? > WARN_ONCE(1, "memremap attempted on ram %pa size: %#lx\n", > &offset, (unsigned long) size); > return NULL; > -- > Sent by a computer through tubes >
Quoting Evan Green (2019-09-18 12:37:34) > On Tue, Sep 10, 2019 at 9:09 AM Stephen Boyd <swboyd@chromium.org> wrote: > > > > @@ -53,6 +60,9 @@ static void *try_ram_remap(resource_size_t offset, size_t size, > > * mapping types will be attempted in the order listed below until one of > > * them succeeds. > > * > > + * MEMREMAP_RO - establish a mapping whereby writes are ignored/rejected. > > + * Attempts to map System RAM with this mapping type will fail. > > Why should attempts to map RAM with this flag fail? MEMREMAP_WB will > allow RAM and quietly give you back the direct mapping, so it seems > like at least some values in this function allow RAM. > > Oh, I see a comment below about "Enforce that this mapping is not > aliasing System RAM". I guess this is worried about cache coloring? > But is that a problem with RO mappings? I guess the RO mappings could > get partially stale, so if the memory were being updated out from > under you, you might see some updates but not others. Was that the > rationale? Will Deacon, Dan Williams, and I talked about this RO flag at LPC and I believe we decided to mostly get rid of the flags argument to this function. The vast majority of callers pass MEMREMAP_WB, so I'll just make that be the implementation default and support the flags for encrpytion (MEMREMAP_ENC and MEMREMAP_DEC). There are a few callers that pass MEMREMAP_WC or MEMREMAP_WT (and one that passes all of them), but I believe those can be changed to MEMREMAP_WB and not care. There's also the efi framebuffer code that matches the memory attributes in the EFI memory map. I'm not sure what to do with that one to be quite honest. Maybe EFI shouldn't care and just use whatever is already there in the mapping? Either way, I'll introduce a memremap_ro() API that maps memory as read only if possible and return a const void pointer as well. I'm debating making that API fallback to memremap() if RO isn't supported for some reason or can't work because we're remapping system memory but that seems a little too nice when the caller could just as well decide to fail if memory can't be mapped as read only.
On Thu, Oct 3, 2019 at 11:56 AM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Evan Green (2019-09-18 12:37:34) > > On Tue, Sep 10, 2019 at 9:09 AM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > @@ -53,6 +60,9 @@ static void *try_ram_remap(resource_size_t offset, size_t size, > > > * mapping types will be attempted in the order listed below until one of > > > * them succeeds. > > > * > > > + * MEMREMAP_RO - establish a mapping whereby writes are ignored/rejected. > > > + * Attempts to map System RAM with this mapping type will fail. > > > > Why should attempts to map RAM with this flag fail? MEMREMAP_WB will > > allow RAM and quietly give you back the direct mapping, so it seems > > like at least some values in this function allow RAM. > > > > Oh, I see a comment below about "Enforce that this mapping is not > > aliasing System RAM". I guess this is worried about cache coloring? > > But is that a problem with RO mappings? I guess the RO mappings could > > get partially stale, so if the memory were being updated out from > > under you, you might see some updates but not others. Was that the > > rationale? > > Will Deacon, Dan Williams, and I talked about this RO flag at LPC and I > believe we decided to mostly get rid of the flags argument to this > function. The vast majority of callers pass MEMREMAP_WB, so I'll just > make that be the implementation default and support the flags for > encrpytion (MEMREMAP_ENC and MEMREMAP_DEC). There are a few callers that > pass MEMREMAP_WC or MEMREMAP_WT (and one that passes all of them), but I > believe those can be changed to MEMREMAP_WB and not care. There's also > the efi framebuffer code that matches the memory attributes in the EFI > memory map. I'm not sure what to do with that one to be quite honest. > Maybe EFI shouldn't care and just use whatever is already there in the > mapping? I would guess that the folks mapping things like framebuffers would care if their write-combined memory were changed to writeback. But I suppose the better authorities on that are already here, so if they think it's fine, I guess it's all good. Whatever logic is used to defend that would likely apply equally well to the EFI mappings. > > Either way, I'll introduce a memremap_ro() API that maps memory as read > only if possible and return a const void pointer as well. I'm debating > making that API fallback to memremap() if RO isn't supported for some > reason or can't work because we're remapping system memory but that > seems a little too nice when the caller could just as well decide to > fail if memory can't be mapped as read only. Sounds good. My small vote would be for the nicer fallback to memremap(). I can't think of a case where someone would rather not have their memory mapped at all than have it mapped writeable. -Evan
Hi, On Thu, Oct 03, 2019 at 01:05:53PM -0700, Evan Green wrote: > On Thu, Oct 3, 2019 at 11:56 AM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Evan Green (2019-09-18 12:37:34) > > > On Tue, Sep 10, 2019 at 9:09 AM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > > > @@ -53,6 +60,9 @@ static void *try_ram_remap(resource_size_t offset, size_t size, > > > > * mapping types will be attempted in the order listed below until one of > > > > * them succeeds. > > > > * > > > > + * MEMREMAP_RO - establish a mapping whereby writes are ignored/rejected. > > > > + * Attempts to map System RAM with this mapping type will fail. > > > > > > Why should attempts to map RAM with this flag fail? MEMREMAP_WB will > > > allow RAM and quietly give you back the direct mapping, so it seems > > > like at least some values in this function allow RAM. > > > > > > Oh, I see a comment below about "Enforce that this mapping is not > > > aliasing System RAM". I guess this is worried about cache coloring? > > > But is that a problem with RO mappings? I guess the RO mappings could > > > get partially stale, so if the memory were being updated out from > > > under you, you might see some updates but not others. Was that the > > > rationale? > > > > Will Deacon, Dan Williams, and I talked about this RO flag at LPC and I > > believe we decided to mostly get rid of the flags argument to this > > function. The vast majority of callers pass MEMREMAP_WB, so I'll just > > make that be the implementation default and support the flags for > > encrpytion (MEMREMAP_ENC and MEMREMAP_DEC). There are a few callers that > > pass MEMREMAP_WC or MEMREMAP_WT (and one that passes all of them), but I > > believe those can be changed to MEMREMAP_WB and not care. There's also > > the efi framebuffer code that matches the memory attributes in the EFI > > memory map. I'm not sure what to do with that one to be quite honest. > > Maybe EFI shouldn't care and just use whatever is already there in the > > mapping? > > I would guess that the folks mapping things like framebuffers would > care if their write-combined memory were changed to writeback. But I > suppose the better authorities on that are already here, so if they > think it's fine, I guess it's all good. Drive-by comment as this happened to hit one of my hotword filters: dma_init_coherent_memory() uses MEMREMAP_WC, and I think MEMREMAP_WB would be quite unsuitable there. As you say, we use that for framebuffers. Thanks, -Brian > > Whatever logic is used to defend that would likely apply equally well > to the EFI mappings. > > > > > Either way, I'll introduce a memremap_ro() API that maps memory as read > > only if possible and return a const void pointer as well. I'm debating > > making that API fallback to memremap() if RO isn't supported for some > > reason or can't work because we're remapping system memory but that > > seems a little too nice when the caller could just as well decide to > > fail if memory can't be mapped as read only. > > Sounds good. My small vote would be for the nicer fallback to > memremap(). I can't think of a case where someone would rather not > have their memory mapped at all than have it mapped writeable. > -Evan > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/include/linux/io.h b/include/linux/io.h index accac822336a..15a63efcd153 100644 --- a/include/linux/io.h +++ b/include/linux/io.h @@ -148,6 +148,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 62c92e43aa0d..6d76b7398714 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) @@ -45,7 +52,7 @@ static void *try_ram_remap(resource_size_t offset, size_t size, * @offset: iomem resource start address * @size: size of remap * @flags: any of MEMREMAP_WB, MEMREMAP_WT, MEMREMAP_WC, - * MEMREMAP_ENC, MEMREMAP_DEC + * MEMREMAP_ENC, MEMREMAP_DEC, MEMREMAP_RO * * memremap() is "ioremap" for cases where it is known that the resource * being mapped does not have i/o side effects and the __iomem @@ -53,6 +60,9 @@ static void *try_ram_remap(resource_size_t offset, size_t size, * mapping types will be attempted in the order listed below until one of * them succeeds. * + * MEMREMAP_RO - establish a mapping whereby writes are ignored/rejected. + * Attempts to map System RAM with this mapping type will fail. + * * MEMREMAP_WB - matches the default mapping for System RAM on * the architecture. This is usually a read-allocate write-back cache. * Moreover, if MEMREMAP_WB is specified and the requested remap region is RAM @@ -84,7 +94,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 +116,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;