Re: [PATCH v2 2/2] target/riscv: Support xtheadmaee for thead-c906

[Alistair Francis]

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