From 3cfc550fc05ef6c96962b2efb60f8da07399c457 Mon Sep 17 00:00:00 2001 From: Alexander Richardson Date: Sun, 9 Jun 2024 14:08:54 -0700 Subject: [PATCH] arch-arm,mem: Don't hardcode secure mode accesses for semihosting (#1200) When accessing memory using functionalAccess(), the MMU could tell us to use a nonsecure access even though the CPU is operating in secure mode. I noticed this while trying to run a simple semihosting hello world with the MMU+caches enabled and the semihosting calls ended up reading from memory instead of the caches due to an S/NS mismatch. See also https://github.com/gem5/gem5/pull/1198 which happens to also mask the issue I saw, but I believe both changes are needed. Change-Id: I9e6b9839b194fbd41938e2225449c74701ea7fee --- src/arch/arm/mmu.cc | 5 +- src/arch/generic/mmu.cc | 4 +- src/mem/translating_port_proxy.cc | 6 +- src/mem/translation_gen.hh | 3 + src/mem/translation_gen.test.cc | 115 +++++++++++++++--------------- 5 files changed, 71 insertions(+), 62 deletions(-) diff --git a/src/arch/arm/mmu.cc b/src/arch/arm/mmu.cc index f94a18423a..44c3bb3d97 100644 --- a/src/arch/arm/mmu.cc +++ b/src/arch/arm/mmu.cc @@ -813,7 +813,8 @@ MMU::translateMmuOff(ThreadContext *tc, const RequestPtr &req, Mode mode, // security state of the processor if (state.isSecure) req->setFlags(Request::SECURE); - + else + req->clearFlags(Request::SECURE); if (state.aarch64) { bool selbit = bits(vaddr, 55); TCR tcr1 = tc->readMiscReg(MISCREG_TCR_EL1); @@ -917,6 +918,8 @@ MMU::translateMmuOn(ThreadContext* tc, const RequestPtr &req, Mode mode, if (state.isSecure && !te->ns) { req->setFlags(Request::SECURE); + } else { + req->clearFlags(Request::SECURE); } if (!is_fetch && fault == NoFault && (vaddr & mask(flags & AlignmentMask)) && diff --git a/src/arch/generic/mmu.cc b/src/arch/generic/mmu.cc index a765228dd5..feb03a0a70 100644 --- a/src/arch/generic/mmu.cc +++ b/src/arch/generic/mmu.cc @@ -149,8 +149,10 @@ BaseMMU::MMUTranslationGen::translate(Range &range) const range.fault = mmu->translateFunctional(req, tc, mode); - if (range.fault == NoFault) + if (range.fault == NoFault) { range.paddr = req->getPaddr(); + range.flags = req->getFlags(); + } } void diff --git a/src/mem/translating_port_proxy.cc b/src/mem/translating_port_proxy.cc index 8daa390d80..68de8dbb7a 100644 --- a/src/mem/translating_port_proxy.cc +++ b/src/mem/translating_port_proxy.cc @@ -92,7 +92,7 @@ TranslatingPortProxy::tryReadBlob(Addr addr, void *p, uint64_t size) const return tryOnBlob(mode, _tc->getMMUPtr()->translateFunctional( addr, size, _tc, mode, flags), [this, &p](const auto &range) { - PortProxy::readBlobPhys(range.paddr, flags, p, range.size); + PortProxy::readBlobPhys(range.paddr, range.flags, p, range.size); p = static_cast(p) + range.size; }); } @@ -105,7 +105,7 @@ TranslatingPortProxy::tryWriteBlob( return tryOnBlob(mode, _tc->getMMUPtr()->translateFunctional( addr, size, _tc, mode, flags), [this, &p](const auto &range) { - PortProxy::writeBlobPhys(range.paddr, flags, p, range.size); + PortProxy::writeBlobPhys(range.paddr, range.flags, p, range.size); p = static_cast(p) + range.size; }); } @@ -117,7 +117,7 @@ TranslatingPortProxy::tryMemsetBlob(Addr addr, uint8_t v, uint64_t size) const return tryOnBlob(mode, _tc->getMMUPtr()->translateFunctional( addr, size, _tc, mode, flags), [this, v](const auto &range) { - PortProxy::memsetBlobPhys(range.paddr, flags, v, range.size); + PortProxy::memsetBlobPhys(range.paddr, range.flags, v, range.size); }); } diff --git a/src/mem/translation_gen.hh b/src/mem/translation_gen.hh index ed5960aba2..b23f03114a 100644 --- a/src/mem/translation_gen.hh +++ b/src/mem/translation_gen.hh @@ -33,6 +33,7 @@ #include "base/logging.hh" #include "base/types.hh" +#include "mem/request.hh" namespace gem5 { @@ -71,6 +72,8 @@ class TranslationGen Addr size = 0; Addr paddr = 0; + // PTEs can also set the Secure/non-secure bit, so it is stored here. + Request::Flags flags = 0; Fault fault = NoFault; }; diff --git a/src/mem/translation_gen.test.cc b/src/mem/translation_gen.test.cc index 276e24027a..f4d81c895e 100644 --- a/src/mem/translation_gen.test.cc +++ b/src/mem/translation_gen.test.cc @@ -123,6 +123,7 @@ class TestTranslationGen : public TranslationGen // If there wasn't a fault, size and paddr are meaningful. range.size = resultPos->size; range.paddr = resultPos->paddr; + range.flags = resultPos->flags; } // Advance to the next result. resultPos++; @@ -161,8 +162,8 @@ TEST(TranslationGen, SuccessfulTwoStep) { TestTranslationGen gen(0x10000, 0x10000, { // Results for translate. - {0x0, 0x8000, 0x30000, NoFault}, - {0x0, 0x8000, 0x40000, NoFault} + {0x0, 0x8000, 0x30000, {}, NoFault}, + {0x0, 0x8000, 0x40000, {}, NoFault} }); RangeList range_list; @@ -171,15 +172,15 @@ TEST(TranslationGen, SuccessfulTwoStep) // What the generator should return. const RangeList expected_gen{ - {0x10000, 0x8000, 0x30000, NoFault}, - {0x18000, 0x8000, 0x40000, NoFault} + {0x10000, 0x8000, 0x30000, {}, NoFault}, + {0x18000, 0x8000, 0x40000, {}, NoFault} }; EXPECT_THAT(range_list, Pointwise(GenRangeEq(), expected_gen)); // What the generator should have been asked to translate. const RangeList expected_trans{ - {0x10000, 0x10000, 0x0, NoFault}, - {0x18000, 0x8000, 0x0, NoFault} + {0x10000, 0x10000, 0x0, {}, NoFault}, + {0x18000, 0x8000, 0x0, {}, NoFault} }; EXPECT_THAT(gen.args, Pointwise(TransRangeEq(), expected_trans)); } @@ -188,9 +189,9 @@ TEST(TranslationGen, RetryOnFault) { TestTranslationGen gen(0x10000, 0x10000, { // Results for translate. - {0x0, 0x8000, 0x30000, NoFault}, - {0x0, 0x0, 0x0, dummyFault1}, - {0x0, 0x8000, 0x40000, NoFault} + {0x0, 0x8000, 0x30000, {}, NoFault}, + {0x0, 0x0, 0x0, {}, dummyFault1}, + {0x0, 0x8000, 0x40000, {}, NoFault} }); RangeList range_list; @@ -199,17 +200,17 @@ TEST(TranslationGen, RetryOnFault) // What the generator should return. const RangeList expected_gen{ - {0x10000, 0x8000, 0x30000, NoFault}, - {0x18000, 0x0, 0x0, dummyFault1}, - {0x18000, 0x8000, 0x40000, NoFault} + {0x10000, 0x8000, 0x30000, {}, NoFault}, + {0x18000, 0x0, 0x0, {}, dummyFault1}, + {0x18000, 0x8000, 0x40000, {}, NoFault} }; EXPECT_THAT(range_list, Pointwise(GenRangeEq(), expected_gen)); // What the generator should have been asked to translate. const RangeList expected_trans{ - {0x10000, 0x10000, 0x0, NoFault}, - {0x18000, 0x8000, 0x0, NoFault}, - {0x18000, 0x8000, 0x0, NoFault} + {0x10000, 0x10000, 0x0, {}, NoFault}, + {0x18000, 0x8000, 0x0, {}, NoFault}, + {0x18000, 0x8000, 0x0, {}, NoFault} }; EXPECT_THAT(gen.args, Pointwise(TransRangeEq(), expected_trans)); } @@ -218,10 +219,10 @@ TEST(TranslationGen, RetryTwiceOnFault) { TestTranslationGen gen(0x10000, 0x10000, { // Results for translate. - {0x0, 0x8000, 0x30000, NoFault}, - {0x0, 0x0, 0x0, dummyFault1}, - {0x0, 0x0, 0x0, dummyFault2}, - {0x0, 0x8000, 0x40000, NoFault} + {0x0, 0x8000, 0x30000, {}, NoFault}, + {0x0, 0x0, 0x0, {}, dummyFault1}, + {0x0, 0x0, 0x0, {}, dummyFault2}, + {0x0, 0x8000, 0x40000, {}, NoFault} }); RangeList range_list; @@ -230,19 +231,19 @@ TEST(TranslationGen, RetryTwiceOnFault) // What the generator should return. const RangeList expected_gen{ - {0x10000, 0x8000, 0x30000, NoFault}, - {0x18000, 0x0, 0x0, dummyFault1}, - {0x18000, 0x0, 0x0, dummyFault2}, - {0x18000, 0x8000, 0x40000, NoFault} + {0x10000, 0x8000, 0x30000, {}, NoFault}, + {0x18000, 0x0, 0x0, {}, dummyFault1}, + {0x18000, 0x0, 0x0, {}, dummyFault2}, + {0x18000, 0x8000, 0x40000, {}, NoFault} }; EXPECT_THAT(range_list, Pointwise(GenRangeEq(), expected_gen)); // What the generator should have been asked to translate. const RangeList expected_trans{ - {0x10000, 0x10000, 0x0, NoFault}, - {0x18000, 0x8000, 0x0, NoFault}, - {0x18000, 0x8000, 0x0, NoFault}, - {0x18000, 0x8000, 0x0, NoFault} + {0x10000, 0x10000, 0x0, {}, NoFault}, + {0x18000, 0x8000, 0x0, {}, NoFault}, + {0x18000, 0x8000, 0x0, {}, NoFault}, + {0x18000, 0x8000, 0x0, {}, NoFault} }; EXPECT_THAT(gen.args, Pointwise(TransRangeEq(), expected_trans)); } @@ -251,9 +252,9 @@ TEST(TranslationGen, FaultAtStart) { TestTranslationGen gen(0x10000, 0x10000, { // Results for translate. - {0x0, 0x0, 0x0, dummyFault1}, - {0x0, 0x8000, 0x30000, NoFault}, - {0x0, 0x8000, 0x40000, NoFault} + {0x0, 0x0, 0x0, {}, dummyFault1}, + {0x0, 0x8000, 0x30000, {}, NoFault}, + {0x0, 0x8000, 0x40000, {}, NoFault} }); RangeList range_list; @@ -262,17 +263,17 @@ TEST(TranslationGen, FaultAtStart) // What the generator should return. const RangeList expected_gen{ - {0x10000, 0x0, 0x0, dummyFault1}, - {0x10000, 0x8000, 0x30000, NoFault}, - {0x18000, 0x8000, 0x40000, NoFault} + {0x10000, 0x0, 0x0, {}, dummyFault1}, + {0x10000, 0x8000, 0x30000, {}, NoFault}, + {0x18000, 0x8000, 0x40000, {}, NoFault} }; EXPECT_THAT(range_list, Pointwise(GenRangeEq(), expected_gen)); // What the generator should have been asked to translate. const RangeList expected_trans{ - {0x10000, 0x10000, 0x0, NoFault}, - {0x10000, 0x10000, 0x0, NoFault}, - {0x18000, 0x8000, 0x0, NoFault} + {0x10000, 0x10000, 0x0, {}, NoFault}, + {0x10000, 0x10000, 0x0, {}, NoFault}, + {0x18000, 0x8000, 0x0, {}, NoFault} }; EXPECT_THAT(gen.args, Pointwise(TransRangeEq(), expected_trans)); } @@ -281,10 +282,10 @@ TEST(TranslationGen, FaultInMiddle) { TestTranslationGen gen(0x10000, 0x18000, { // Results for translate. - {0x0, 0x8000, 0x30000, NoFault}, - {0x0, 0x0, 0x0, dummyFault1}, - {0x0, 0x8000, 0x40000, NoFault}, - {0x0, 0x8000, 0x50000, NoFault} + {0x0, 0x8000, 0x30000, {}, NoFault}, + {0x0, 0x0, 0x0, {}, dummyFault1}, + {0x0, 0x8000, 0x40000, {}, NoFault}, + {0x0, 0x8000, 0x50000, {}, NoFault} }); RangeList range_list; @@ -293,19 +294,19 @@ TEST(TranslationGen, FaultInMiddle) // What the generator should return. const RangeList expected_gen{ - {0x10000, 0x8000, 0x30000, NoFault}, - {0x18000, 0x0, 0x0, dummyFault1}, - {0x18000, 0x8000, 0x40000, NoFault}, - {0x20000, 0x8000, 0x50000, NoFault} + {0x10000, 0x8000, 0x30000, {}, NoFault}, + {0x18000, 0x0, 0x0, {}, dummyFault1}, + {0x18000, 0x8000, 0x40000, {}, NoFault}, + {0x20000, 0x8000, 0x50000, {}, NoFault} }; EXPECT_THAT(range_list, Pointwise(GenRangeEq(), expected_gen)); // What the generator should have been asked to translate. const RangeList expected_trans{ - {0x10000, 0x18000, 0x0, NoFault}, - {0x18000, 0x10000, 0x0, NoFault}, - {0x18000, 0x10000, 0x0, NoFault}, - {0x20000, 0x8000, 0x0, NoFault} + {0x10000, 0x18000, 0x0, {}, NoFault}, + {0x18000, 0x10000, 0x0, {}, NoFault}, + {0x18000, 0x10000, 0x0, {}, NoFault}, + {0x20000, 0x8000, 0x0, {}, NoFault} }; EXPECT_THAT(gen.args, Pointwise(TransRangeEq(), expected_trans)); } @@ -314,9 +315,9 @@ TEST(TranslationGen, VariablePageSize) { TestTranslationGen gen(0x10000, 0x20000, { // Results for translate. - {0x0, 0x8000, 0x30000, NoFault}, - {0x0, 0x10000, 0x40000, NoFault}, - {0x0, 0x8000, 0x50000, NoFault} + {0x0, 0x8000, 0x30000, {}, NoFault}, + {0x0, 0x10000, 0x40000, {}, NoFault}, + {0x0, 0x8000, 0x50000, {}, NoFault} }); RangeList range_list; @@ -325,17 +326,17 @@ TEST(TranslationGen, VariablePageSize) // What the generator should return. const RangeList expected_gen{ - {0x10000, 0x8000, 0x30000, NoFault}, - {0x18000, 0x10000, 0x40000, NoFault}, - {0x28000, 0x8000, 0x50000, NoFault} + {0x10000, 0x8000, 0x30000, {}, NoFault}, + {0x18000, 0x10000, 0x40000, {}, NoFault}, + {0x28000, 0x8000, 0x50000, {}, NoFault} }; EXPECT_THAT(range_list, Pointwise(GenRangeEq(), expected_gen)); // What the generator should have been asked to translate. const RangeList expected_trans{ - {0x10000, 0x20000, 0x0, NoFault}, - {0x18000, 0x18000, 0x0, NoFault}, - {0x28000, 0x8000, 0x0, NoFault} + {0x10000, 0x20000, 0x0, {}, NoFault}, + {0x18000, 0x18000, 0x0, {}, NoFault}, + {0x28000, 0x8000, 0x0, {}, NoFault} }; EXPECT_THAT(gen.args, Pointwise(TransRangeEq(), expected_trans)); }