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.

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

[Bug Report] Wrong exception priority during access memory #971

Closed
Phantom1003 opened this issue Apr 11, 2022 · 6 comments
Closed

[Bug Report] Wrong exception priority during access memory #971

Phantom1003 opened this issue Apr 11, 2022 · 6 comments

Comments

@Phantom1003
Copy link
Contributor

Let's take the load instruction as an example:

riscv-isa-sim/riscv/mmu.h

Lines 99 to 125 in 0f15aa0

#define load_func(type, prefix, xlate_flags) \
inline type##_t prefix##_##type(reg_t addr, bool require_alignment = false) { \
if (unlikely(addr & (sizeof(type##_t)-1))) { \
if (require_alignment) load_reserved_address_misaligned(addr); \
else return misaligned_load(addr, sizeof(type##_t), xlate_flags); \
} \
reg_t vpn = addr >> PGSHIFT; \
size_t size = sizeof(type##_t); \
if ((xlate_flags) == 0 && likely(tlb_load_tag[vpn % TLB_ENTRIES] == vpn)) { \
if (proc) READ_MEM(addr, size); \
return from_target(*(target_endian<type##_t>*)(tlb_data[vpn % TLB_ENTRIES].host_offset + addr)); \
} \
if ((xlate_flags) == 0 && unlikely(tlb_load_tag[vpn % TLB_ENTRIES] == (vpn | TLB_CHECK_TRIGGERS))) { \
type##_t data = from_target(*(target_endian<type##_t>*)(tlb_data[vpn % TLB_ENTRIES].host_offset + addr)); \
if (!matched_trigger) { \
matched_trigger = trigger_exception(OPERATION_LOAD, addr, data); \
if (matched_trigger) \
throw *matched_trigger; \
} \
if (proc) READ_MEM(addr, size); \
return data; \
} \
target_endian<type##_t> res; \
load_slow_path(addr, sizeof(type##_t), (uint8_t*)&res, (xlate_flags)); \
if (proc) READ_MEM(addr, size); \
return from_target(res); \
}

At line 101, load will first check if it is aligned, then at line 122 it will try to access the address in the load_slow_path function.

riscv-isa-sim/riscv/mmu.cc

Lines 142 to 162 in 0f15aa0

void mmu_t::load_slow_path(reg_t addr, reg_t len, uint8_t* bytes, uint32_t xlate_flags)
{
reg_t paddr = translate(addr, len, LOAD, xlate_flags);
if (auto host_addr = sim->addr_to_mem(paddr)) {
memcpy(bytes, host_addr, len);
if (tracer.interested_in_range(paddr, paddr + PGSIZE, LOAD))
tracer.trace(paddr, len, LOAD);
else if (xlate_flags == 0)
refill_tlb(addr, paddr, host_addr, LOAD);
} else if (!mmio_load(paddr, len, bytes)) {
throw trap_load_access_fault((proc) ? proc->state.v : false, addr, 0, 0);
}
if (!matched_trigger) {
reg_t data = reg_from_bytes(len, bytes);
matched_trigger = trigger_exception(OPERATION_LOAD, addr, data);
if (matched_trigger)
throw *matched_trigger;
}
}

In load_slow_path, it will first check if it is legal address at line 153, and the watch point will be checked at the end of the function.

Briefly, the order of priority is as follows: trap_load_address_misaligned > trap_load_access_fault > trap_breakpoint

However, in the specification, trap_breakpoint has a higher priority than the others:
image

We also co-simulate with rocket to check this point, rocket threw a breakpoint exception, while spike threw an error misaligned exception.
The test point is at 0x800001c0 where loading a misaligned illegal address 0x100004001:

3 0x00800001bc (0x7a261073)
core   0: 0x00000000800001bc (0x7a261073) csrw    tdata2, a2
3 0x0080000004 (0x34302f73) x30 0x0000000100004001
core   0: 0x00000000800001c0 (0x00062603) lw      a2, 0(a2)
core   0: exception trap_load_address_misaligned, epc 0x00000000800001c0
core   0:           tval 0x0000000100004001
core   0: 0x0000000080000004 (0x34302f73) csrr    t5, mtval
3 0x0080000008 (0x34202f73) x30 0x0000000000000003
core   0: 0x0000000080000008 (0x34202f73) csrr    t5, mcause
[error] WDATA SIM 0000000000000004, DUT 0000000000000003
[error] check board clear 30 error
[CJ] integer register Judge Failed

spike-0.zip

@scottj97
Copy link
Collaborator

I believe this is a duplicate of #538

@Phantom1003
Copy link
Contributor Author

Yes, but the priority of the misaligned exception is also wrong, which is outside of the load_slow_path function.

@scottj97
Copy link
Collaborator

@timsifive perhaps this is of interest to you, since you're doing some trigger work now.

@timsifive
Copy link
Collaborator

I agree that this is wrong, and a (exceptionally clearly described) problem. It's been a long time since I added that trigger code. I'm not sure how to raise the trigger priority while minimizing the performance impact.

I might have time to look at this in a few weeks, when I'm back from vacation.

timsifive added a commit that referenced this issue May 25, 2022
With this change, #971 is resolved for loads.
timsifive added a commit that referenced this issue May 25, 2022
Fixes the other half of #971.

Compared the start of this set of changes, it now takes 11% longer to
run the towers benchmark (with 22 discs).
timsifive added a commit that referenced this issue May 26, 2022
With this change, #971 is resolved for loads.
timsifive added a commit that referenced this issue May 26, 2022
Fixes the other half of #971.

Compared the start of this set of changes, it now takes 11% longer to
run the towers benchmark (with 22 discs).
timsifive added a commit that referenced this issue May 27, 2022
When the slow path occurs, don't check for alignment in load_fast().

I haven't tested unaligned accesses, nor different endianness.

With this change, #971 is resolved for loads.
timsifive added a commit that referenced this issue May 27, 2022
Fixes the other half of #971.

Compared the start of this set of changes, it now takes 11% longer to
run the towers benchmark (with 22 discs).
timsifive added a commit that referenced this issue Jun 10, 2022
When the slow path occurs, don't check for alignment in load_fast().

I haven't tested different endianness.

With this change, #971 is resolved for loads.
timsifive added a commit that referenced this issue Jun 10, 2022
Fixes the other half of #971.

Compared the start of this set of changes, it now takes 11% longer to
run the towers benchmark (with 22 discs).
@scottj97
Copy link
Collaborator

scottj97 commented Dec 5, 2022

@Phantom1003 can you confirm if #1105 fixed this for you?

@Phantom1003
Copy link
Contributor Author

Yes, the breakpoint can now be triggered successfully.

YenHaoChen added a commit to YenHaoChen/riscv-isa-sim that referenced this issue Dec 6, 2022
This commit fixes the following two issues on AMO triggers:
1. The address trigger has a higher priority than the misaligned fault
and page fault. [riscv-software-src#971]
2. AMO can fire on a chain with a load-only trigger and a store-only
trigger. [riscv/riscv-debug-spec#780]

This commit does NOT fix the priority for data trigger on AMO.
YenHaoChen added a commit to YenHaoChen/riscv-isa-sim that referenced this issue Dec 6, 2022
This commit fixes the following two issues for address trigger on AMO:
1. The address trigger has a higher priority than the misaligned fault
and page fault. [riscv-software-src#971] [riscv-software-src#1105]
2. AMO can fire on a chain with a load-only trigger and a store-only
trigger. [riscv/riscv-debug-spec#780]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants