[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/2] target/riscv: Support xtheadmaee for thead-c906
From: |
Alistair Francis |
Subject: |
Re: [PATCH v2 2/2] target/riscv: Support xtheadmaee for thead-c906 |
Date: |
Thu, 28 Mar 2024 11:18:53 +1000 |
On Wed, Mar 27, 2024 at 9:19 PM Conor Dooley <conor@kernel.org> wrote:
>
> Christoph linked here on his submission to Linux of a fix for this, so I
> am reviving this to leave a couple comments :)
>
> On Thu, Feb 15, 2024 at 02:24:02PM +1000, Alistair Francis wrote:
> > On Mon, Feb 5, 2024 at 6:37 PM Christoph Müllner
> > <christoph.muellner@vrull.eu> wrote:
> > > On Mon, Feb 5, 2024 at 3:42 AM Alistair Francis <alistair23@gmail.com>
> > > wrote:
> > > > On Sun, Feb 4, 2024 at 3:44 PM LIU Zhiwei
> > > > <zhiwei_liu@linux.alibaba.com> wrote:
>
> > > > > ppn = (pte & (target_ulong)PTE_PPN_MASK) >>
> > > > > PTE_PPN_SHIFT;
> > > >
> > > > Unfortunately we won't be able to take this upstream. This is core
> > > > QEMU RISC-V code that is now being changed against the spec. I think
> > > > adding the CSR is fine, but we can't take this core change.
> > > >
> > > > A fix that works for everyone should be supporting the th_mxstatus
> > > > CSR, but don't support setting the TH_MXSTATUS_MAEE bit. That way
> > > > guests can detect that the bit isn't set and not use the reserved bits
> > > > in the PTE. From my understanding the extra PTE bits are related to
> > > > cache control in the hardware, which we don't need here
> > >
> > > Sounds good! Let me recap the overall plan:
> > > * QEMU does not emulate MAEE, but signals that MAEE is not available
> > > by setting TH_MXSTATUS_MAEE to 0.
> >
> > Yep!
> >
> > > * Consequence: The c906 emulation does not enable any page-base memory
> > > attribute mechanism.
> >
> > Exactly
> >
> > > * OpenSBI tests the TH_MXSTATUS_MAEE bit (M-mode only) and provides
> > > that information to user-space (e.g. DTB).
> >
> > To the kernel, but yep!
> >
> > > * The current Linux errata code will be enhanced to not assume MAEE
> > > for each core with T-Head vendor ID, but also query the MAEE bit and
> > > ensure it is set.
> >
> > I feel like it should already do that :)
>
> It doesn't quite do this right now. It only makes the assumption for
> CPUs where marchid and mvendorid are zero. The c908, and I think Guo Ren
> confirmed it will be the case going forward, sets these to non-zero
> values. We should have always required a dt property be set, rather than
> using m*id, but we can't go back on that for these devices. Going
> forward, if there are more CPUs that want to use this e.g. C908 in MAEE
> mode (it can do svpbmt too) I'm gonna require it is explicitly set in
> DT.
A DT node that we don't set also works fine for us
Alistair