Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: mcontrol/mcontrol6 on fetch #1459

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix: mcontrol/mcontrol6 on fetch #1459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0578364
triggers: fix data mcontrol/mcontrol6 on fetching instruction data
YenHaoChen Sep 14, 2023
146e41e
triggers: fix address mcontrol/mcontrol6 on fetching PC
YenHaoChen Sep 14, 2023
0d8067b
triggers: refactor: move access_info to where it is used in mmu_t::tr…
YenHaoChen Sep 13, 2023
661c6a2
triggers: performance improvement on execute mcontrol/mcontrol6
YenHaoChen Sep 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions riscv/mmu.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,10 @@ reg_t mmu_t::translate(mem_access_info_t access_info, reg_t len)

tlb_entry_t mmu_t::fetch_slow_path(reg_t vaddr)
{
auto access_info = generate_access_info(vaddr, FETCH, {false, false, false});
check_triggers(triggers::OPERATION_EXECUTE, vaddr, access_info.effective_virt);

tlb_entry_t result;
reg_t vpn = vaddr >> PGSHIFT;
if (unlikely(tlb_insn_tag[vpn % TLB_ENTRIES] != (vpn | TLB_CHECK_TRIGGERS))) {
auto access_info = generate_access_info(vaddr, FETCH, {false, false, false});
reg_t paddr = translate(access_info, sizeof(fetch_temp));
if (auto host_addr = sim->addr_to_mem(paddr)) {
result = refill_tlb(vaddr, paddr, host_addr, FETCH);
Expand All @@ -88,8 +86,6 @@ tlb_entry_t mmu_t::fetch_slow_path(reg_t vaddr)
result = tlb_data[vpn % TLB_ENTRIES];
}

check_triggers(triggers::OPERATION_EXECUTE, vaddr, access_info.effective_virt, from_le(*(const uint16_t*)(result.host_offset + vaddr)));

return result;
}

Expand Down
6 changes: 6 additions & 0 deletions riscv/mmu.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,10 @@ class mmu_t
if (matched_trigger)
throw *matched_trigger;

reg_t vpn = addr >> PGSHIFT;
auto access_info = generate_access_info(addr, FETCH, {false, false, false});
if (unlikely(tlb_insn_tag[vpn % TLB_ENTRIES] != vpn))
check_triggers(triggers::OPERATION_EXECUTE, addr, access_info.effective_virt);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this will spuriously invoke check_triggers on ITLB misses. Note that fetches from MMIO devices accesses always result in ITLB misses, so this means check_triggers will be invoked on every MMIO fetch. Maybe this is functionally correct and is only a perf issue, but it seems undesirable.

What's wrong with my original suggestion of moving this check back into fetch_slow_path? By construction, we should not need to check the triggers on the fetch fast path...

Copy link
Collaborator Author

@YenHaoChen YenHaoChen Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation splits instruction fetching into multiple 2-byte operations, which call fetch_slow_path separately. This results in that a 4-byte instruction 0xe9830313 matches data triggers with tdata2={0xe983, 0x0313} instead of one with tdata2=0xe9830313 incorrectly. Additionally, a 4-byte instruction with PC at 0x4000 matches address triggers with tdata2={0x4000, 0x4002}, which is unexpected from a user perspective.

The behavior of the data trigger in the current implementation does not conform to the specification. It is weird to me that the address trigger matches PCs of 2-byte granularity. Clarify whether the specification allows the address trigger to match an arbitrary subset of the accessing addresses: riscv/riscv-debug-spec#877.

Update 2023.09.20:
The riscv/riscv-debug-spec#877 clarifies that the specification allows the address trigger to match an arbitrary subset of the accessing addresses. Thus, the specification allows the original behavior of the address trigger. Possibly, we can improve performance by keeping address trigger checking in fetch_slow_path. However, the data trigger truly does not work in fetch_slow_path. Moving address triggers out from fetch_slow_path also provides a more reasonable behavior from a user perspective.
@aswaterman How do you feel about this?

auto tlb_entry = translate_insn_addr(addr);
insn_bits_t insn = from_le(*(uint16_t*)(tlb_entry.host_offset + addr));
int length = insn_length(insn);
Expand All @@ -307,6 +311,8 @@ class mmu_t
insn |= (insn_bits_t)from_le(*(const uint16_t*)translate_insn_addr_to_host(addr + 4)) << 32;
insn |= (insn_bits_t)from_le(*(const uint16_t*)translate_insn_addr_to_host(addr + 6)) << 48;
}
if (unlikely(tlb_insn_tag[vpn % TLB_ENTRIES] != vpn))
check_triggers(triggers::OPERATION_EXECUTE, addr, access_info.effective_virt, insn);

insn_fetch_t fetch = {proc->decode_insn(insn), insn};
entry->tag = addr;
Expand Down
Loading