Re: [Qemu-devel] [PULL 04/32] target/riscv: Implement riscv_cpu_unassigned_access
On Tue, 17 Sep 2019 09:37:47 PDT (-0700), alistai...@gmail.com wrote:
On Tue, Sep 17, 2019 at 6:56 AM Peter Maydell wrote:
On Fri, 16 Aug 2019 at 09:57, Peter Maydell wrote:
>
> On Thu, 15 Aug 2019 at 23:17, Palmer Dabbelt 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
Re: [Qemu-devel] [PULL 04/32] target/riscv: Implement riscv_cpu_unassigned_access
On Tue, Sep 17, 2019 at 6:56 AM Peter Maydell wrote:
>
> On Fri, 16 Aug 2019 at 09:57, Peter Maydell wrote:
> >
> > On Thu, 15 Aug 2019 at 23:17, Palmer Dabbelt 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
Re: [Qemu-devel] [PULL 04/32] target/riscv: Implement riscv_cpu_unassigned_access
On Fri, 16 Aug 2019 at 09:57, Peter Maydell wrote:
>
> On Thu, 15 Aug 2019 at 23:17, Palmer Dabbelt 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
Re: [Qemu-devel] [PULL 04/32] target/riscv: Implement riscv_cpu_unassigned_access
On Thu, 15 Aug 2019 at 23:17, Palmer Dabbelt 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
Re: [Qemu-devel] [PULL 04/32] target/riscv: Implement riscv_cpu_unassigned_access
On Thu, 15 Aug 2019 14:39:18 PDT (-0700), alistai...@gmail.com wrote: On Tue, Aug 13, 2019 at 3:44 PM Palmer Dabbelt wrote: On Thu, 01 Aug 2019 08:39:17 PDT (-0700), Peter Maydell wrote: > On Wed, 3 Jul 2019 at 09:41, Palmer Dabbelt wrote: >> >> From: Michael Clark >> >> 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 >> [Changes by AF: >> - Squash two patches and rewrite commit message >> - Set baddr to the access address >> ] >> Signed-off-by: Alistair Francis >> Reviewed-by: Palmer Dabbelt >> Signed-off-by: Palmer Dabbelt > > 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.
Re: [Qemu-devel] [PULL 04/32] target/riscv: Implement riscv_cpu_unassigned_access
On Tue, Aug 13, 2019 at 3:44 PM Palmer Dabbelt wrote: > > On Thu, 01 Aug 2019 08:39:17 PDT (-0700), Peter Maydell wrote: > > On Wed, 3 Jul 2019 at 09:41, Palmer Dabbelt wrote: > >> > >> From: Michael Clark > >> > >> 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 > >> [Changes by AF: > >> - Squash two patches and rewrite commit message > >> - Set baddr to the access address > >> ] > >> Signed-off-by: Alistair Francis > >> Reviewed-by: Palmer Dabbelt > >> Signed-off-by: Palmer Dabbelt > > > > 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 >
Re: [Qemu-devel] [PULL 04/32] target/riscv: Implement riscv_cpu_unassigned_access
On Thu, 01 Aug 2019 08:39:17 PDT (-0700), Peter Maydell wrote: On Wed, 3 Jul 2019 at 09:41, Palmer Dabbelt wrote: From: Michael Clark 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 [Changes by AF: - Squash two patches and rewrite commit message - Set baddr to the access address ] Signed-off-by: Alistair Francis Reviewed-by: Palmer Dabbelt Signed-off-by: Palmer Dabbelt 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.
Re: [Qemu-devel] [PULL 04/32] target/riscv: Implement riscv_cpu_unassigned_access
On Wed, 3 Jul 2019 at 09:41, Palmer Dabbelt wrote: > > From: Michael Clark > > 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 > [Changes by AF: > - Squash two patches and rewrite commit message > - Set baddr to the access address > ] > Signed-off-by: Alistair Francis > Reviewed-by: Palmer Dabbelt > Signed-off-by: Palmer Dabbelt 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
[Qemu-devel] [PULL 04/32] target/riscv: Implement riscv_cpu_unassigned_access
From: Michael Clark
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
[Changes by AF:
- Squash two patches and rewrite commit message
- Set baddr to the access address
]
Signed-off-by: Alistair Francis
Reviewed-by: Palmer Dabbelt
Signed-off-by: Palmer Dabbelt
---
target/riscv/cpu.c| 1 +
target/riscv/cpu.h| 2 ++
target/riscv/cpu_helper.c | 16
3 files changed, 19 insertions(+)
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)
--
2.21.0
