search

riscvarchive / riscv-qemu

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

PMP address range & NAPOT decoding bugs #150

Closed dayeol closed 6 years ago

dayeol commented 6 years ago

Suggested fix for the issue #149

michaeljclark commented 6 years ago

Just to note the PMP code has not been thoroughly tested. I would like to write some PMP test cases at some point. riscv-tests currently does not have any PMP tests.

Also note there were some recent changes with respect to PMP granularity in the latest draft spec. Currently PMPs are checked in get_physical_address on the TLB slow path so the granularity in QEMU’s PMP implementation is 12-bits (PAGE_SIZE). Will have to find the wording to see how we are supposed to correctly handle this granularity.

dayeol commented 6 years ago

@michaeljclark Thank you for your comment. I see. Does that mean we postpone this fix until we have the tests?

Speaking of granularity, can we add the page offset with the pa before we call pmp_hart_has_privs? Thank you.

michaeljclark commented 6 years ago

On 17/07/2018, at 4:52 PM, Dayeol Lee notifications@github.com wrote:

@michaeljclark Thank you for your comment. I see. Does that mean we postpone this fix until we have the tests?

Speaking of granularity, can we add the page offset with the pa before we call pmp_hart_has_privs?

It is static. It will always be TARGET_PAGE_SIZE which is 4096 bytes.

Here is the text from the spec that I was looking for: https://github.com/riscv/riscv-isa-manual/blob/master/src/machine.tex

“\begin{commentary} Software may determine the PMP granularity by writing zero to {\tt pmp0cfg}, then writing all ones to {\tt pmpaddr0}, then reading back {\tt pmpaddr0}. If $G$ is the index of the least-significant bit set, the PMP granularity is $2^{G+2}$ bytes. \end{commentary}”

Given we have to indicate granularity detection via this method, then the PMP code also needs to be statically aware of the granule size. This is a new requirement recently added to the spec so it has not been implemented in the PMP code.

I’m happy to apply fixes to this code but it would be nice to have some test cases... :-D