| Message ID | 20190703084048.6980-5-palmer@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL] RISC-V Patches for the 4.1 Soft Freeze, Part 2 v3 | expand |
On Wed, 3 Jul 2019 at 09:41, Palmer Dabbelt <palmer@sifive.com> wrote: > > From: Michael Clark <mjc@sifive.com> > > This patch adds support for the riscv_cpu_unassigned_access call > and will raise a load or store access fault. > > Signed-off-by: Michael Clark <mjc@sifive.com> > [Changes by AF: > - Squash two patches and rewrite commit message > - Set baddr to the access address > ] > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > Reviewed-by: Palmer Dabbelt <palmer@sifive.com> > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> Oops, I missed seeing this go by. The do_unassigned_access hook is deprecated and you should drop this and use the do_transaction_failed hook instead. The distinction between the two is that do_unassigned_access will end up being called for any failing access, including not just "normal" guest accesses but also for bad accesses that happen during page table walks (which often want to be reported to the guest differently) and also accesses by random devices like DMA controllers (where throwing a cpu exception is always a bug). Changing the hook implementation itself should be straightforward; commit 6ad4d7eed05a1e23537f is an example of doing that on Alpha. You also want to check all the places in your target code that do physical memory accesses, determine what the right behaviour if they get a bus fault is, and implement that (or at least put in TODO comments). thanks -- PMM
On Thu, 01 Aug 2019 08:39:17 PDT (-0700), Peter Maydell wrote: > On Wed, 3 Jul 2019 at 09:41, Palmer Dabbelt <palmer@sifive.com> wrote: >> >> From: Michael Clark <mjc@sifive.com> >> >> This patch adds support for the riscv_cpu_unassigned_access call >> and will raise a load or store access fault. >> >> Signed-off-by: Michael Clark <mjc@sifive.com> >> [Changes by AF: >> - Squash two patches and rewrite commit message >> - Set baddr to the access address >> ] >> Signed-off-by: Alistair Francis <alistair.francis@wdc.com> >> Reviewed-by: Palmer Dabbelt <palmer@sifive.com> >> Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > > Oops, I missed seeing this go by. The do_unassigned_access > hook is deprecated and you should drop this and use > the do_transaction_failed hook instead. > > The distinction between the two is that do_unassigned_access > will end up being called for any failing access, including > not just "normal" guest accesses but also for bad accesses > that happen during page table walks (which often want to > be reported to the guest differently) and also accesses > by random devices like DMA controllers (where throwing a > cpu exception is always a bug). > > Changing the hook implementation itself should be straightforward; > commit 6ad4d7eed05a1e23537f is an example of doing that on Alpha. > You also want to check all the places in your target code that > do physical memory accesses, determine what the right behaviour > if they get a bus fault is, and implement that (or at least put > in TODO comments). Sorry, updating that has been on my TODO list for a while now. I figured it was better to have the deprecated version in there than nothing at all. I've written some patches to fix this, but I want to give them another look before sending them out.
On Tue, Aug 13, 2019 at 3:44 PM Palmer Dabbelt <palmer@sifive.com> wrote: > > On Thu, 01 Aug 2019 08:39:17 PDT (-0700), Peter Maydell wrote: > > On Wed, 3 Jul 2019 at 09:41, Palmer Dabbelt <palmer@sifive.com> wrote: > >> > >> From: Michael Clark <mjc@sifive.com> > >> > >> This patch adds support for the riscv_cpu_unassigned_access call > >> and will raise a load or store access fault. > >> > >> Signed-off-by: Michael Clark <mjc@sifive.com> > >> [Changes by AF: > >> - Squash two patches and rewrite commit message > >> - Set baddr to the access address > >> ] > >> Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > >> Reviewed-by: Palmer Dabbelt <palmer@sifive.com> > >> Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > > > > Oops, I missed seeing this go by. The do_unassigned_access > > hook is deprecated and you should drop this and use > > the do_transaction_failed hook instead. Argh! > > > > The distinction between the two is that do_unassigned_access > > will end up being called for any failing access, including > > not just "normal" guest accesses but also for bad accesses > > that happen during page table walks (which often want to > > be reported to the guest differently) and also accesses > > by random devices like DMA controllers (where throwing a > > cpu exception is always a bug). > > > > Changing the hook implementation itself should be straightforward; > > commit 6ad4d7eed05a1e23537f is an example of doing that on Alpha. > > You also want to check all the places in your target code that > > do physical memory accesses, determine what the right behaviour > > if they get a bus fault is, and implement that (or at least put > > in TODO comments). > > Sorry, updating that has been on my TODO list for a while now. I figured it > was better to have the deprecated version in there than nothing at all. I've > written some patches to fix this, but I want to give them another look before > sending them out. I was going to start looking into this, but if you already have patches I won't bother. Let me know if you want a hand with the conversion. Alistair >
On Thu, 15 Aug 2019 14:39:18 PDT (-0700), alistair23@gmail.com wrote: > On Tue, Aug 13, 2019 at 3:44 PM Palmer Dabbelt <palmer@sifive.com> wrote: >> >> On Thu, 01 Aug 2019 08:39:17 PDT (-0700), Peter Maydell wrote: >> > On Wed, 3 Jul 2019 at 09:41, Palmer Dabbelt <palmer@sifive.com> wrote: >> >> >> >> From: Michael Clark <mjc@sifive.com> >> >> >> >> This patch adds support for the riscv_cpu_unassigned_access call >> >> and will raise a load or store access fault. >> >> >> >> Signed-off-by: Michael Clark <mjc@sifive.com> >> >> [Changes by AF: >> >> - Squash two patches and rewrite commit message >> >> - Set baddr to the access address >> >> ] >> >> Signed-off-by: Alistair Francis <alistair.francis@wdc.com> >> >> Reviewed-by: Palmer Dabbelt <palmer@sifive.com> >> >> Signed-off-by: Palmer Dabbelt <palmer@sifive.com> >> > >> > Oops, I missed seeing this go by. The do_unassigned_access >> > hook is deprecated and you should drop this and use >> > the do_transaction_failed hook instead. > > Argh! > >> > >> > The distinction between the two is that do_unassigned_access >> > will end up being called for any failing access, including >> > not just "normal" guest accesses but also for bad accesses >> > that happen during page table walks (which often want to >> > be reported to the guest differently) and also accesses >> > by random devices like DMA controllers (where throwing a >> > cpu exception is always a bug). >> > >> > Changing the hook implementation itself should be straightforward; >> > commit 6ad4d7eed05a1e23537f is an example of doing that on Alpha. >> > You also want to check all the places in your target code that >> > do physical memory accesses, determine what the right behaviour >> > if they get a bus fault is, and implement that (or at least put >> > in TODO comments). >> >> Sorry, updating that has been on my TODO list for a while now. I figured it >> was better to have the deprecated version in there than nothing at all. I've >> written some patches to fix this, but I want to give them another look before >> sending them out. > > I was going to start looking into this, but if you already have > patches I won't bother. Let me know if you want a hand with the > conversion. You're more than welcome to take them over. I've got something that boots Linux on my unassigned_access branch (github.com/palmer-dabbelt/qemu), but I haven't sanitized the whole port for physical accesses and I haven't convinced myself that my hook implementation is correct.
On Thu, 15 Aug 2019 at 23:17, Palmer Dabbelt <palmer@sifive.com> wrote: > You're more than welcome to take them over. I've got something that boots > Linux on my unassigned_access branch (github.com/palmer-dabbelt/qemu), but I > haven't sanitized the whole port for physical accesses and I haven't convinced > myself that my hook implementation is correct. Rather than doing if (retaddr) { cpu_restore_state(cs, retaddr, true); } at the start of the hook I think you just want to pass 'retaddr' as the final argument to riscv_raise_exception() instead of using GETPC(). Other than that I think the hook itself is right. The 'git grep' regexes in docs/devel/loads-stores.rst are handy for finding the places where the target code is doing physical accesses. IIRC the only ones I found with a quick scan were the PTE loads in get_physical_address() via ldl_phys/ldq_phys, which will now return 0 and run into the 'invalid PTE' code path. I don't know whether your architecture requires some different behaviour for bus errors on page table walk than that (you might want to specifically code the error path anyway or comment it even if the behaviour is right, to be a bit more explicit that it can happen). thanks -- PMM
On Fri, 16 Aug 2019 at 09:57, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Thu, 15 Aug 2019 at 23:17, Palmer Dabbelt <palmer@sifive.com> wrote: > > You're more than welcome to take them over. I've got something that boots > > Linux on my unassigned_access branch (github.com/palmer-dabbelt/qemu), but I > > haven't sanitized the whole port for physical accesses and I haven't convinced > > myself that my hook implementation is correct. > > Rather than doing > if (retaddr) { > cpu_restore_state(cs, retaddr, true); > } > > at the start of the hook I think you just want to pass 'retaddr' > as the final argument to riscv_raise_exception() instead of > using GETPC(). Other than that I think the hook itself is right. > > The 'git grep' regexes in docs/devel/loads-stores.rst are handy > for finding the places where the target code is doing physical > accesses. IIRC the only ones I found with a quick scan were the > PTE loads in get_physical_address() via ldl_phys/ldq_phys, which will > now return 0 and run into the 'invalid PTE' code path. I don't > know whether your architecture requires some different behaviour > for bus errors on page table walk than that (you might want to > specifically code the error path anyway or comment it even if the > behaviour is right, to be a bit more explicit that it can happen). Gentle ping -- would somebody like to have a go at the riscv do_transaction_failed hook conversion? I think it should be straightforward, and riscv is now the only architecture still using the old unassigned_access hook and preventing us from getting rid of it entirely. thanks -- PMM
On Tue, Sep 17, 2019 at 6:56 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Fri, 16 Aug 2019 at 09:57, Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Thu, 15 Aug 2019 at 23:17, Palmer Dabbelt <palmer@sifive.com> wrote: > > > You're more than welcome to take them over. I've got something that boots > > > Linux on my unassigned_access branch (github.com/palmer-dabbelt/qemu), but I > > > haven't sanitized the whole port for physical accesses and I haven't convinced > > > myself that my hook implementation is correct. > > > > Rather than doing > > if (retaddr) { > > cpu_restore_state(cs, retaddr, true); > > } > > > > at the start of the hook I think you just want to pass 'retaddr' > > as the final argument to riscv_raise_exception() instead of > > using GETPC(). Other than that I think the hook itself is right. > > > > The 'git grep' regexes in docs/devel/loads-stores.rst are handy > > for finding the places where the target code is doing physical > > accesses. IIRC the only ones I found with a quick scan were the > > PTE loads in get_physical_address() via ldl_phys/ldq_phys, which will > > now return 0 and run into the 'invalid PTE' code path. I don't > > know whether your architecture requires some different behaviour > > for bus errors on page table walk than that (you might want to > > specifically code the error path anyway or comment it even if the > > behaviour is right, to be a bit more explicit that it can happen). > > Gentle ping -- would somebody like to have a go at the riscv > do_transaction_failed hook conversion? I think it should be > straightforward, and riscv is now the only architecture still > using the old unassigned_access hook and preventing us from > getting rid of it entirely. Thanks for the ping Peter, I forgot about this. @Palmer I have taken your patches, made some changes based on Peter's comments and rebased them on your PR branch. I'll double check, but the hook implementation looks correct and I can't see any other obvious unsanitised physical accesses so it should be ok. I'll send them out today if I don't find any issues. Alistair > > thanks > -- PMM
On Tue, 17 Sep 2019 09:37:47 PDT (-0700), alistair23@gmail.com wrote: > On Tue, Sep 17, 2019 at 6:56 AM Peter Maydell <peter.maydell@linaro.org> wrote: >> >> On Fri, 16 Aug 2019 at 09:57, Peter Maydell <peter.maydell@linaro.org> wrote: >> > >> > On Thu, 15 Aug 2019 at 23:17, Palmer Dabbelt <palmer@sifive.com> wrote: >> > > You're more than welcome to take them over. I've got something that boots >> > > Linux on my unassigned_access branch (github.com/palmer-dabbelt/qemu), but I >> > > haven't sanitized the whole port for physical accesses and I haven't convinced >> > > myself that my hook implementation is correct. >> > >> > Rather than doing >> > if (retaddr) { >> > cpu_restore_state(cs, retaddr, true); >> > } >> > >> > at the start of the hook I think you just want to pass 'retaddr' >> > as the final argument to riscv_raise_exception() instead of >> > using GETPC(). Other than that I think the hook itself is right. >> > >> > The 'git grep' regexes in docs/devel/loads-stores.rst are handy >> > for finding the places where the target code is doing physical >> > accesses. IIRC the only ones I found with a quick scan were the >> > PTE loads in get_physical_address() via ldl_phys/ldq_phys, which will >> > now return 0 and run into the 'invalid PTE' code path. I don't >> > know whether your architecture requires some different behaviour >> > for bus errors on page table walk than that (you might want to >> > specifically code the error path anyway or comment it even if the >> > behaviour is right, to be a bit more explicit that it can happen). >> >> Gentle ping -- would somebody like to have a go at the riscv >> do_transaction_failed hook conversion? I think it should be >> straightforward, and riscv is now the only architecture still >> using the old unassigned_access hook and preventing us from >> getting rid of it entirely. > > Thanks for the ping Peter, I forgot about this. > > @Palmer I have taken your patches, made some changes based on Peter's > comments and rebased them on your PR branch. > > I'll double check, but the hook implementation looks correct and I > can't see any other obvious unsanitised physical accesses so it should > be ok. I'll send them out today if I don't find any issues. Thanks! > > Alistair > >> >> thanks >> -- PMM
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index 0632ac08cf35..5b9fae608cca 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -482,6 +482,7 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data) cc->gdb_stop_before_watchpoint = true; cc->disas_set_info = riscv_cpu_disas_set_info; #ifndef CONFIG_USER_ONLY + cc->do_unassigned_access = riscv_cpu_unassigned_access; cc->do_unaligned_access = riscv_cpu_do_unaligned_access; cc->get_phys_page_debug = riscv_cpu_get_phys_page_debug; #endif diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index b47cde501766..2e743312536b 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -259,6 +259,8 @@ void riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr, bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size, MMUAccessType access_type, int mmu_idx, bool probe, uintptr_t retaddr); +void riscv_cpu_unassigned_access(CPUState *cpu, hwaddr addr, bool is_write, + bool is_exec, int unused, unsigned size); char *riscv_isa_string(RISCVCPU *cpu); void riscv_cpu_list(void); diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index 8b6754b91798..0bbfb7f48b79 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -375,6 +375,22 @@ hwaddr riscv_cpu_get_phys_page_debug(CPUState *cs, vaddr addr) return phys_addr; } +void riscv_cpu_unassigned_access(CPUState *cs, hwaddr addr, bool is_write, + bool is_exec, int unused, unsigned size) +{ + RISCVCPU *cpu = RISCV_CPU(cs); + CPURISCVState *env = &cpu->env; + + if (is_write) { + cs->exception_index = RISCV_EXCP_STORE_AMO_ACCESS_FAULT; + } else { + cs->exception_index = RISCV_EXCP_LOAD_ACCESS_FAULT; + } + + env->badaddr = addr; + riscv_raise_exception(&cpu->env, cs->exception_index, GETPC()); +} + void riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr, MMUAccessType access_type, int mmu_idx, uintptr_t retaddr)