Date:   Tue, 28 Sep 2021 11:45:25 +0200
From:   Philipp Tomsich <philipp.tomsich@...ll.eu>
To:     Atish Patra <atishp@...shpatra.org>
Cc:     Greg Favor <gfavor@...tanamicro.com>, Guo Ren <guoren@...nel.org>,
        Palmer Dabbelt <palmer@...belt.com>,
        Anup Patel <anup.patel@....com>,
        Atish Patra <atish.patra@....com>,
        Palmer Dabbelt <palmerdabbelt@...gle.com>,
        Christoph Müllner <christoph.muellner@...ll.eu>,
        Christoph Hellwig <hch@....de>,
        liush <liush@...winnertech.com>, wefu@...hat.com,
        Wei Wu (吴伟) <lazyparser@...il.com>,
        Drew Fustini <drew@...gleboard.org>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org List" <linux-kernel@...r.kernel.org>,
        taiten.peng@...onical.com, aniket.ponkshe@...onical.com,
        Heinrich Schuchardt <heinrich.schuchardt@...onical.com>,
        gordan.markus@...onical.com, Guo Ren <guoren@...ux.alibaba.com>,
        Arnd Bergmann <arnd@...db.de>, Chen-Yu Tsai <wens@...e.org>,
        Maxime Ripard <maxime@...no.tech>,
        Daniel Lustig <dlustig@...dia.com>,
        Andrea Mondelli <andrea.mondelli@...wei.com>,
        Jonathan Behrens <behrensj@....edu>,
        Xinhaoqu <xinhaoqu@...wei.com>,
        Bill Huffman <huffman@...ence.com>,
        Nick Kossifidis <mick@....forth.gr>,
        Allen Baum <allen.baum@...erantotech.com>,
        Josh Scheid <jscheid@...tanamicro.com>,
        Richard Trauben <rtrauben@...il.com>
Subject: Re: [PATCH V2 1/2] riscv: Add RISC-V svpbmt extension

On Tue, 28 Sept 2021 at 01:05, Atish Patra <atishp@...shpatra.org> wrote:
>
> On Mon, Sep 27, 2021 at 1:53 PM Greg Favor <gfavor@...tanamicro.com> wrote:
> >
> > With the big caveat that I haven't been in the middle of this discussion, it seems like Allwinner D1's changes represent a custom (and nonconforming) extension.
>
> As per the v1.12 privilege specification, bit 63 is reserved for
> Svnapot extension while bit 60–54 are reserved for future standard
> use.
> D1's implementation uses both 60 and 63 bit for their custom "PBMT"
> extension in addition to bit 61 & 62 [1].
>
> > Isn't this just a matter of the patch needing to be treated as for a RISC-V custom extension per the recently clarified policy for handling upstreaming/etc. of custom extensions?  (Philipp can > speak to this clarified policy.)  Or what am I missing?
>
> Linux kernel upstream policy is yet to adopt that clarification as it
> was recently discussed at RVI meetings. Is there a written definition
> of non-conforming/custom/incompatible ?
> Moreover, as per the platform specification[2],
>
> A non-conforming extension that conflicts with a supported standard
> extensions must satisfy at least one of the following:
>   --- It must be disabled by default.
>   --- The supported standard extension must be declared as unsupported
> in all feature discovery structures used by software.
>       This option is allowed only if the standard extension is not required.

The terminology for those cases (and x-thead-v is the mental
test-case) would be "non-compliant and non-conflicting".
The "non-compliant" part comes from the fact that the specification
has been violated: in the case of x-thead-v by using the reserved
opcode space, in the case of the PTEs by using reserved bits.
The "non-conflicting" is based on a system still being able to operate
according to the specification, even if the non-compliant extension is
simply ignored (e.g. RVV is not mandatory and the opcodes for RVV are
not required to raise illegal insn exceptions, so these systems would
simply appear as not having RVV)…

Now, with their "custom" PBMT, we are pushing the boundaries of the
'non-conflicting' definition but are still within the same reasoning:
if we only signal sv39 and no PBMT-support, then the abuse of the PTE
bits does not conflict.  In the end, the 'non-conflicting' status will
hinge on whether we make svpbmt mandatory in the Platform (or the
referenced Profile).

In this particular case — given the importance of the D1 boards for
bootstrapping the software ecosystem — I would make the case that we
need to provide a provision in the Platforms (i.e. OS-A base) to
retain the 'non-conflicting' status (e.g. by mandating "some pbmt" —
with an application note stating that this will be restricted to
svpbmt in the next major revision of the Platforms ... and already
restricting it to svpbmt for the OS-A server extension).

> In this case, the custom non-conforming implementation can not be
> disabled or marked unsupported as it is critical for all the necessary
> I/O devices (usb, mmc, ethernet).
> Without this custom implementation support in upstream, we can not
> really use the mainline kernel for Allwinner D1.

I don't agree from a specification standpoint and from the
'non-conflicting' standpoint: whether or not device drivers can be
used, does not change the 'non-conflicting' status.
This is similar to the 'non-conflicting' status with x-thead-v: vector
instructions (as in "the standard RVV instructions") also can't be
used with a toolchain/libraries/OS that only implements RVV ... but
nonetheless, vendor-specific vector instructions are available.

I would argue the same for Alibaba's PBMT: I/O devices will not work,
unless a vendor-specific non-conflicting PBMT is used.

The brings us back to the requirements that I defined multiple times
for this: the implementation needs to be properly modularized and
quarantined as to not adversely impact compliant implementations.
If this can be assured, I will always argue for inclusion based on the
benefit to the ecosystem and reconciling the imminent fragmentation.
Or in other words: In what world does it make sense to encourage a
vendor of a board with significant market share to create a
vendor-fork of the kernel, if that vendor is willing to work with the
upstream?  Note that I normally am the first to argue on principle,
but have come to the conclusion that we are really between a rock and
a hard place on this one — and my priority is to keep the ecosystem
from fragmenting.

>> >> > We need to decide whether we should support the upstream kernel for D1. Few things to consider.
> >> > – Can it be considered as an errata ?
> >> > – Does it set a bad precedent and open can of worms in future ?
> >> > – Can we just ignore D1 given the mass volume ?
> >> >
> >> > One solution I can think of is that we allow this as an exception to the patch acceptance policy.
> >> > We need to explicitly specify this board as an exception because the policy was not in place during the design phase of the hardware.
> >> > At least, it protects us from accepting the incompatible changes in the future. Any other ideas ?

Anything we do needs to consider future impact and precedent.  But
that should not preclude us from making an exception, when it benefits
the ecosystem by preventing further fragmentation.
I consider the stated requirements of proper modularization
(quarantining the impact within the code base) and a precondition on
the vendor maintaining the changes, as a strong-enough discouragement
for any parties that might also consider to apply gross negligence to
the meaning of 'reserved'.

Philipp.

Powered by blists - more mailing lists