riscvarchive / riscv-qemu

QEMU with RISC-V (RV64G, RV32G) Emulation Support
384 stars 154 forks source link

fix matching logic of pmp_hart_has_privs #165

Closed wxjstz closed 5 years ago

wxjstz commented 5 years ago

Should skip the empty PMP entry.

michaeljclark commented 5 years ago

This looks correct. I will need to test in my local tree.

If you say "Go!" and trust your patches, then i'll merge them. We can't regress many users here i.e. you are most likely one of the first real users on PMP in QEMU.

Say "Go!"?

Thanks, Michael.

michaeljclark commented 5 years ago

Perhaps I can put them on an qemu-inprogress branch and merge qemu-for-upstream and qemu-inprogress into riscv-all. We can put them in qemu-for-upstream after I have tests to verify locally.

This sounds like a good idea?

michaeljclark commented 5 years ago

Can you please re-target your PR to the new qemu-for-testing branch (i just made)?

I'll merge them there and integrate qemu-for-upstream and qemu-for-testing into riscv-all.

Alternatively if you have a test case I can try locally I can merge them straight to qemu-for-upstream (which has had extensive testing, just not yet for PMP).

Thanks very much indeed.

michaeljclark commented 5 years ago

You could also fold your series into one PR i.e. one inbound/outbound pull at a time. You don't need to do that but its less PRs to retarget. I can add your tree as a local remote and do this locally if I get to it first. thx

michaeljclark commented 5 years ago

I can't retarget PRs, so I added you as a remote and integrated locally.

Your changes are here: https://github.com/riscv/riscv-qemu/commits/qemu-for-testing

Closing this PR. If you target qemu-for-testing they can be merged easily, and i'll work on making them appear in riscv-all. Thanks!

wxjstz commented 5 years ago

I try to test my pmp code base on riscv-qemu/riscv-probe. This is the problem I found and the test passed.

However, riscv-qemu's pmp still has the error I want to solve.