[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Alistair Francis
Subject: Re: [PATCH v2 1/1] target/riscv/pmp: Fix bug preventing
Date: Fri, 11 Oct 2019 15:18:56 -0700

On Sun, Oct 6, 2019 at 1:32 AM Chris Williams <address@hidden> wrote:
> Hello. I hope you don't mind me resubmitting this patch. Please let me know if

Not at all!

Thanks for the patch.

In future if people don't respond you can just keep pinging your patch.

> I've formatted it incorrectly or if it needs more explanation. My previous
> attempt probably wasn't formatted quite right. This is my first time
> contributing to Qemu, so I'm keen to get it right - thanks.

This whole paragraph should not be in the commit. It can go below the
line though.

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:

> 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:

>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

> 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.

Thanks for the patch though! Please send any more fixes that you find :)


> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index d4f1007109..9308672e20 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -235,8 +235,9 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong
> addr,
>      /* 1.10 draft priv spec states there is an implicit order
>           from low to high */
>      for (i = 0; i < MAX_RISCV_PMPS; i++) {
> +        /* catch zero-size instruction checks */
>          s = pmp_is_in_range(env, i, addr);
> -        e = pmp_is_in_range(env, i, addr + size - 1);
> +        e = pmp_is_in_range(env, i, (size == 0) ? addr : addr + size - 1);
>          /* partially inside */
>          if ((s + e) == 1) {

reply via email to

[Prev in Thread] Current Thread [Next in Thread]