Re: [Qemu-devel] [Qemu-riscv] [PATCHv3 5/5] RISC-V: Fix a PMP check with the correct access size

From: Hesham Almatary <hesham.almatary@cl.cam.ac.uk>
To: Jonathan Behrens <fintelia@gmail.com>
Cc: "open list:RISC-V" <qemu-riscv@nongnu.org>,
	Sagar Karandikar <sagark@eecs.berkeley.edu>,
	Bastian Koppelmann <kbastian@mail.uni-paderborn.de>,
	Palmer Dabbelt <palmer@sifive.com>,
	"qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>,
	Alistair Francis <Alistair.Francis@wdc.com>,
	Alistair Francis <alistair23@gmail.com>
Subject: Re: [Qemu-devel] [Qemu-riscv] [PATCHv3 5/5] RISC-V: Fix a PMP check with the correct access size
Date: Wed, 22 May 2019 09:55:01 +0100	[thread overview]
Message-ID: <CA+wsVCAiJ9i4yDooV3ZsAFM87m75Jx-hLbAMupfJdJvP-S-PGg@mail.gmail.com> (raw)
In-Reply-To: <CANnJOVG-fqEsRqOu3e8Jd=OanGwi2eEKPK0_AqGcMV28QzVz+g@mail.gmail.com>

On Wed, 22 May 2019 at 03:25, Jonathan Behrens <fintelia@gmail.com> wrote:
>
> Hesham,
>
> I don't think this is quite right. If I understand correctly, PMP permissions are only validated on TLB fills, not on all accesses. (Is anyone able to confirm this?) If so, this function can't just validate the range of a single access and then place the entire page into the TLB. However, the current code is also wrong because an access should succeed/fail based on the permissions only for the range it actually touches even regardless of the permissions on the rest of the page. Now that I think about it, I'd also expect that somewhere in the PMP logic would flush the TLB every time any of the related control registers change though I can't find anywhere that this is happening...
>
I believe the TLB fill function is called on all accesses, but I might
be wrong. I will wait for someone to confirm otherwise.

It's mentioned in the spec that sfence.vma has to be executed after
changing PMP configs, so it's a SW concern (i.e., not QEMU's).

> Sorry to keep raising complaints about this patch set, the interaction between physical memory protection and paging is very subtle. Even some real hardware has had errata related to it!
>
> Jonathan
>
> On Tue, May 21, 2019 at 6:33 PM Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Tue, May 21, 2019 at 3:45 AM Hesham Almatary
>> <Hesham.Almatary@cl.cam.ac.uk> wrote:
>> >
>> > The PMP check should be of the memory access size rather
>> > than TARGET_PAGE_SIZE.
>> >
>> > Signed-off-by: Hesham Almatary <Hesham.Almatary@cl.cam.ac.uk>
>>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>
>> Alistair
>>
>> > ---
>> >  target/riscv/cpu_helper.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> > index d0b0f9cf88..ce1f47e4e3 100644
>> > --- a/target/riscv/cpu_helper.c
>> > +++ b/target/riscv/cpu_helper.c
>> > @@ -410,7 +410,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>> >
>> >      if (riscv_feature(env, RISCV_FEATURE_PMP) &&
>> >          (ret == TRANSLATE_SUCCESS) &&
>> > -        !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type)) {
>> > +        !pmp_hart_has_privs(env, pa, size, 1 << access_type)) {
>> >          ret = TRANSLATE_PMP_FAIL;
>> >      }
>> >      if (ret == TRANSLATE_PMP_FAIL) {
>> > --
>> > 2.17.1
>> >
>> >
>>