| Message ID | 20240327200917.2576034-1-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | soc: qcom: cmd-db: map shared memory as WT, not WB | expand |
Hi Konrad, Konrad Dybcio <konrad.dybcio@linaro.org> 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 <volodymyr_babchuk@epam.com> >> --- > > 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.
On 27/03/2024 21:06, Konrad Dybcio wrote: > On 27.03.2024 10:04 PM, Volodymyr Babchuk wrote: >> >> Hi Konrad, >> >> Konrad Dybcio <konrad.dybcio@linaro.org> 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 <volodymyr_babchuk@epam.com> >>>> --- >>> >>> 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
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 <konrad.dybcio@linaro.org> 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 <volodymyr_babchuk@epam.com> > >>>> --- > >>> > >>> 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/20190910160903.65694-1-swboyd@chromium.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. Thanks, Stephan
On Wed, Mar 27, 2024 at 08:09:34PM +0000, 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. > Hi! I observe a similar issue while trying to boot Linux in EL2 after taking over qcom's hyp on a sc7180 WoA device: [ 0.337736] CPU: All CPU(s) started at EL2 (...) [ 0.475135] Serial: AMBA PL011 UART driver [ 0.479649] Internal error: synchronous external abort: 0000000096000410 [#1] PREEMPT SMP [ 0.488053] Modules linked in: [ 0.491213] CPU: 6 PID: 1 Comm: swapper/0 Not tainted 6.7.0 #41 [ 0.497310] Hardware name: Acer Aspire 1 (DT) [ 0.501800] pstate: 00400009 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 0.508964] pc : cmd_db_dev_probe+0x38/0xc4 [ 0.513290] lr : cmd_db_dev_probe+0x2c/0xc4 [ 0.517606] sp : ffff8000817ebab0 [ 0.521019] x29: ffff8000817ebab0 x28: 0000000000000000 x27: ffff800081346050 <uart cuts out> Unfortunately this patch doesn't help in this case (I beileve I even tried same/similar change a while back when trying to debug this) Currently I can work around this by just reocationg the cmd-db while still under the qcom's hyp [1] but it would be nice to find a generic solution that doesn't need pre-boot hacks... AFAIK this is not observed on at least sc8280xp WoA devices and I'd assume cros is not affected because they don't use qcom's TZ and instead use TF-A (which is overall more friendly, though still uses qcom's proprietary qtiseclib under the hood) Nikita [1] https://github.com/TravMurav/slbounce/blob/main/src/dtbhack_main.c#L17 > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > --- > drivers/soc/qcom/cmd-db.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c > index a5fd68411bed5..dd5ababdb476c 100644 > --- a/drivers/soc/qcom/cmd-db.c > +++ b/drivers/soc/qcom/cmd-db.c > @@ -324,7 +324,7 @@ static int cmd_db_dev_probe(struct platform_device *pdev) > return -EINVAL; > } > > - cmd_db_header = memremap(rmem->base, rmem->size, MEMREMAP_WB); > + cmd_db_header = memremap(rmem->base, rmem->size, MEMREMAP_WT); > if (!cmd_db_header) { > ret = -ENOMEM; > cmd_db_header = NULL; > -- > 2.43.0
On 3/28/2024 1:39 AM, 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. Linux does not write into cmd-db region. This region is write protected by XPU. Making this region uncached magically solves the XPU write fault issue. Can you please include above details? > > 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. Using MEMREMAP_WC also resolves for qcm6490, see below comment. > > I tested this on SA8155P with Xen. > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > --- > drivers/soc/qcom/cmd-db.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c > index a5fd68411bed5..dd5ababdb476c 100644 > --- a/drivers/soc/qcom/cmd-db.c > +++ b/drivers/soc/qcom/cmd-db.c > @@ -324,7 +324,7 @@ static int cmd_db_dev_probe(struct platform_device *pdev) > return -EINVAL; > } > > - cmd_db_header = memremap(rmem->base, rmem->size, MEMREMAP_WB); > + cmd_db_header = memremap(rmem->base, rmem->size, MEMREMAP_WT); In downstream, we have below which resolved similar issue on qcm6490. cmd_db_header = memremap(rmem->base, rmem->size, MEMREMAP_WC); Downstream SA8155P also have MEMREMAP_WC. Can you please give it a try on your device? Thanks, Maulik
On Thu, Mar 28, 2024 at 04:12:11PM +0500, Nikita Travkin wrote: > On Wed, Mar 27, 2024 at 08:09:34PM +0000, 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. > > > > Hi! > > I observe a similar issue while trying to boot Linux in EL2 after taking > over qcom's hyp on a sc7180 WoA device: > > [ 0.337736] CPU: All CPU(s) started at EL2 > (...) > [ 0.475135] Serial: AMBA PL011 UART driver > [ 0.479649] Internal error: synchronous external abort: 0000000096000410 [#1] PREEMPT SMP > [ 0.488053] Modules linked in: > [ 0.491213] CPU: 6 PID: 1 Comm: swapper/0 Not tainted 6.7.0 #41 > [ 0.497310] Hardware name: Acer Aspire 1 (DT) > [ 0.501800] pstate: 00400009 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 0.508964] pc : cmd_db_dev_probe+0x38/0xc4 > [ 0.513290] lr : cmd_db_dev_probe+0x2c/0xc4 > [ 0.517606] sp : ffff8000817ebab0 > [ 0.521019] x29: ffff8000817ebab0 x28: 0000000000000000 x27: ffff800081346050 > <uart cuts out> > > Unfortunately this patch doesn't help in this case (I beileve I even > tried same/similar change a while back when trying to debug this) > I'm sorry, it looks like I made a mistake in my tooling while testing this patch, which I only realized after trying Maulik's suggestion... Both _WT and _WC fix the issue I see on sc7180 WoA, so whether you keep the patch as is or change it to _WC as suggested: Tested-By: Nikita Travkin <nikita@trvn.ru> # sc7180 WoA in EL2 Thanks for looking into this! Nikita > Currently I can work around this by just reocationg the cmd-db while > still under the qcom's hyp [1] but it would be nice to find a generic > solution that doesn't need pre-boot hacks... > > AFAIK this is not observed on at least sc8280xp WoA devices and I'd > assume cros is not affected because they don't use qcom's TZ and instead > use TF-A (which is overall more friendly, though still uses qcom's > proprietary qtiseclib under the hood) > > Nikita > > [1] https://github.com/TravMurav/slbounce/blob/main/src/dtbhack_main.c#L17 > > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > > --- > > drivers/soc/qcom/cmd-db.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c > > index a5fd68411bed5..dd5ababdb476c 100644 > > --- a/drivers/soc/qcom/cmd-db.c > > +++ b/drivers/soc/qcom/cmd-db.c > > @@ -324,7 +324,7 @@ static int cmd_db_dev_probe(struct platform_device *pdev) > > return -EINVAL; > > } > > > > - cmd_db_header = memremap(rmem->base, rmem->size, MEMREMAP_WB); > > + cmd_db_header = memremap(rmem->base, rmem->size, MEMREMAP_WT); > > if (!cmd_db_header) { > > ret = -ENOMEM; > > cmd_db_header = NULL; > > -- > > 2.43.0
Hi Caleb, Caleb Connolly <caleb.connolly@linaro.org> writes: > On 27/03/2024 21:06, Konrad Dybcio wrote: >> On 27.03.2024 10:04 PM, Volodymyr Babchuk wrote: >>> >>> Hi Konrad, >>> >>> Konrad Dybcio <konrad.dybcio@linaro.org> 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 <volodymyr_babchuk@epam.com> >>>>> --- >>>> >>>> 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.
Hi Maulik "Maulik Shah (mkshah)" <quic_mkshah@quicinc.com> writes: > On 3/28/2024 1:39 AM, 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. > > Linux does not write into cmd-db region. This region is write > protected by XPU. Making this region uncached magically solves the XPU > write fault > issue. > > Can you please include above details? Sure, I'll add this to the next version. >> 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. > > Using MEMREMAP_WC also resolves for qcm6490, see below comment. > >> I tested this on SA8155P with Xen. >> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> >> --- >> drivers/soc/qcom/cmd-db.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c >> index a5fd68411bed5..dd5ababdb476c 100644 >> --- a/drivers/soc/qcom/cmd-db.c >> +++ b/drivers/soc/qcom/cmd-db.c >> @@ -324,7 +324,7 @@ static int cmd_db_dev_probe(struct platform_device *pdev) >> return -EINVAL; >> } >> - cmd_db_header = memremap(rmem->base, rmem->size, MEMREMAP_WB); >> + cmd_db_header = memremap(rmem->base, rmem->size, MEMREMAP_WT); > > In downstream, we have below which resolved similar issue on qcm6490. > > cmd_db_header = memremap(rmem->base, rmem->size, MEMREMAP_WC); > > Downstream SA8155P also have MEMREMAP_WC. Can you please give it a try > on your device? Yes, MEMREMAP_WC works as well. This opens the question: which type is more correct? I have no deep understanding in QCOM internals so it is hard to me to answer this question.
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/20190910160903.65694-1-swboyd@chromium.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 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 <swboyd@chromium.org> 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
On 3/29/2024 3:49 AM, Volodymyr Babchuk wrote: > > Hi Maulik > > "Maulik Shah (mkshah)" <quic_mkshah@quicinc.com> writes: > >> On 3/28/2024 1:39 AM, 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. >> >> Linux does not write into cmd-db region. This region is write >> protected by XPU. Making this region uncached magically solves the XPU >> write fault >> issue. >> >> Can you please include above details? > > Sure, I'll add this to the next version. > Thanks. >> >> In downstream, we have below which resolved similar issue on qcm6490. >> >> cmd_db_header = memremap(rmem->base, rmem->size, MEMREMAP_WC); >> >> Downstream SA8155P also have MEMREMAP_WC. Can you please give it a try >> on your device? > > Yes, MEMREMAP_WC works as well. This opens the question: which type is > more correct? I have no deep understanding in QCOM internals so it is > hard to me to answer this question. > XPU may have falsely detected clean cache eviction as "write" into the write protected region so using uncached flag MEMREMAP_WC may be helping here and is more correct in my understanding. This can also be included in commit message. Thanks, Maulik
Hi Stephan, Stephan Gerhold <stephan@gerhold.net> 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 <konrad.dybcio@linaro.org> 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 <volodymyr_babchuk@epam.com> >> >>>> --- >> >>> >> >>> 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/20190910160903.65694-1-swboyd@chromium.org/__;!!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.
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/20190910160903.65694-1-swboyd@chromium.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 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 <swboyd@chromium.org> > 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/20190910160903.65694-1-swboyd@chromium.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 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 <stephan@gerhold.net> 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 <konrad.dybcio@linaro.org> 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 <volodymyr_babchuk@epam.com> > >> >>>> --- > >> >>> > >> >>> 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/20190910160903.65694-1-swboyd@chromium.org/__;!!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
Quoting Stephan Gerhold (2024-04-11 01:02:01) > On Wed, Apr 10, 2024 at 10:12:37PM +0000, Volodymyr Babchuk wrote: > > Stephan Gerhold <stephan@gerhold.net> 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 <konrad.dybcio@linaro.org> 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 <volodymyr_babchuk@epam.com> > > >> >>>> --- > > >> >>> > > >> >>> Interesting.. > > >> >>> > > >> >>> +Doug, Rob have you ever seen this on Chrome? (FYI, Volodymyr, chromebooks > > >> >>> ship with no qcom hypervisor) ChromeOS boots the kernel at EL2 on sc7180. But more importantly we don't enable whichever xPU it is that you're running into. > > >> >> > > >> >> 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/20190910160903.65694-1-swboyd@chromium.org/__;!!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. Qualcomm knows for certain. It's not a cache writeback per my recollection. I recall the problem always being that it's a speculative access to xPU protected memory. If there's a cacheable mapping in the non-secure page tables then it may be loaded at the bus with the non-secure bit set (NS). Once the xPU sees that it reboots the system. It used to be that we could never map secure memory regions in the kernel. I suspect with EL2 the story changes slightly. The hypervisor is the one mapping cmd-db at stage2, so any speculative access goes on the bus as EL2 tagged, and thus "approved" by the xPU. Then if the hypervisor sees EL1 (secure or non-secure) access cmd-db, it traps and makes sure it can actually access that address. If not, the hypervisor "panics" and reboots. Either way, EL1 can have a cacheable mapping and EL2 can make sure the secrets are safe, while the cache never goes out to the bus as anything besides EL2. > > 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 > How is Xen mapping this protected memory region? It sounds like maybe that should be mapped differently. Also, how is EL2 accessible on this device?
Hi Volodymyr/Maulik, On Fri, Mar 29, 2024 at 10:22:47AM +0530, Maulik Shah (mkshah) wrote: > > > On 3/29/2024 3:49 AM, Volodymyr Babchuk wrote: > > > > Hi Maulik > > > > "Maulik Shah (mkshah)" <quic_mkshah@quicinc.com> writes: > > > > > On 3/28/2024 1:39 AM, 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. > > > > > > Linux does not write into cmd-db region. This region is write > > > protected by XPU. Making this region uncached magically solves the XPU > > > write fault > > > issue. > > > > > > Can you please include above details? > > > > Sure, I'll add this to the next version. > > > > Thanks. > > > > > > > In downstream, we have below which resolved similar issue on qcm6490. > > > > > > cmd_db_header = memremap(rmem->base, rmem->size, MEMREMAP_WC); > > > > > > Downstream SA8155P also have MEMREMAP_WC. Can you please give it a try > > > on your device? > > > > Yes, MEMREMAP_WC works as well. This opens the question: which type is > > more correct? I have no deep understanding in QCOM internals so it is > > hard to me to answer this question. > > > > XPU may have falsely detected clean cache eviction as "write" into the write > protected region so using uncached flag MEMREMAP_WC may be helping here and > is more correct in my understanding. > I have got the very same explanation from my other colleagues at Qualcomm. I could reproduce the problem 100% of the time on QCS6490 RB3 board with Linux booting in EL2. The problem goes away with non-cached mapping (MEMREMAP_WC/MEMREMAP_WB). Do you guys plan to send V2? Please CC me on the V2. Thanks, Pavan
diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c index a5fd68411bed5..dd5ababdb476c 100644 --- a/drivers/soc/qcom/cmd-db.c +++ b/drivers/soc/qcom/cmd-db.c @@ -324,7 +324,7 @@ static int cmd_db_dev_probe(struct platform_device *pdev) return -EINVAL; } - cmd_db_header = memremap(rmem->base, rmem->size, MEMREMAP_WB); + cmd_db_header = memremap(rmem->base, rmem->size, MEMREMAP_WT); if (!cmd_db_header) { ret = -ENOMEM; cmd_db_header = NULL;
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 <volodymyr_babchuk@epam.com> --- drivers/soc/qcom/cmd-db.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)