Hi, Alistair, Thank you for reminding me. I already had the local patch, so I re-submitted the patch. Please let me know if that's fair enough (or you have any other comments)
Thanks, Dayeol On Fri, Oct 11, 2019 at 3:24 PM Alistair Francis <alistai...@gmail.com> wrote: > On Sun, Oct 6, 2019 at 1:32 AM Chris Williams <diodes...@tuta.io> 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: > https://wiki.qemu.org/Contribute/SubmitAPatch > > > > > 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/20191007052813.25814-1-day...@berkeley.edu/ > > 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. > > > > > Signed-off-by: Chris Williams <diodes...@tuta.io <mailto: > diodes...@tuta.io>> > > 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 :) > > Alistair > > > > > 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) { > > > > >
