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

["Christoph Müllner" <christoph.muellner@vrull.eu>]

On Thu, Mar 28, 2024 at 2:19 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> 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
>
> A DT node that we don't set also works fine for us

I would really like to do that, but given the page table is set up so
early in the boot process
probing via CSR is much easier to realize in the kernel than parsing the DT.
Therefore, I think th.sxstatus emulation + probing is the best way to
move forward.

Thanks,
Christoph