On 27.03.2024 10:04 PM, Volodymyr Babchuk wrote:
>
> Hi Konrad,
>
> Konrad Dybcio <[email protected]> writes:
>
>> On 27.03.2024 9:09 PM, Volodymyr Babchuk wrote:
>>> It appears that hardware does not like cacheable accesses to this
>>> region. Trying to access this shared memory region as Normal Memory
>>> leads to secure interrupt which causes an endless loop somewhere in
>>> Trust Zone.
>>>
>>> The only reason it is working right now is because Qualcomm Hypervisor
>>> maps the same region as Non-Cacheable memory in Stage 2 translation
>>> tables. The issue manifests if we want to use another hypervisor (like
>>> Xen or KVM), which does not know anything about those specific
>>> mappings. This patch fixes the issue by mapping the shared memory as
>>> Write-Through. This removes dependency on correct mappings in Stage 2
>>> tables.
>>>
>>> I tested this on SA8155P with Xen.
>>>
>>> Signed-off-by: Volodymyr Babchuk <[email protected]>
>>> ---
>>
>> Interesting..
>>
>> +Doug, Rob have you ever seen this on Chrome? (FYI, Volodymyr, chromebooks
>> ship with no qcom hypervisor)
>
> Well, maybe I was wrong when called this thing "hypervisor". All I know
> that it sits in hyp.mbn partition and all what it does is setup EL2
> before switching to EL1 and running UEFI.
>
> In my experiments I replaced contents of hyp.mbn with U-Boot, which gave
> me access to EL2 and I was able to boot Xen and then Linux as Dom0.
Yeah we're talking about the same thing. I was just curious whether
the Chrome folks have heard of it, or whether they have any changes/
workarounds for it.
Konrad
On 27/03/2024 21:06, Konrad Dybcio wrote:
> On 27.03.2024 10:04 PM, Volodymyr Babchuk wrote:
>>
>> Hi Konrad,
>>
>> Konrad Dybcio <[email protected]> writes:
>>
>>> On 27.03.2024 9:09 PM, Volodymyr Babchuk wrote:
>>>> It appears that hardware does not like cacheable accesses to this
>>>> region. Trying to access this shared memory region as Normal Memory
>>>> leads to secure interrupt which causes an endless loop somewhere in
>>>> Trust Zone.
>>>>
>>>> The only reason it is working right now is because Qualcomm Hypervisor
>>>> maps the same region as Non-Cacheable memory in Stage 2 translation
>>>> tables. The issue manifests if we want to use another hypervisor (like
>>>> Xen or KVM), which does not know anything about those specific
>>>> mappings. This patch fixes the issue by mapping the shared memory as
>>>> Write-Through. This removes dependency on correct mappings in Stage 2
>>>> tables.
>>>>
>>>> I tested this on SA8155P with Xen.
>>>>
>>>> Signed-off-by: Volodymyr Babchuk <[email protected]>
>>>> ---
>>>
>>> Interesting..
>>>
>>> +Doug, Rob have you ever seen this on Chrome? (FYI, Volodymyr, chromebooks
>>> ship with no qcom hypervisor)
>>
>> Well, maybe I was wrong when called this thing "hypervisor". All I know
>> that it sits in hyp.mbn partition and all what it does is setup EL2
>> before switching to EL1 and running UEFI.
>>
>> In my experiments I replaced contents of hyp.mbn with U-Boot, which gave
>> me access to EL2 and I was able to boot Xen and then Linux as Dom0.
>
> Yeah we're talking about the same thing. I was just curious whether
> the Chrome folks have heard of it, or whether they have any changes/
> workarounds for it.
Does Linux ever write to this region? Given that the Chromebooks don't
seem to have issues with this (we have a bunch of them in pmOS and I'd
be very very surprised if this was an issue there which nobody had tried
upstreaming before) I'd guess the significant difference here is between
booting Linux in EL2 (as Chromebooks do?) vs with Xen.
Volodymyr: have you tried booting the kernel directly from U-Boot in
EL2? Can you confirm if this issues also happens then?
>
> Konrad
--
// Caleb (they/them)
On Wed, Mar 27, 2024 at 11:29:09PM +0000, Caleb Connolly wrote:
> On 27/03/2024 21:06, Konrad Dybcio wrote:
> > On 27.03.2024 10:04 PM, Volodymyr Babchuk wrote:
> >> Konrad Dybcio <[email protected]> writes:
> >>> On 27.03.2024 9:09 PM, Volodymyr Babchuk wrote:
> >>>> It appears that hardware does not like cacheable accesses to this
> >>>> region. Trying to access this shared memory region as Normal Memory
> >>>> leads to secure interrupt which causes an endless loop somewhere in
> >>>> Trust Zone.
> >>>>
> >>>> The only reason it is working right now is because Qualcomm Hypervisor
> >>>> maps the same region as Non-Cacheable memory in Stage 2 translation
> >>>> tables. The issue manifests if we want to use another hypervisor (like
> >>>> Xen or KVM), which does not know anything about those specific
> >>>> mappings. This patch fixes the issue by mapping the shared memory as
> >>>> Write-Through. This removes dependency on correct mappings in Stage 2
> >>>> tables.
> >>>>
> >>>> I tested this on SA8155P with Xen.
> >>>>
> >>>> Signed-off-by: Volodymyr Babchuk <[email protected]>
> >>>> ---
> >>>
> >>> Interesting..
> >>>
> >>> +Doug, Rob have you ever seen this on Chrome? (FYI, Volodymyr, chromebooks
> >>> ship with no qcom hypervisor)
> >>
> >> Well, maybe I was wrong when called this thing "hypervisor". All I know
> >> that it sits in hyp.mbn partition and all what it does is setup EL2
> >> before switching to EL1 and running UEFI.
> >>
> >> In my experiments I replaced contents of hyp.mbn with U-Boot, which gave
> >> me access to EL2 and I was able to boot Xen and then Linux as Dom0.
> >
> > Yeah we're talking about the same thing. I was just curious whether
> > the Chrome folks have heard of it, or whether they have any changes/
> > workarounds for it.
>
> Does Linux ever write to this region? Given that the Chromebooks don't
> seem to have issues with this (we have a bunch of them in pmOS and I'd
> be very very surprised if this was an issue there which nobody had tried
> upstreaming before) I'd guess the significant difference here is between
> booting Linux in EL2 (as Chromebooks do?) vs with Xen.
>
FWIW: This old patch series from Stephen Boyd is closely related:
https://lore.kernel.org/linux-arm-msm/[email protected]/
> The main use case I have is to map the command-db memory region on
> Qualcomm devices with a read-only mapping. It's already a const marked
> pointer and the API returns const pointers as well, so this series
> makes sure that even stray writes can't modify the memory.
Stephen, what was the end result of that patch series? Mapping the
cmd-db read-only sounds cleaner than trying to be lucky with the right
set of cache flags.
Thanks,
Stephan
Hi Caleb,
Caleb Connolly <[email protected]> writes:
> On 27/03/2024 21:06, Konrad Dybcio wrote:
>> On 27.03.2024 10:04 PM, Volodymyr Babchuk wrote:
>>>
>>> Hi Konrad,
>>>
>>> Konrad Dybcio <[email protected]> writes:
>>>
>>>> On 27.03.2024 9:09 PM, Volodymyr Babchuk wrote:
>>>>> It appears that hardware does not like cacheable accesses to this
>>>>> region. Trying to access this shared memory region as Normal Memory
>>>>> leads to secure interrupt which causes an endless loop somewhere in
>>>>> Trust Zone.
>>>>>
>>>>> The only reason it is working right now is because Qualcomm Hypervisor
>>>>> maps the same region as Non-Cacheable memory in Stage 2 translation
>>>>> tables. The issue manifests if we want to use another hypervisor (like
>>>>> Xen or KVM), which does not know anything about those specific
>>>>> mappings. This patch fixes the issue by mapping the shared memory as
>>>>> Write-Through. This removes dependency on correct mappings in Stage 2
>>>>> tables.
>>>>>
>>>>> I tested this on SA8155P with Xen.
>>>>>
>>>>> Signed-off-by: Volodymyr Babchuk <[email protected]>
>>>>> ---
>>>>
>>>> Interesting..
>>>>
>>>> +Doug, Rob have you ever seen this on Chrome? (FYI, Volodymyr, chromebooks
>>>> ship with no qcom hypervisor)
>>>
>>> Well, maybe I was wrong when called this thing "hypervisor". All I know
>>> that it sits in hyp.mbn partition and all what it does is setup EL2
>>> before switching to EL1 and running UEFI.
>>>
>>> In my experiments I replaced contents of hyp.mbn with U-Boot, which gave
>>> me access to EL2 and I was able to boot Xen and then Linux as Dom0.
>>
>> Yeah we're talking about the same thing. I was just curious whether
>> the Chrome folks have heard of it, or whether they have any changes/
>> workarounds for it.
>
> Does Linux ever write to this region? Given that the Chromebooks don't
> seem to have issues with this (we have a bunch of them in pmOS and I'd
> be very very surprised if this was an issue there which nobody had tried
> upstreaming before) I'd guess the significant difference here is between
> booting Linux in EL2 (as Chromebooks do?) vs with Xen.
It does not write, but I assume that direction is irrelevant in this
case. AFAIK, CPU signals memory type to the bus for both reads and
writes, just with different signals: ARCACHE[3:0] and AWCACHE[3:0].
>
> Volodymyr: have you tried booting the kernel directly from U-Boot in
> EL2? Can you confirm if this issues also happens then?
Yes, behavior is exactly the same. It does not work with WB, but work
with WC or WT.
--
WBR, Volodymyr
Quoting Stephan Gerhold (2024-03-28 02:58:56)
>
> FWIW: This old patch series from Stephen Boyd is closely related:
> https://lore.kernel.org/linux-arm-msm/[email protected]/
>
> > The main use case I have is to map the command-db memory region on
> > Qualcomm devices with a read-only mapping. It's already a const marked
> > pointer and the API returns const pointers as well, so this series
> > makes sure that even stray writes can't modify the memory.
>
> Stephen, what was the end result of that patch series? Mapping the
> cmd-db read-only sounds cleaner than trying to be lucky with the right
> set of cache flags.
>
I dropped it because I got busy with other stuff. Feel free to pick it
back up. It looks like the part where I left off was where we had to
make the API fallback to mapping the memory as writeable if read-only
isn't supported on the architecture. I also wanted to return a const
pointer. The other weird thing was that we passed both MEMREMAP_RO and
MEMREMAP_WB to indicate what sort of fallback we want. Perhaps that can
be encoded in the architecture layer so that you get the closest thing
to read-only memory (i.e. any sort of write side caching is removed) and
you don't have to pass a fallback mapping type.
Here's my stash patch on top of the branch (from 2019!).
---8<----
From: Stephen Boyd <[email protected]>
Date: Tue, 14 May 2019 13:22:01 -0700
Subject: [PATCH] stash const iounmap
---
arch/arm64/include/asm/io.h | 2 +-
arch/arm64/mm/ioremap.c | 9 +++++----
arch/x86/mm/ioremap.c | 2 +-
drivers/firmware/google/vpd.c | 22 +++++++++++-----------
drivers/soc/qcom/rmtfs_mem.c | 5 ++---
include/asm-generic/io.h | 2 +-
include/linux/io.h | 2 +-
include/linux/mmiotrace.h | 2 +-
kernel/iomem.c | 2 +-
9 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
index 245bd371e8dc..0fd4f1678300 100644
--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h
@@ -178,7 +178,7 @@ extern void __memset_io(volatile void __iomem *,
int, size_t);
* I/O memory mapping functions.
*/
extern void __iomem *__ioremap(phys_addr_t phys_addr, size_t size,
pgprot_t prot);
-extern void __iounmap(volatile void __iomem *addr);
+extern void __iounmap(const volatile void __iomem *addr);
extern void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size);
#define ioremap(addr, size) __ioremap((addr), (size),
__pgprot(PROT_DEVICE_nGnRE))
diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c
index c4c8cd4c31d4..e39fb2cb6042 100644
--- a/arch/arm64/mm/ioremap.c
+++ b/arch/arm64/mm/ioremap.c
@@ -80,16 +80,17 @@ void __iomem *__ioremap(phys_addr_t phys_addr,
size_t size, pgprot_t prot)
}
EXPORT_SYMBOL(__ioremap);
-void __iounmap(volatile void __iomem *io_addr)
+void __iounmap(const volatile void __iomem *io_addr)
{
- unsigned long addr = (unsigned long)io_addr & PAGE_MASK;
+ const unsigned long addr = (const unsigned long)io_addr & PAGE_MASK;
+ const void *vaddr = (const void __force *)addr;
/*
* We could get an address outside vmalloc range in case
* of ioremap_cache() reusing a RAM mapping.
*/
- if (is_vmalloc_addr((void *)addr))
- vunmap((void *)addr);
+ if (is_vmalloc_addr(vaddr))
+ vunmap(vaddr);
}
EXPORT_SYMBOL(__iounmap);
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 0029604af8a4..e9a2910d0c63 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -392,7 +392,7 @@ EXPORT_SYMBOL(ioremap_prot);
*
* Caller must ensure there is only one unmapping for the same pointer.
*/
-void iounmap(volatile void __iomem *addr)
+void iounmap(const volatile void __iomem *addr)
{
struct vm_struct *p, *o;
diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
index c0c0b4e4e281..7428f189999e 100644
--- a/drivers/firmware/google/vpd.c
+++ b/drivers/firmware/google/vpd.c
@@ -48,7 +48,7 @@ struct vpd_section {
const char *name;
char *raw_name; /* the string name_raw */
struct kobject *kobj; /* vpd/name directory */
- char *baseaddr;
+ const char *baseaddr;
struct bin_attribute bin_attr; /* vpd/name_raw bin_attribute */
struct list_head attribs; /* key/value in vpd_attrib_info list */
};
@@ -187,19 +187,19 @@ static int vpd_section_create_attribs(struct
vpd_section *sec)
return 0;
}
-static int vpd_section_init(const char *name, struct vpd_section *sec,
+static int vpd_section_init(struct device *dev, const char *name,
struct vpd_section *sec,
phys_addr_t physaddr, size_t size)
{
int err;
- sec->baseaddr = memremap(physaddr, size, MEMREMAP_WB);
- if (!sec->baseaddr)
- return -ENOMEM;
+ sec->baseaddr = devm_memremap(physaddr, size, MEMREMAP_RO | MEMREMAP_WB);
+ if (IS_ERR(sec->baseaddr))
+ return PTR_ERR(sec->baseaddr);
sec->name = name;
/* We want to export the raw partition with name ${name}_raw */
- sec->raw_name = kasprintf(GFP_KERNEL, "%s_raw", name);
+ sec->raw_name = devm_kasprintf(GFP_KERNEL, "%s_raw", name);
if (!sec->raw_name) {
err = -ENOMEM;
goto err_memunmap;
@@ -252,11 +252,12 @@ static int vpd_section_destroy(struct vpd_section *sec)
return 0;
}
-static int vpd_sections_init(phys_addr_t physaddr)
+static int vpd_sections_init(struct coreboot_device *cdev)
{
struct vpd_cbmem __iomem *temp;
struct vpd_cbmem header;
int ret = 0;
+ phys_addr_t physaddr = cdev->cbmem_ref.cbmem_addr;
temp = memremap(physaddr, sizeof(struct vpd_cbmem), MEMREMAP_WB);
if (!temp)
@@ -269,7 +270,7 @@ static int vpd_sections_init(phys_addr_t physaddr)
return -ENODEV;
if (header.ro_size) {
- ret = vpd_section_init("ro", &ro_vpd,
+ ret = vpd_section_init(&cdev->dev, "ro", &ro_vpd,
physaddr + sizeof(struct vpd_cbmem),
header.ro_size);
if (ret)
@@ -294,10 +295,9 @@ static int vpd_probe(struct coreboot_device *dev)
int ret;
vpd_kobj = kobject_create_and_add("vpd", firmware_kobj);
- if (!vpd_kobj)
- return -ENOMEM;
+ if (!vpd_kobj) return -ENOMEM;
- ret = vpd_sections_init(dev->cbmem_ref.cbmem_addr);
+ ret = vpd_sections_init(dev);
if (ret) {
kobject_put(vpd_kobj);
return ret;
diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
index 6f5e8be9689c..137e5240d916 100644
--- a/drivers/soc/qcom/rmtfs_mem.c
+++ b/drivers/soc/qcom/rmtfs_mem.c
@@ -212,10 +212,9 @@ static int qcom_rmtfs_mem_probe(struct
platform_device *pdev)
rmtfs_mem->dev.groups = qcom_rmtfs_mem_groups;
rmtfs_mem->dev.release = qcom_rmtfs_mem_release_device;
- rmtfs_mem->base = devm_memremap(&rmtfs_mem->dev, rmtfs_mem->addr,
- rmtfs_mem->size, MEMREMAP_WC);
+ rmtfs_mem->base = devm_memremap_reserved_mem(&pdev->dev,
+ MEMREMAP_WC);
if (IS_ERR(rmtfs_mem->base)) {
- dev_err(&pdev->dev, "failed to remap rmtfs_mem region\n");
ret = PTR_ERR(rmtfs_mem->base);
goto put_device;
}
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 303871651f8a..d675a574eeb3 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -982,7 +982,7 @@ static inline void __iomem *__ioremap(phys_addr_t
offset, size_t size,
#ifndef iounmap
#define iounmap iounmap
-static inline void iounmap(void __iomem *addr)
+static inline void iounmap(const void __iomem *addr)
{
}
#endif
diff --git a/include/linux/io.h b/include/linux/io.h
index 16c7f4498869..82e6c4d6bdd3 100644
--- a/include/linux/io.h
+++ b/include/linux/io.h
@@ -163,7 +163,7 @@ enum {
};
void *memremap(resource_size_t offset, size_t size, unsigned long flags);
-void memunmap(void *addr);
+void memunmap(const void *addr);
/*
* On x86 PAT systems we have memory tracking that keeps track of
diff --git a/include/linux/mmiotrace.h b/include/linux/mmiotrace.h
index 88236849894d..04607c468b73 100644
--- a/include/linux/mmiotrace.h
+++ b/include/linux/mmiotrace.h
@@ -47,7 +47,7 @@ extern int kmmio_handler(struct pt_regs *regs,
unsigned long addr);
/* Called from ioremap.c */
extern void mmiotrace_ioremap(resource_size_t offset, unsigned long size,
void __iomem *addr);
-extern void mmiotrace_iounmap(volatile void __iomem *addr);
+extern void mmiotrace_iounmap(const volatile void __iomem *addr);
/* For anyone to insert markers. Remember trailing newline. */
extern __printf(1, 2) int mmiotrace_printk(const char *fmt, ...);
diff --git a/kernel/iomem.c b/kernel/iomem.c
index 8d3cf74a32cb..22d0fa336360 100644
--- a/kernel/iomem.c
+++ b/kernel/iomem.c
@@ -130,7 +130,7 @@ void *memremap(resource_size_t offset, size_t
size, unsigned long flags)
}
EXPORT_SYMBOL(memremap);
-void memunmap(void *addr)
+void memunmap(const void *addr)
{
if (is_vmalloc_addr(addr))
iounmap((void __iomem *) addr);
base-commit: 9e98c678c2d6ae3a17cb2de55d17f69dddaa231b
prerequisite-patch-id: 62119e27c0c0686e02f0cb55c296b878fb7f5e47
prerequisite-patch-id: bda32cfc1733c245ae3f141d7c27b18e4adcc628
prerequisite-patch-id: b8f8097161bd15e87d54dcfbfa67b9ca1abc7204
prerequisite-patch-id: cd374fb6e39941b8613d213b4a75909749409d63
prerequisite-patch-id: d8dbc8485a0f86353a314ab5c22fc92d8eac1cc0
prerequisite-patch-id: e539621b0eccf82aabf9891de69c30398abf76a0
prerequisite-patch-id: 59d60033b80dec940201edd5aefed22726122a37
prerequisite-patch-id: 0d16b23cec20eaab7f45ee84fd8d2950657dc72e
--
https://chromeos.dev
Hi Stephan,
Stephan Gerhold <[email protected]> writes:
> On Wed, Mar 27, 2024 at 11:29:09PM +0000, Caleb Connolly wrote:
>> On 27/03/2024 21:06, Konrad Dybcio wrote:
>> > On 27.03.2024 10:04 PM, Volodymyr Babchuk wrote:
>> >> Konrad Dybcio <[email protected]> writes:
>> >>> On 27.03.2024 9:09 PM, Volodymyr Babchuk wrote:
>> >>>> It appears that hardware does not like cacheable accesses to this
>> >>>> region. Trying to access this shared memory region as Normal Memory
>> >>>> leads to secure interrupt which causes an endless loop somewhere in
>> >>>> Trust Zone.
>> >>>>
>> >>>> The only reason it is working right now is because Qualcomm Hypervisor
>> >>>> maps the same region as Non-Cacheable memory in Stage 2 translation
>> >>>> tables. The issue manifests if we want to use another hypervisor (like
>> >>>> Xen or KVM), which does not know anything about those specific
>> >>>> mappings. This patch fixes the issue by mapping the shared memory as
>> >>>> Write-Through. This removes dependency on correct mappings in Stage 2
>> >>>> tables.
>> >>>>
>> >>>> I tested this on SA8155P with Xen.
>> >>>>
>> >>>> Signed-off-by: Volodymyr Babchuk <[email protected]>
>> >>>> ---
>> >>>
>> >>> Interesting..
>> >>>
>> >>> +Doug, Rob have you ever seen this on Chrome? (FYI, Volodymyr, chromebooks
>> >>> ship with no qcom hypervisor)
>> >>
>> >> Well, maybe I was wrong when called this thing "hypervisor". All I know
>> >> that it sits in hyp.mbn partition and all what it does is setup EL2
>> >> before switching to EL1 and running UEFI.
>> >>
>> >> In my experiments I replaced contents of hyp.mbn with U-Boot, which gave
>> >> me access to EL2 and I was able to boot Xen and then Linux as Dom0.
>> >
>> > Yeah we're talking about the same thing. I was just curious whether
>> > the Chrome folks have heard of it, or whether they have any changes/
>> > workarounds for it.
>>
>> Does Linux ever write to this region? Given that the Chromebooks don't
>> seem to have issues with this (we have a bunch of them in pmOS and I'd
>> be very very surprised if this was an issue there which nobody had tried
>> upstreaming before) I'd guess the significant difference here is between
>> booting Linux in EL2 (as Chromebooks do?) vs with Xen.
>>
>
> FWIW: This old patch series from Stephen Boyd is closely related:
> https://urldefense.com/v3/__https://lore.kernel.org/linux-arm-msm/[email protected]/__;!!GF_29dbcQIUBPA!yGecMHGezwkDU9t7XATVTI80PNGjZdQV2xsYFTl6EhpMMsRf_7xryKx8mEVpmTwTcKMGaaWomtyvr05zFcmsf2Kk$
> [lore[.]kernel[.]org]
>
>> The main use case I have is to map the command-db memory region on
>> Qualcomm devices with a read-only mapping. It's already a const marked
>> pointer and the API returns const pointers as well, so this series
>> makes sure that even stray writes can't modify the memory.
>
> Stephen, what was the end result of that patch series? Mapping the
> cmd-db read-only sounds cleaner than trying to be lucky with the right
> set of cache flags.
>
I checked the series, but I am afraid that I have no capacity to finish
this. Will it be okay to move forward with my patch? I understand that
this is not the best solution, but it is simple and it works. If this is
fine, I'll send v2 with all comments addressed.
--
WBR, Volodymyr
On Thu, Mar 28, 2024 at 07:40:49PM -0500, Stephen Boyd wrote:
> Quoting Stephan Gerhold (2024-03-28 02:58:56)
> >
> > FWIW: This old patch series from Stephen Boyd is closely related:
> > https://lore.kernel.org/linux-arm-msm/[email protected]/
> >
> > > The main use case I have is to map the command-db memory region on
> > > Qualcomm devices with a read-only mapping. It's already a const marked
> > > pointer and the API returns const pointers as well, so this series
> > > makes sure that even stray writes can't modify the memory.
> >
> > Stephen, what was the end result of that patch series? Mapping the
> > cmd-db read-only sounds cleaner than trying to be lucky with the right
> > set of cache flags.
> >
>
> I dropped it because I got busy with other stuff. Feel free to pick it
> back up. It looks like the part where I left off was where we had to
> make the API fallback to mapping the memory as writeable if read-only
> isn't supported on the architecture.
[...]
> The other weird thing was that we passed both MEMREMAP_RO and
> MEMREMAP_WB to indicate what sort of fallback we want. Perhaps that can
> be encoded in the architecture layer so that you get the closest thing
> to read-only memory (i.e. any sort of write side caching is removed) and
> you don't have to pass a fallback mapping type.
Was there any motivation for having the fallback? I suspect driver
owners that want to use MEMREMAP_RO will know which architectures the
driver is applicable to and also know whether MEMREMAP_RO would work.
> I also wanted to return a const pointer.
The stash looks mostly complete. Do you think it should be part of the
series or submitted separately?
>
> Here's my stash patch on top of the branch (from 2019!).
>
> ---8<----
> From: Stephen Boyd <[email protected]>
> Date: Tue, 14 May 2019 13:22:01 -0700
> Subject: [PATCH] stash const iounmap
>
> ---
> arch/arm64/include/asm/io.h | 2 +-
> arch/arm64/mm/ioremap.c | 9 +++++----
> arch/x86/mm/ioremap.c | 2 +-
> drivers/firmware/google/vpd.c | 22 +++++++++++-----------
> drivers/soc/qcom/rmtfs_mem.c | 5 ++---
> include/asm-generic/io.h | 2 +-
> include/linux/io.h | 2 +-
> include/linux/mmiotrace.h | 2 +-
> kernel/iomem.c | 2 +-
> 9 files changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
> index 245bd371e8dc..0fd4f1678300 100644
> --- a/arch/arm64/include/asm/io.h
> +++ b/arch/arm64/include/asm/io.h
> @@ -178,7 +178,7 @@ extern void __memset_io(volatile void __iomem *,
> int, size_t);
> * I/O memory mapping functions.
> */
> extern void __iomem *__ioremap(phys_addr_t phys_addr, size_t size,
> pgprot_t prot);
> -extern void __iounmap(volatile void __iomem *addr);
> +extern void __iounmap(const volatile void __iomem *addr);
> extern void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size);
>
> #define ioremap(addr, size) __ioremap((addr), (size),
> __pgprot(PROT_DEVICE_nGnRE))
> diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c
> index c4c8cd4c31d4..e39fb2cb6042 100644
> --- a/arch/arm64/mm/ioremap.c
> +++ b/arch/arm64/mm/ioremap.c
> @@ -80,16 +80,17 @@ void __iomem *__ioremap(phys_addr_t phys_addr,
> size_t size, pgprot_t prot)
> }
> EXPORT_SYMBOL(__ioremap);
>
> -void __iounmap(volatile void __iomem *io_addr)
> +void __iounmap(const volatile void __iomem *io_addr)
> {
> - unsigned long addr = (unsigned long)io_addr & PAGE_MASK;
> + const unsigned long addr = (const unsigned long)io_addr & PAGE_MASK;
> + const void *vaddr = (const void __force *)addr;
>
> /*
> * We could get an address outside vmalloc range in case
> * of ioremap_cache() reusing a RAM mapping.
> */
> - if (is_vmalloc_addr((void *)addr))
> - vunmap((void *)addr);
> + if (is_vmalloc_addr(vaddr))
> + vunmap(vaddr);
> }
> EXPORT_SYMBOL(__iounmap);
>
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 0029604af8a4..e9a2910d0c63 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -392,7 +392,7 @@ EXPORT_SYMBOL(ioremap_prot);
> *
> * Caller must ensure there is only one unmapping for the same pointer.
> */
> -void iounmap(volatile void __iomem *addr)
> +void iounmap(const volatile void __iomem *addr)
> {
> struct vm_struct *p, *o;
>
> diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
> index c0c0b4e4e281..7428f189999e 100644
> --- a/drivers/firmware/google/vpd.c
> +++ b/drivers/firmware/google/vpd.c
> @@ -48,7 +48,7 @@ struct vpd_section {
> const char *name;
> char *raw_name; /* the string name_raw */
> struct kobject *kobj; /* vpd/name directory */
> - char *baseaddr;
> + const char *baseaddr;
> struct bin_attribute bin_attr; /* vpd/name_raw bin_attribute */
> struct list_head attribs; /* key/value in vpd_attrib_info list */
> };
> @@ -187,19 +187,19 @@ static int vpd_section_create_attribs(struct
> vpd_section *sec)
> return 0;
> }
>
> -static int vpd_section_init(const char *name, struct vpd_section *sec,
> +static int vpd_section_init(struct device *dev, const char *name,
> struct vpd_section *sec,
> phys_addr_t physaddr, size_t size)
> {
> int err;
>
> - sec->baseaddr = memremap(physaddr, size, MEMREMAP_WB);
> - if (!sec->baseaddr)
> - return -ENOMEM;
> + sec->baseaddr = devm_memremap(physaddr, size, MEMREMAP_RO | MEMREMAP_WB);
> + if (IS_ERR(sec->baseaddr))
> + return PTR_ERR(sec->baseaddr);
>
> sec->name = name;
>
> /* We want to export the raw partition with name ${name}_raw */
> - sec->raw_name = kasprintf(GFP_KERNEL, "%s_raw", name);
> + sec->raw_name = devm_kasprintf(GFP_KERNEL, "%s_raw", name);
> if (!sec->raw_name) {
> err = -ENOMEM;
> goto err_memunmap;
> @@ -252,11 +252,12 @@ static int vpd_section_destroy(struct vpd_section *sec)
> return 0;
> }
>
> -static int vpd_sections_init(phys_addr_t physaddr)
> +static int vpd_sections_init(struct coreboot_device *cdev)
> {
> struct vpd_cbmem __iomem *temp;
> struct vpd_cbmem header;
> int ret = 0;
> + phys_addr_t physaddr = cdev->cbmem_ref.cbmem_addr;
>
> temp = memremap(physaddr, sizeof(struct vpd_cbmem), MEMREMAP_WB);
> if (!temp)
> @@ -269,7 +270,7 @@ static int vpd_sections_init(phys_addr_t physaddr)
> return -ENODEV;
>
> if (header.ro_size) {
> - ret = vpd_section_init("ro", &ro_vpd,
> + ret = vpd_section_init(&cdev->dev, "ro", &ro_vpd,
> physaddr + sizeof(struct vpd_cbmem),
> header.ro_size);
> if (ret)
> @@ -294,10 +295,9 @@ static int vpd_probe(struct coreboot_device *dev)
> int ret;
>
> vpd_kobj = kobject_create_and_add("vpd", firmware_kobj);
> - if (!vpd_kobj)
> - return -ENOMEM;
> + if (!vpd_kobj) return -ENOMEM;
>
> - ret = vpd_sections_init(dev->cbmem_ref.cbmem_addr);
> + ret = vpd_sections_init(dev);
> if (ret) {
> kobject_put(vpd_kobj);
> return ret;
> diff --git a/drivers/soc/qcom/rmtfs_mem.c b/drivers/soc/qcom/rmtfs_mem.c
> index 6f5e8be9689c..137e5240d916 100644
> --- a/drivers/soc/qcom/rmtfs_mem.c
> +++ b/drivers/soc/qcom/rmtfs_mem.c
> @@ -212,10 +212,9 @@ static int qcom_rmtfs_mem_probe(struct
> platform_device *pdev)
> rmtfs_mem->dev.groups = qcom_rmtfs_mem_groups;
> rmtfs_mem->dev.release = qcom_rmtfs_mem_release_device;
>
> - rmtfs_mem->base = devm_memremap(&rmtfs_mem->dev, rmtfs_mem->addr,
> - rmtfs_mem->size, MEMREMAP_WC);
> + rmtfs_mem->base = devm_memremap_reserved_mem(&pdev->dev,
> + MEMREMAP_WC);
> if (IS_ERR(rmtfs_mem->base)) {
> - dev_err(&pdev->dev, "failed to remap rmtfs_mem region\n");
> ret = PTR_ERR(rmtfs_mem->base);
> goto put_device;
> }
> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> index 303871651f8a..d675a574eeb3 100644
> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> @@ -982,7 +982,7 @@ static inline void __iomem *__ioremap(phys_addr_t
> offset, size_t size,
> #ifndef iounmap
> #define iounmap iounmap
>
> -static inline void iounmap(void __iomem *addr)
> +static inline void iounmap(const void __iomem *addr)
> {
> }
> #endif
> diff --git a/include/linux/io.h b/include/linux/io.h
> index 16c7f4498869..82e6c4d6bdd3 100644
> --- a/include/linux/io.h
> +++ b/include/linux/io.h
> @@ -163,7 +163,7 @@ enum {
> };
>
> void *memremap(resource_size_t offset, size_t size, unsigned long flags);
> -void memunmap(void *addr);
> +void memunmap(const void *addr);
>
> /*
> * On x86 PAT systems we have memory tracking that keeps track of
> diff --git a/include/linux/mmiotrace.h b/include/linux/mmiotrace.h
> index 88236849894d..04607c468b73 100644
> --- a/include/linux/mmiotrace.h
> +++ b/include/linux/mmiotrace.h
> @@ -47,7 +47,7 @@ extern int kmmio_handler(struct pt_regs *regs,
> unsigned long addr);
> /* Called from ioremap.c */
> extern void mmiotrace_ioremap(resource_size_t offset, unsigned long size,
> void __iomem *addr);
> -extern void mmiotrace_iounmap(volatile void __iomem *addr);
> +extern void mmiotrace_iounmap(const volatile void __iomem *addr);
>
> /* For anyone to insert markers. Remember trailing newline. */
> extern __printf(1, 2) int mmiotrace_printk(const char *fmt, ...);
> diff --git a/kernel/iomem.c b/kernel/iomem.c
> index 8d3cf74a32cb..22d0fa336360 100644
> --- a/kernel/iomem.c
> +++ b/kernel/iomem.c
> @@ -130,7 +130,7 @@ void *memremap(resource_size_t offset, size_t
> size, unsigned long flags)
> }
> EXPORT_SYMBOL(memremap);
>
> -void memunmap(void *addr)
> +void memunmap(const void *addr)
> {
> if (is_vmalloc_addr(addr))
> iounmap((void __iomem *) addr);
>
> base-commit: 9e98c678c2d6ae3a17cb2de55d17f69dddaa231b
> prerequisite-patch-id: 62119e27c0c0686e02f0cb55c296b878fb7f5e47
> prerequisite-patch-id: bda32cfc1733c245ae3f141d7c27b18e4adcc628
> prerequisite-patch-id: b8f8097161bd15e87d54dcfbfa67b9ca1abc7204
> prerequisite-patch-id: cd374fb6e39941b8613d213b4a75909749409d63
> prerequisite-patch-id: d8dbc8485a0f86353a314ab5c22fc92d8eac1cc0
> prerequisite-patch-id: e539621b0eccf82aabf9891de69c30398abf76a0
> prerequisite-patch-id: 59d60033b80dec940201edd5aefed22726122a37
> prerequisite-patch-id: 0d16b23cec20eaab7f45ee84fd8d2950657dc72e
> --
> https://chromeos.dev
>
Quoting Elliot Berman (2024-04-10 20:54:24)
> On Thu, Mar 28, 2024 at 07:40:49PM -0500, Stephen Boyd wrote:
> > Quoting Stephan Gerhold (2024-03-28 02:58:56)
> > >
> > > FWIW: This old patch series from Stephen Boyd is closely related:
> > > https://lore.kernel.org/linux-arm-msm/[email protected]/
> > >
> > > > The main use case I have is to map the command-db memory region on
> > > > Qualcomm devices with a read-only mapping. It's already a const marked
> > > > pointer and the API returns const pointers as well, so this series
> > > > makes sure that even stray writes can't modify the memory.
> > >
> > > Stephen, what was the end result of that patch series? Mapping the
> > > cmd-db read-only sounds cleaner than trying to be lucky with the right
> > > set of cache flags.
> > >
> >
> > I dropped it because I got busy with other stuff. Feel free to pick it
> > back up. It looks like the part where I left off was where we had to
> > make the API fallback to mapping the memory as writeable if read-only
> > isn't supported on the architecture.
> [...]
> > The other weird thing was that we passed both MEMREMAP_RO and
> > MEMREMAP_WB to indicate what sort of fallback we want. Perhaps that can
> > be encoded in the architecture layer so that you get the closest thing
> > to read-only memory (i.e. any sort of write side caching is removed) and
> > you don't have to pass a fallback mapping type.
>
> Was there any motivation for having the fallback? I suspect driver
> owners that want to use MEMREMAP_RO will know which architectures the
> driver is applicable to and also know whether MEMREMAP_RO would work.
I don't think there was any motivation, but it has been many years so
maybe I forgot. I suspect you're suggesting that we just don't do that
and driver writers can call the memremap API another time if they want
to implement a fallback? Sounds OK to me.
>
> > I also wanted to return a const pointer.
>
> The stash looks mostly complete. Do you think it should be part of the
> series or submitted separately?
>
Make it part of the series so this topic can be finished? For example,
it seems like iounmap() should have always taken a const pointer.
On Wed, Apr 10, 2024 at 10:12:37PM +0000, Volodymyr Babchuk wrote:
> Stephan Gerhold <[email protected]> writes:
> > On Wed, Mar 27, 2024 at 11:29:09PM +0000, Caleb Connolly wrote:
> >> On 27/03/2024 21:06, Konrad Dybcio wrote:
> >> > On 27.03.2024 10:04 PM, Volodymyr Babchuk wrote:
> >> >> Konrad Dybcio <[email protected]> writes:
> >> >>> On 27.03.2024 9:09 PM, Volodymyr Babchuk wrote:
> >> >>>> It appears that hardware does not like cacheable accesses to this
> >> >>>> region. Trying to access this shared memory region as Normal Memory
> >> >>>> leads to secure interrupt which causes an endless loop somewhere in
> >> >>>> Trust Zone.
> >> >>>>
> >> >>>> The only reason it is working right now is because Qualcomm Hypervisor
> >> >>>> maps the same region as Non-Cacheable memory in Stage 2 translation
> >> >>>> tables. The issue manifests if we want to use another hypervisor (like
> >> >>>> Xen or KVM), which does not know anything about those specific
> >> >>>> mappings. This patch fixes the issue by mapping the shared memory as
> >> >>>> Write-Through. This removes dependency on correct mappings in Stage 2
> >> >>>> tables.
> >> >>>>
> >> >>>> I tested this on SA8155P with Xen.
> >> >>>>
> >> >>>> Signed-off-by: Volodymyr Babchuk <[email protected]>
> >> >>>> ---
> >> >>>
> >> >>> Interesting..
> >> >>>
> >> >>> +Doug, Rob have you ever seen this on Chrome? (FYI, Volodymyr, chromebooks
> >> >>> ship with no qcom hypervisor)
> >> >>
> >> >> Well, maybe I was wrong when called this thing "hypervisor". All I know
> >> >> that it sits in hyp.mbn partition and all what it does is setup EL2
> >> >> before switching to EL1 and running UEFI.
> >> >>
> >> >> In my experiments I replaced contents of hyp.mbn with U-Boot, which gave
> >> >> me access to EL2 and I was able to boot Xen and then Linux as Dom0.
> >> >
> >> > Yeah we're talking about the same thing. I was just curious whether
> >> > the Chrome folks have heard of it, or whether they have any changes/
> >> > workarounds for it.
> >>
> >> Does Linux ever write to this region? Given that the Chromebooks don't
> >> seem to have issues with this (we have a bunch of them in pmOS and I'd
> >> be very very surprised if this was an issue there which nobody had tried
> >> upstreaming before) I'd guess the significant difference here is between
> >> booting Linux in EL2 (as Chromebooks do?) vs with Xen.
> >>
> >
> > FWIW: This old patch series from Stephen Boyd is closely related:
> > https://urldefense.com/v3/__https://lore.kernel.org/linux-arm-msm/[email protected]/__;!!GF_29dbcQIUBPA!yGecMHGezwkDU9t7XATVTI80PNGjZdQV2xsYFTl6EhpMMsRf_7xryKx8mEVpmTwTcKMGaaWomtyvr05zFcmsf2Kk$
> > [lore[.]kernel[.]org]
> >
> >> The main use case I have is to map the command-db memory region on
> >> Qualcomm devices with a read-only mapping. It's already a const marked
> >> pointer and the API returns const pointers as well, so this series
> >> makes sure that even stray writes can't modify the memory.
> >
> > Stephen, what was the end result of that patch series? Mapping the
> > cmd-db read-only sounds cleaner than trying to be lucky with the right
> > set of cache flags.
> >
>
> I checked the series, but I am afraid that I have no capacity to finish
> this. Will it be okay to move forward with my patch? I understand that
> this is not the best solution, but it is simple and it works. If this is
> fine, I'll send v2 with all comments addressed.
>
My current understanding is that the important property here is to have
a non-cacheable mapping, which is the case for both MEMREMAP_WT and
MEMREMAP_WC, but not MEMREMAP_WB. Unfortunately, the MEMREMAP_RO option
Stephen introduced is also a cacheable mapping, which still seems to
trigger the issue in some cases. I'm not sure why a cache writeback
still happens when the mapping is read-only and nobody writes anything.
You can also test it if you want. For a quick test,
- cmd_db_header = memremap(rmem->base, rmem->size, MEMREMAP_WB);
+ cmd_db_header = ioremap_prot(rmem->base, rmem->size, _PAGE_KERNEL_RO);
should be (largely) equivalent to MEMREMAP_RO with Stephen's patch
series. I asked Nikita to test this on SC7180 and it still seems to
cause the crash.
It seems to work only with a read-only non-cacheable mapping, e.g. with
+ cmd_db_header = ioremap_prot(rmem->base, rmem->size,
((PROT_NORMAL_NC & ~PTE_WRITE) | PTE_RDONLY));
The lines I just suggested for testing are highly architecture-specific
though so not usable for a proper patch. If MEMREMAP_RO does not solve
the real problem here then the work to make an usable read-only mapping
would go beyond just finishing Stephen's patch series, since one would
need to introduce some kind of MEMREMAP_RO_NC flag that creates a
read-only non-cacheable mapping.
It is definitely easier to just change the driver to use the existing
MEMREMAP_WC. Given the crash you found, the hardware/firmware seems to
have a built-in write protection on most platforms anyway. :D
Thanks,
Stephan