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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3666 bytes --]

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.

> > * Booting on a QEMU c906 will not enable MAEE, but will still boot.
> 
> That's the hope
> 
> >
> > We never had a complete MAEE implementation upstream, because that's not needed.
> > But until recently we could mess with reserved bits how we want.
> > Now that QEMU is more restrictive about reserved bits in the PTEs, we
> > need to address this in the kernel.
> >
> > The downside is, that the kernel now sees a c906 CPU without MAEE and
> > such a CPU does not exist.
> 
> Yeah, that is the downside. But in reality a CPU could exist that
> doesn't allow seeing MAEE, so I don't think it's that insane.

Aye, and the case of a new CPU disabling it but not setting m*id will be
a pain.

> > But that's fine, because this does not require extra code to handle this case.
> > Also, the additional check for the MAEE bit in the kernel errata
> > probing code is likely needed anyway for future T-Head CPUs.
> 
> Exactly

I don't really want to have extension detection side channels like this
in Linux if it can be at all avoided, I'd rather get this information from
DT or ACPI. The marchid == mvendorid == 0x0 case has a pretty high chance
of needing some special handling in the future though, so something
targeting those cases when there's some demonstrable problem seems fair
to me.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]