Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i84414492:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-499-gf27bbf33e2-fm-20230619.001-gf27bbf33
Mime-Version: 1.0
Message-Id: <8f1ba16d-42e3-4e45-9fac-a91b3b0556a8@app.fastmail.com>
In-Reply-To: <1788842.TLkxdtWsSY@diego>
References: <20230228215435.3366914-1-heiko@sntech.de>
 <c01ee919-9abb-4796-9ed4-9bb4565dc863@app.fastmail.com>
 <1961474.usQuhbGJ8B@diego> <1788842.TLkxdtWsSY@diego>
Date:   Fri, 23 Jun 2023 23:23:11 -0400
From:   "Stefan O'Rear" <sorear@fastmail.com>
To:     "Heiko Stuebner" <heiko@sntech.de>,
        "Palmer Dabbelt" <palmer@dabbelt.com>
Cc:     linux-riscv@lists.infradead.org, samuel@sholland.org,
        guoren@kernel.org, christoph.muellner@vrull.eu,
        conor.dooley@microchip.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC 2/2] RISC-V: add T-Head vector errata handling
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Jun 23, 2023, at 7:26 PM, Heiko St=C3=BCbner wrote:
> Am Freitag, 23. Juni 2023, 12:22:35 CEST schrieb Heiko St=C3=BCbner:
>> Am Freitag, 23. Juni 2023, 05:06:44 CEST schrieb Stefan O'Rear:
>> > On Thu, Jun 22, 2023, at 4:35 PM, Heiko St=C3=BCbner wrote:
>> > > Am Donnerstag, 22. Juni 2023, 20:58:37 CEST schrieb Stefan O'Rear:
>> > >> Are you aware of "3.7. Vector Fixed-Point Fields in fcsr" in
>> > >> riscv-v-spec-0.7.1.pdf?
>> > >
>> > > oh wow, thanks a lot for that pointer, now I understand your conc=
ern.
>> > >
>> > > So in vector-0.7.1 fcsr[10:9] mirrors vxrm and fcsr[8] mirrors vx=
sat.
>> > >
>> > >
>> > > On a positive note, the T-Head cores seem to not implement the fu=
ll
>> > > vector 0.7.1 specification after all, in the documentation I have=
 [0]
>> > > fcsr[31:8] are declared as "0" and uppermost bits are [7:5] for t=
he "frm"
>> > > field.
>> >=20
>> > Given that the pdf you linked does not mention any vector CSRs, I a=
m not
>> > confident that it provides a complete and accurate description of v=
ector
>> > functionality in other registers for the C906 with vector extension.
>> >=20
>> > Assuming that you have access to such a chip, I would be much happi=
er with
>> > the proposed "just a comment" approach if our understanding of the =
behavior
>> > were confirmed on hardware (specifically: csr_write(CSR_FCSR, 0x700=
) should
>> > not affect csr_read(CSR_VXRM) or csr_read(CSR_VXSAT)).
>>=20
>> For one, you're right that I should definitly try to confirm this on =
hardware :-) .
>
> ok, so now I know the documentation is wrong.
>
> 	before, vxrm 0x0, vxsat 0x0
> writing the 0x700 to fcsr
> 	after, vxrm 0x3, vxsat 0x1
>
> Essentially the link between the CSRs really is there - oh fun.
> So we're back at your original concern - sadly.
>
> I guess I need to figure out how to not have this stuff break
> because relying on the fpu parts to handle feels not correct
> at first glance.

I don't see a conceptual problem here.

There are a large number of vector instructions that access the floating=
 point
state (frm, fflags, f{0..31}).  These instructions require a valid float=
ing
point context, sstatus.FS>0, trap if sstatus.FS=3D0, and set sstatus.FS=3D=
3 if they
change anything.  vfadd, vfsub, vfmul, vfdiv, vfmv, etc, etc in both 0.7=
.1 and 1.0.

0.7.1 extends the floating point state to include vxrm and vxsat and add=
s vaaddu,
vnclip, vsmul, etc to the list of instructions which access both floatin=
g point and
vector state.  This breaks floating-point emulation (if a core provides =
integer
vectors in hardware, it provides a fcsr register with three writeable bi=
ts, which
means that accesses to fcsr won't trap and can't be emulated to provide =
access to
the software frm and fflags), which is probably why the behavior was cha=
nged in 1.0,
but is otherwise internally consistent.

You can continue to treat "fpu parts" and "vector parts" as independent =
as long as
you recognize vxrm and vxsat as _exclusively_ owned by the "fpu parts".

Access to vxrm and vxsat should be exclusively controlled by sstatus.FS =
since they
are aliases for fields in fcsr, while access to vstart/vtype/vlen should=
 be
exclusively controlled by sstatus.VS.  It would be good to test this, si=
nce it's not
actually in the spec (risc-v has a rather systematic underspecification =
problem),
and T-Head's idea of obviously correct behavior may differ from mine.

1.0 puts vxrm and vxsat into the vector state, controlled by sstatus.VS;=
 vsmul now
works on the vector state only, so as far as state management is concern=
ed it now
acts like vadd and not like vfadd.  This is also internally consistent, =
but vxcsr
must be owned by whoever manages sstatus.VS.

It's a little bit weird to say "vxrm and vxsat live in different parts o=
f the kernel
state depending on V extension version" but it might be less weird to sa=
y "fcsr and
all its parts lives somewhere, vxcsr and all its parts lives somewhere e=
lse,
vxcsr doesn't exist in 0.7.1".  Is this adequate?

-s

>
> Heiko