From 013f773d3145a2a8985b30158935ecbb27a186cf Mon Sep 17 00:00:00 2001 From: Jason Lowe-Power Date: Thu, 20 Jun 2024 13:24:50 -0700 Subject: [PATCH] arch-riscv: Fix TLB lookup with vaddrs (#1264) Previously, all of the TLB lookup/insert functions were using the full virtual addresses even though the variables in the functions said "vpn." This change explicitly converts the virtual address to the VPN without any least significant zeros for the offset. I.e., vpn >> page_size. The main bug solved in this changeset is the asid was |'d with the upper bits of the virtual address, but sometimes there were all 1's. Therefore, you could get a TLB hit even if the ASID was different. Interestingly, the page that seemed to cause these issues was a 1 GiB page. This change also starts refactoring some of the page table details to support sv46 and sv57 page table formats. In my testing, the Linux kernel boot uses large pages (even OpenSBI uses large pages), so it seems that large pages also work. However, this seems like magic to me, so I'm not sure if it's correct. This change also updates some asserts, and debug statements with more useful debugging information. Partially fixes #1235. More testing needs to be done to be confident. --- src/arch/riscv/pagetable.cc | 15 ++++++ src/arch/riscv/pagetable.hh | 8 +++ src/arch/riscv/pagetable_walker.cc | 16 +++--- src/arch/riscv/tlb.cc | 84 +++++++++++++++++++++--------- src/arch/riscv/tlb.hh | 22 +++++++- 5 files changed, 112 insertions(+), 33 deletions(-) diff --git a/src/arch/riscv/pagetable.cc b/src/arch/riscv/pagetable.cc index b2901e947d..b3cbf08299 100644 --- a/src/arch/riscv/pagetable.cc +++ b/src/arch/riscv/pagetable.cc @@ -60,5 +60,20 @@ TlbEntry::unserialize(CheckpointIn &cp) UNSERIALIZE_SCALAR(lruSeq); } +Addr +getVPNFromVAddr(Addr vaddr, Addr mode) +{ + switch (mode) { + case SV39: + return bits(vaddr, 38, 12); + case SV48: + return bits(vaddr, 47, 12); + case SV57: + return bits(vaddr, 56, 12); + default: + panic("Unknown address translation mode %d\n", mode); + } +} + } // namespace RiscvISA } // namespace gem5 diff --git a/src/arch/riscv/pagetable.hh b/src/arch/riscv/pagetable.hh index 06a054faa9..3a3164fddf 100644 --- a/src/arch/riscv/pagetable.hh +++ b/src/arch/riscv/pagetable.hh @@ -53,6 +53,7 @@ enum AddrXlateMode BARE = 0, SV39 = 8, SV48 = 9, + SV57 = 10, }; // Sv39 paging @@ -76,6 +77,13 @@ BitUnion64(PTESv39) Bitfield<0> v; EndBitUnion(PTESv39) +/** + * Remove the page offset and the upper bits that are + * not part of the VPN from the address. + * Note that this must assume the smallest page size + */ +Addr getVPNFromVAddr(Addr vaddr, Addr mode); + struct TlbEntry; typedef Trie TlbEntryTrie; diff --git a/src/arch/riscv/pagetable_walker.cc b/src/arch/riscv/pagetable_walker.cc index f998a6445e..98180e480c 100644 --- a/src/arch/riscv/pagetable_walker.cc +++ b/src/arch/riscv/pagetable_walker.cc @@ -203,7 +203,8 @@ Walker::startWalkWrapper() // check if we get a tlb hit to skip the walk Addr vaddr = Addr(sext(currState->req->getVaddr())); - TlbEntry *e = tlb->lookup(vaddr, currState->satp.asid, currState->mode, + Addr vpn = getVPNFromVAddr(vaddr, currState->satp.mode); + TlbEntry *e = tlb->lookup(vpn, currState->satp.asid, currState->mode, true); Fault fault = NoFault; if (e) { @@ -242,7 +243,8 @@ Walker::startWalkWrapper() if (currStates.size()) { currState = currStates.front(); vaddr = Addr(sext(currState->req->getVaddr())); - e = tlb->lookup(vaddr, currState->satp.asid, currState->mode, + Addr vpn = getVPNFromVAddr(vaddr, currState->satp.mode); + e = tlb->lookup(vpn, currState->satp.asid, currState->mode, true); if (e) { fault = tlb->checkPermissions(currState->status, @@ -461,9 +463,10 @@ Walker::WalkerState::stepWalk(PacketPtr &write) } if (doTLBInsert) { - if (!functional) - walker->tlb->insert(entry.vaddr, entry); - else { + if (!functional) { + Addr vpn = getVPNFromVAddr(entry.vaddr, satp.mode); + walker->tlb->insert(vpn, entry); + } else { DPRINTF(PageTableWalker, "Translated %#x -> %#x\n", entry.vaddr, entry.paddr << PageShift | (entry.vaddr & mask(entry.logBytes))); @@ -571,7 +574,8 @@ Walker::WalkerState::recvPacket(PacketPtr pkt) */ Addr vaddr = req->getVaddr(); vaddr = Addr(sext(vaddr)); - Addr paddr = walker->tlb->translateWithTLB(vaddr, satp.asid, mode); + Addr paddr = walker->tlb->translateWithTLB(vaddr, satp.asid, + satp.mode, mode); req->setPaddr(paddr); // do pmp check if any checking condition is met. diff --git a/src/arch/riscv/tlb.cc b/src/arch/riscv/tlb.cc index dc9201d3e4..f6d23a8480 100644 --- a/src/arch/riscv/tlb.cc +++ b/src/arch/riscv/tlb.cc @@ -67,6 +67,12 @@ using namespace RiscvISA; static Addr buildKey(Addr vpn, uint16_t asid) { + // Note ASID is 16 bits + // The VPN in sv39 is up to 39-12=27 bits + // The VPN in sv48 is up to 48-12=36 bits + // The VPN in sv57 is up to 57-12=45 bits + // So, shifting the ASID into the top 16 bits is safe. + assert(bits(vpn, 63, 48) == 0); return (static_cast(asid) << 48) | vpn; } @@ -110,6 +116,12 @@ TLB::lookup(Addr vpn, uint16_t asid, BaseMMU::Mode mode, bool hidden) { TlbEntry *entry = trie.lookup(buildKey(vpn, asid)); + DPRINTF(TLBVerbose, "lookup(vpn=%#x, asid=%#x, key=%#x): " + "%s ppn=%#x (%#x) %s\n", + vpn, asid, buildKey(vpn, asid), entry ? "hit" : "miss", + entry ? entry->paddr : 0, entry ? entry->size() : 0, + hidden ? "hidden" : ""); + if (!hidden) { if (entry) entry->lruSeq = nextSeq(); @@ -131,9 +143,6 @@ TLB::lookup(Addr vpn, uint16_t asid, BaseMMU::Mode mode, bool hidden) else stats.readHits++; } - - DPRINTF(TLBVerbose, "lookup(vpn=%#x, asid=%#x): %s ppn %#x\n", - vpn, asid, entry ? "hit" : "miss", entry ? entry->paddr : 0); } return entry; @@ -142,15 +151,19 @@ TLB::lookup(Addr vpn, uint16_t asid, BaseMMU::Mode mode, bool hidden) TlbEntry * TLB::insert(Addr vpn, const TlbEntry &entry) { - DPRINTF(TLB, "insert(vpn=%#x, asid=%#x): ppn=%#x pte=%#x size=%#x\n", - vpn, entry.asid, entry.paddr, entry.pte, entry.size()); + DPRINTF(TLB, "insert(vpn=%#x, asid=%#x, key=%#x): " + "vaddr=%#x paddr=%#x pte=%#x size=%#x\n", + vpn, entry.asid, buildKey(vpn, entry.asid), entry.vaddr, entry.paddr, + entry.pte, entry.size()); // If somebody beat us to it, just use that existing entry. TlbEntry *newEntry = lookup(vpn, entry.asid, BaseMMU::Read, true); if (newEntry) { // update PTE flags (maybe we set the dirty/writable flag) newEntry->pte = entry.pte; - assert(newEntry->vaddr == vpn); + assert(newEntry->vaddr == entry.vaddr); + assert(newEntry->asid == entry.asid); + assert(newEntry->logBytes == entry.logBytes); return newEntry; } @@ -163,31 +176,46 @@ TLB::insert(Addr vpn, const TlbEntry &entry) Addr key = buildKey(vpn, entry.asid); *newEntry = entry; newEntry->lruSeq = nextSeq(); - newEntry->vaddr = vpn; - newEntry->trieHandle = - trie.insert(key, TlbEntryTrie::MaxBits - entry.logBytes, newEntry); + newEntry->trieHandle = trie.insert( + key, TlbEntryTrie::MaxBits - entry.logBytes + PageShift, newEntry + ); return newEntry; } void -TLB::demapPage(Addr vpn, uint64_t asid) +TLB::demapPage(Addr vaddr, uint64_t asid) { + // Note: vaddr is Reg[rs1] and asid is Reg[rs2] + // The definition of this instruction is + // if vaddr=x0 and asid=x0, then flush all + // if vaddr=x0 and asid!=x0 then flush all with matching asid + // if vaddr!=x0 and asid=x0 then flush all leaf PTEs that match vaddr + // if vaddr!=x0 and asid!=x0 then flush the leaf PTE that matches vaddr + // in the given asid. + // No effect if vaddr is not valid + // Currently, we assume if the values of the registers are 0 then it was + // referencing x0. + asid &= 0xFFFF; - if (vpn == 0 && asid == 0) + DPRINTF(TLB, "flush(vaddr=%#x, asid=%#x)\n", vaddr, asid); + if (vaddr == 0 && asid == 0) { + DPRINTF(TLB, "Flushing all TLB entries\n"); flushAll(); - else { - DPRINTF(TLB, "flush(vpn=%#x, asid=%#x)\n", vpn, asid); - if (vpn != 0 && asid != 0) { - TlbEntry *newEntry = lookup(vpn, asid, BaseMMU::Read, true); - if (newEntry) - remove(newEntry - tlb.data()); + } else { + if (vaddr != 0 && asid != 0) { + // TODO: When supporting other address translation modes, fix this + Addr vpn = getVPNFromVAddr(vaddr, AddrXlateMode::SV39); + TlbEntry *entry = lookup(vpn, asid, BaseMMU::Read, true); + if (entry) { + remove(entry - tlb.data()); + } } else { for (size_t i = 0; i < size; i++) { if (tlb[i].trieHandle) { Addr mask = ~(tlb[i].size() - 1); - if ((vpn == 0 || (vpn & mask) == tlb[i].vaddr) && + if ((vaddr == 0 || (vaddr & mask) == tlb[i].vaddr) && (asid == 0 || tlb[i].asid == asid)) remove(i); } @@ -267,9 +295,10 @@ TLB::createPagefault(Addr vaddr, BaseMMU::Mode mode) } Addr -TLB::translateWithTLB(Addr vaddr, uint16_t asid, BaseMMU::Mode mode) +TLB::translateWithTLB(Addr vaddr, uint16_t asid, Addr xmode, + BaseMMU::Mode mode) { - TlbEntry *e = lookup(vaddr, asid, mode, false); + TlbEntry *e = lookup(getVPNFromVAddr(vaddr, xmode), asid, mode, false); assert(e != nullptr); return e->paddr << PageShift | (vaddr & mask(e->logBytes)); } @@ -284,7 +313,8 @@ TLB::doTranslate(const RequestPtr &req, ThreadContext *tc, Addr vaddr = Addr(sext(req->getVaddr())); SATP satp = tc->readMiscReg(MISCREG_SATP); - TlbEntry *e = lookup(vaddr, satp.asid, mode, false); + Addr vpn = getVPNFromVAddr(vaddr, satp.mode); + TlbEntry *e = lookup(vpn, satp.asid, mode, false); if (!e) { Fault fault = walker->start(tc, translation, req, mode); if (translation != nullptr || fault != NoFault) { @@ -292,7 +322,7 @@ TLB::doTranslate(const RequestPtr &req, ThreadContext *tc, delayed = true; return fault; } - e = lookup(vaddr, satp.asid, mode, true); + e = lookup(vpn, satp.asid, mode, true); assert(e != nullptr); } @@ -315,8 +345,8 @@ TLB::doTranslate(const RequestPtr &req, ThreadContext *tc, } Addr paddr = e->paddr << PageShift | (vaddr & mask(e->logBytes)); - DPRINTF(TLBVerbose, "translate(vpn=%#x, asid=%#x): %#x\n", - vaddr, satp.asid, paddr); + DPRINTF(TLBVerbose, "translate(vaddr=%#x, vpn=%#x, asid=%#x): %#x\n", + vaddr, vpn, satp.asid, paddr); req->setPaddr(paddr); return NoFault; @@ -505,9 +535,11 @@ TLB::unserialize(CheckpointIn &cp) freeList.pop_front(); newEntry->unserializeSection(cp, csprintf("Entry%d", x)); - Addr key = buildKey(newEntry->vaddr, newEntry->asid); + // TODO: When supporting other addressing modes fix this + Addr vpn = getVPNFromVAddr(newEntry->vaddr, AddrXlateMode::SV39); + Addr key = buildKey(vpn, newEntry->asid); newEntry->trieHandle = trie.insert(key, - TlbEntryTrie::MaxBits - newEntry->logBytes, newEntry); + TlbEntryTrie::MaxBits - newEntry->logBytes + PageShift, newEntry); } } diff --git a/src/arch/riscv/tlb.hh b/src/arch/riscv/tlb.hh index 389c1f995c..afe2c90db8 100644 --- a/src/arch/riscv/tlb.hh +++ b/src/arch/riscv/tlb.hh @@ -97,6 +97,14 @@ class TLB : public BaseTLB void takeOverFrom(BaseTLB *old) override {} + /** + * Insert an entry into the TLB. + * @param vpn The virtual page number extracted from the address. + * It is shifted based on the page size. We assume the + * smallest defined page size and remove the upper bits of the + * virtual address that are not part of the page number. + * @param entry The entry to insert. + */ TlbEntry *insert(Addr vpn, const TlbEntry &entry); void flushAll() override; void demapPage(Addr vaddr, uint64_t asn) override; @@ -123,7 +131,8 @@ class TLB : public BaseTLB */ Port *getTableWalkerPort() override; - Addr translateWithTLB(Addr vaddr, uint16_t asid, BaseMMU::Mode mode); + Addr translateWithTLB(Addr vaddr, uint16_t asid, Addr xmode, + BaseMMU::Mode mode); Fault translateAtomic(const RequestPtr &req, ThreadContext *tc, BaseMMU::Mode mode) override; @@ -134,6 +143,17 @@ class TLB : public BaseTLB BaseMMU::Mode mode) override; Fault finalizePhysical(const RequestPtr &req, ThreadContext *tc, BaseMMU::Mode mode) const override; + + /** + * Perform the tlb lookup + * @param vpn The virtual page number extracted from the address. + * It is shifted based on the page size. We assume the + * smallest defined page size and remove the upper bits of the + * virtual address that are not part of the page number. + * @param asid The address space identifier as specified by satp. + * @param mode The mode of the memory operation. + * @param hidden If the lookup should be hidden from the statistics. + */ TlbEntry *lookup(Addr vpn, uint16_t asid, BaseMMU::Mode mode, bool hidden); private: