Re: [PATCH v2 1/1] target/riscv/pmp: Fix bug preventing

[Chris Williams]

Hi,
Oct 11, 2019, 15:18 by address@hidden:

> On Sun, Oct 6, 2019 at 1:32 AM Chris Williams <address@hidden> wrote:
>
> Also please use `git format-patch` to format the patch and then `git
> send-email` to send the patch. There is a whole heap of detail here:
> https://wiki.qemu.org/Contribute/SubmitAPatch 
> <https://wiki.qemu.org/Contribute/SubmitAPatch>
>
OK, I will do in future. I read the page but failed to get it right. Thanks for 
spotting my patch, and the advice, though.

>> This fixes an issue that prevents a RISC-V CPU from executing instructions
>> immediately from the base address of a PMP TOR region.
>>
>> When jumping to an instruction in a PMP TOR region, pmp_hart_has_privs() is
>> called to validate the access. If this instruction is the very first word of 
>> a
>> PMP TOR region, at address 0 relative to the start address of the region, 
>> then
>> the access will fail. This is because pmp_hart_has_privs() is called with 
>> size
>> 0 to perform this validation, causing this check...
>>
>> e = pmp_is_in_range(env, i, addr + size - 1);
>>
>> ... to fail, as (addr + size - 1) falls below the base address of the PMP
>> region. Really, the access should succeed. For example, if I have a region
>> spanning 0x80d96000 to 0x88d95fff and the CPU jumps to 0x80d96000, then:
>>
>> s = 0x80d96000
>> e = 0x80d95fff
>>
>> And the validation fails. The size check proposed below catches these 
>> zero-size
>> instruction fetch access probes. The word alignment in pmpaddr{0-15} and
>> earlier instruction alignment checks should prevent the execution of
>> instructions over the upper boundary of the PMP region, though I'm happy to 
>> give
>> this more attention if this is a concern.
>>
>
> This seems like a similar issue to this patch as well:
> https://lore.kernel.org/qemu-devel/address@hidden/ 
> <https://lore.kernel.org/qemu-devel/address@hidden/>
>
Yes, it appears Dayeol and I have encountered the same issue.

> From that discussion:
>
> "In general, size 0 means "unknown size".  In this case, the one tlb lookup is
> going to be used by lots of instructions -- everything that fits on the page."
>
> Richard's last comment seems like a better fix:
>
> "You certainly could do
>
>  if (size == 0) {
>  size = -(addr | TARGET_PAGE_MASK);
>  }
>
> to assume that all bytes from addr to the end of the page are accessed.  That
> would avoid changing too much of the rest of the logic.
>
> That said, this code will continue to not work for mis-aligned boundaries."
>
> So I don't think this is the correct solution. I'm not sure if Dayeol
> is planning on sending a follow up version. If not feel free to send
> it.
>
I'm happy for Dayeol to submit a better patch, if necessary. 
>> Signed-off-by: Chris Williams <address@hidden <mailto:address@hidden>>
>>
>
> It looks like this is a HTML patch, also ensure all patches are just
> plain text, `git send-email` will do this.
>
Yes, you're right: my webmail client isn't particularly neighborly with respect 
to Qemu's submission process.

C.