Re: [Qemu-devel] [PULL 04/32] target/riscv: Implement riscv_cpu_unassigned_access

From: Alistair Francis <alistair23@gmail.com>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: "open list:RISC-V" <qemu-riscv@nongnu.org>,
	Palmer Dabbelt <palmer@sifive.com>,
	Alistair Francis <Alistair.Francis@wdc.com>,
	QEMU Developers <qemu-devel@nongnu.org>
Subject: Re: [Qemu-devel] [PULL 04/32] target/riscv: Implement riscv_cpu_unassigned_access
Date: Tue, 17 Sep 2019 09:37:47 -0700	[thread overview]
Message-ID: <CAKmqyKPPqzo+dKqXoqmBpuaHp3hbdzA=mpgqECMLGX+whn2pcw@mail.gmail.com> (raw)
In-Reply-To: <CAFEAcA-r5nr3xF6A5C6mdt2rakB8TXPSp+s_g6sd-aUy566LrQ@mail.gmail.com>

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