[v5,2/4] dt-bindings: riscv: Add Svade and Svadu Entries

Message ID	20240605121512.32083-3-yongxuan.wang@sifive.com (mailing list archive)
State	New, archived
Headers	show Received: from mail-pf1-f182.google.com (mail-pf1-f182.google.com [209.85.210.182]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C3A5A1B14FE for <kvm@vger.kernel.org>; Wed, 5 Jun 2024 12:15:46 +0000 (UTC) From: Yong-Xuan Wang <yongxuan.wang@sifive.com> To: linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org, kvm-riscv@lists.infradead.org, kvm@vger.kernel.org Cc: apatel@ventanamicro.com, alex@ghiti.fr, ajones@ventanamicro.com, greentime.hu@sifive.com, vincent.chen@sifive.com, Yong-Xuan Wang <yongxuan.wang@sifive.com>, Conor Dooley <conor@kernel.org>, Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>, Paul Walmsley <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, Albert Ou <aou@eecs.berkeley.edu>, devicetree@vger.kernel.org Subject: [PATCH v5 2/4] dt-bindings: riscv: Add Svade and Svadu Entries Date: Wed, 5 Jun 2024 20:15:08 +0800 Message-Id: <20240605121512.32083-3-yongxuan.wang@sifive.com> In-Reply-To: <20240605121512.32083-1-yongxuan.wang@sifive.com> References: <20240605121512.32083-1-yongxuan.wang@sifive.com> Precedence: bulk
Series	Add Svade and Svadu Extensions Support \| expand [v5,0/4] Add Svade and Svadu Extensions Support [v5,1/4] RISC-V: Add Svade and Svadu Extensions Support [v5,2/4] dt-bindings: riscv: Add Svade and Svadu Entries [v5,3/4] RISC-V: KVM: Add Svade and Svadu Extensions Support for Guest/VM [v5,4/4] KVM: riscv: selftests: Add Svade and Svadu Extension to get-reg-list test

Yong-Xuan Wang June 5, 2024, 12:15 p.m. UTC

Add entries for the Svade and Svadu extensions to the riscv,isa-extensions
property.

Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
---
 .../devicetree/bindings/riscv/extensions.yaml | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)

Conor Dooley June 5, 2024, 4:54 p.m. UTC | #1

On Wed, Jun 05, 2024 at 08:15:08PM +0800, Yong-Xuan Wang wrote:
> Add entries for the Svade and Svadu extensions to the riscv,isa-extensions
> property.
> 
> Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> ---
>  .../devicetree/bindings/riscv/extensions.yaml | 30 +++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> index 468c646247aa..1e30988826b9 100644
> --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> @@ -153,6 +153,36 @@ properties:
>              ratified at commit 3f9ed34 ("Add ability to manually trigger
>              workflow. (#2)") of riscv-time-compare.
>  
> +        - const: svade
> +          description: |
> +            The standard Svade supervisor-level extension for raising page-fault
> +            exceptions when PTE A/D bits need be set as ratified in the 20240213
> +            version of the privileged ISA specification.
> +
> +            Both Svade and Svadu extensions control the hardware behavior when
> +            the PTE A/D bits need to be set. The default behavior for the four
> +            possible combinations of these extensions in the device tree are:
> +            1. Neither svade nor svadu in DT: default to svade.

I think this needs to be expanded on, as to why nothing means svade.

> +            2. Only svade in DT: use svade.

That's a statement of the obvious, right?

> +            3. Only svadu in DT: use svadu.

This is not relevant for Svade.

> +            4. Both svade and svadu in DT: default to svade (Linux can switch to
> +               svadu once the SBI FWFT extension is available).

"The privilege level to which this devicetree has been provided can switch to
Svadu if the SBI FWFT extension is available".

> +        - const: svadu
> +          description: |
> +            The standard Svadu supervisor-level extension for hardware updating
> +            of PTE A/D bits as ratified at commit c1abccf ("Merge pull request
> +            #25 from ved-rivos/ratified") of riscv-svadu.
> +
> +            Both Svade and Svadu extensions control the hardware behavior when
> +            the PTE A/D bits need to be set. The default behavior for the four
> +            possible combinations of these extensions in the device tree are:

@Anup/Drew/Alex, are we missing some wording in here about it only being
valid to have Svadu in isolation if the provider of the devicetree has
actually turned on Svadu? The binding says "the default behaviour", but
it is not the "default" behaviour, the behaviour is a must AFAICT. If
you set Svadu in isolation, you /must/ have turned it on. If you set
Svadu and Svade, you must have Svadu turned off?

> +            1. Neither svade nor svadu in DT: default to svade.
> +            2. Only svade in DT: use svade.

These two are not relevant to Svadu, I'd leave them out.

> +            3. Only svadu in DT: use svadu.

Again, statement of the obvious?

> +            4. Both svade and svadu in DT: default to svade (Linux can switch to
> +               svadu once the SBI FWFT extension is available).

Same here as in the Svade entry.

Thanks,
Conor.

Yong-Xuan Wang June 18, 2024, 10:38 a.m. UTC | #2

On Thu, Jun 6, 2024 at 12:55 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, Jun 05, 2024 at 08:15:08PM +0800, Yong-Xuan Wang wrote:
> > Add entries for the Svade and Svadu extensions to the riscv,isa-extensions
> > property.
> >
> > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> > ---
> >  .../devicetree/bindings/riscv/extensions.yaml | 30 +++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > index 468c646247aa..1e30988826b9 100644
> > --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> > +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > @@ -153,6 +153,36 @@ properties:
> >              ratified at commit 3f9ed34 ("Add ability to manually trigger
> >              workflow. (#2)") of riscv-time-compare.
> >
> > +        - const: svade
> > +          description: |
> > +            The standard Svade supervisor-level extension for raising page-fault
> > +            exceptions when PTE A/D bits need be set as ratified in the 20240213
> > +            version of the privileged ISA specification.
> > +
> > +            Both Svade and Svadu extensions control the hardware behavior when
> > +            the PTE A/D bits need to be set. The default behavior for the four
> > +            possible combinations of these extensions in the device tree are:
> > +            1. Neither svade nor svadu in DT: default to svade.
>
> I think this needs to be expanded on, as to why nothing means svade.
>
> > +            2. Only svade in DT: use svade.
>
> That's a statement of the obvious, right?
>
> > +            3. Only svadu in DT: use svadu.
>
> This is not relevant for Svade.
>
> > +            4. Both svade and svadu in DT: default to svade (Linux can switch to
> > +               svadu once the SBI FWFT extension is available).
>
> "The privilege level to which this devicetree has been provided can switch to
> Svadu if the SBI FWFT extension is available".
>
> > +        - const: svadu
> > +          description: |
> > +            The standard Svadu supervisor-level extension for hardware updating
> > +            of PTE A/D bits as ratified at commit c1abccf ("Merge pull request
> > +            #25 from ved-rivos/ratified") of riscv-svadu.
> > +
> > +            Both Svade and Svadu extensions control the hardware behavior when
> > +            the PTE A/D bits need to be set. The default behavior for the four
> > +            possible combinations of these extensions in the device tree are:
>
> @Anup/Drew/Alex, are we missing some wording in here about it only being
> valid to have Svadu in isolation if the provider of the devicetree has
> actually turned on Svadu? The binding says "the default behaviour", but
> it is not the "default" behaviour, the behaviour is a must AFAICT. If
> you set Svadu in isolation, you /must/ have turned it on. If you set
> Svadu and Svade, you must have Svadu turned off?
>
> > +            1. Neither svade nor svadu in DT: default to svade.
> > +            2. Only svade in DT: use svade.
>
> These two are not relevant to Svadu, I'd leave them out.
>
> > +            3. Only svadu in DT: use svadu.
>
> Again, statement of the obvious?
>
> > +            4. Both svade and svadu in DT: default to svade (Linux can switch to
> > +               svadu once the SBI FWFT extension is available).
>
> Same here as in the Svade entry.
>
> Thanks,
> Conor.
>

Hi Conor,

I will update the description. Thank you!

Regards,
Yong-Xuan

Conor Dooley June 19, 2024, 6:11 p.m. UTC | #3

Anup, Drew, Alex,

On Tue, Jun 18, 2024 at 06:38:13PM +0800, Yong-Xuan Wang wrote:
> On Thu, Jun 6, 2024 at 12:55 AM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Wed, Jun 05, 2024 at 08:15:08PM +0800, Yong-Xuan Wang wrote:
> > > Add entries for the Svade and Svadu extensions to the riscv,isa-extensions
> > > property.
> > >
> > > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> > > ---
> > >  .../devicetree/bindings/riscv/extensions.yaml | 30 +++++++++++++++++++
> > >  1 file changed, 30 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > index 468c646247aa..1e30988826b9 100644
> > > --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > @@ -153,6 +153,36 @@ properties:
> > >              ratified at commit 3f9ed34 ("Add ability to manually trigger
> > >              workflow. (#2)") of riscv-time-compare.
> > >
> > > +        - const: svade
> > > +          description: |
> > > +            The standard Svade supervisor-level extension for raising page-fault
> > > +            exceptions when PTE A/D bits need be set as ratified in the 20240213
> > > +            version of the privileged ISA specification.
> > > +
> > > +            Both Svade and Svadu extensions control the hardware behavior when
> > > +            the PTE A/D bits need to be set. The default behavior for the four
> > > +            possible combinations of these extensions in the device tree are:
> > > +            1. Neither svade nor svadu in DT: default to svade.
> >
> > I think this needs to be expanded on, as to why nothing means svade.
> >
> > > +            2. Only svade in DT: use svade.
> >
> > That's a statement of the obvious, right?
> >
> > > +            3. Only svadu in DT: use svadu.
> >
> > This is not relevant for Svade.
> >
> > > +            4. Both svade and svadu in DT: default to svade (Linux can switch to
> > > +               svadu once the SBI FWFT extension is available).
> >
> > "The privilege level to which this devicetree has been provided can switch to
> > Svadu if the SBI FWFT extension is available".
> >
> > > +        - const: svadu
> > > +          description: |
> > > +            The standard Svadu supervisor-level extension for hardware updating
> > > +            of PTE A/D bits as ratified at commit c1abccf ("Merge pull request
> > > +            #25 from ved-rivos/ratified") of riscv-svadu.
> > > +
> > > +            Both Svade and Svadu extensions control the hardware behavior when
> > > +            the PTE A/D bits need to be set. The default behavior for the four
> > > +            possible combinations of these extensions in the device tree are:
> >
> > @Anup/Drew/Alex, are we missing some wording in here about it only being
> > valid to have Svadu in isolation if the provider of the devicetree has
> > actually turned on Svadu? The binding says "the default behaviour", but
> > it is not the "default" behaviour, the behaviour is a must AFAICT. If
> > you set Svadu in isolation, you /must/ have turned it on. If you set
> > Svadu and Svade, you must have Svadu turned off?
> >
> > > +            1. Neither svade nor svadu in DT: default to svade.
> > > +            2. Only svade in DT: use svade.
> >
> > These two are not relevant to Svadu, I'd leave them out.
> >
> > > +            3. Only svadu in DT: use svadu.
> >
> > Again, statement of the obvious?
> >
> > > +            4. Both svade and svadu in DT: default to svade (Linux can switch to
> > > +               svadu once the SBI FWFT extension is available).
> >
> > Same here as in the Svade entry.

> I will update the description. Thank you!

Before you do, I'd love if Anup, Drew or Alex could comment on my
question about default behaviours. They're the ones with Strong Opinions
here about how the SBI implementation should behave, and I want to make
sure it is correctly documented.

Thanks,
Conor.

Anup Patel June 20, 2024, 6:25 a.m. UTC | #4

On Wed, Jun 5, 2024 at 10:25 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, Jun 05, 2024 at 08:15:08PM +0800, Yong-Xuan Wang wrote:
> > Add entries for the Svade and Svadu extensions to the riscv,isa-extensions
> > property.
> >
> > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> > ---
> >  .../devicetree/bindings/riscv/extensions.yaml | 30 +++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > index 468c646247aa..1e30988826b9 100644
> > --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> > +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > @@ -153,6 +153,36 @@ properties:
> >              ratified at commit 3f9ed34 ("Add ability to manually trigger
> >              workflow. (#2)") of riscv-time-compare.
> >
> > +        - const: svade
> > +          description: |
> > +            The standard Svade supervisor-level extension for raising page-fault
> > +            exceptions when PTE A/D bits need be set as ratified in the 20240213
> > +            version of the privileged ISA specification.
> > +
> > +            Both Svade and Svadu extensions control the hardware behavior when
> > +            the PTE A/D bits need to be set. The default behavior for the four
> > +            possible combinations of these extensions in the device tree are:
> > +            1. Neither svade nor svadu in DT: default to svade.
>
> I think this needs to be expanded on, as to why nothing means svade.

Actually if both Svade and Svadu are not present in DT then
it is left to the platform and OpenSBI does nothing.

>
> > +            2. Only svade in DT: use svade.
>
> That's a statement of the obvious, right?
>
> > +            3. Only svadu in DT: use svadu.
>
> This is not relevant for Svade.
>
> > +            4. Both svade and svadu in DT: default to svade (Linux can switch to
> > +               svadu once the SBI FWFT extension is available).
>
> "The privilege level to which this devicetree has been provided can switch to
> Svadu if the SBI FWFT extension is available".
>
> > +        - const: svadu
> > +          description: |
> > +            The standard Svadu supervisor-level extension for hardware updating
> > +            of PTE A/D bits as ratified at commit c1abccf ("Merge pull request
> > +            #25 from ved-rivos/ratified") of riscv-svadu.
> > +
> > +            Both Svade and Svadu extensions control the hardware behavior when
> > +            the PTE A/D bits need to be set. The default behavior for the four
> > +            possible combinations of these extensions in the device tree are:
>
> @Anup/Drew/Alex, are we missing some wording in here about it only being
> valid to have Svadu in isolation if the provider of the devicetree has
> actually turned on Svadu? The binding says "the default behaviour", but
> it is not the "default" behaviour, the behaviour is a must AFAICT. If
> you set Svadu in isolation, you /must/ have turned it on. If you set
> Svadu and Svade, you must have Svadu turned off?

Yes, the wording should be more of requirement style using
must or may.

How about this ?
1) Both Svade and Svadu not present in DT => Supervisor may
    assume Svade to be present and enabled or it can discover
    based on mvendorid, marchid, and mimpid.
2) Only Svade present in DT => Supervisor must assume Svade
    to be always enabled. (Obvious)
3) Only Svadu present in DT => Supervisor must assume Svadu
    to be always enabled. (Obvious)
4) Both Svade and Svadu present in DT => Supervisor must
    assume Svadu turned-off at boot time. To use Svadu, supervisor
    must explicitly enable it using the SBI FWFT extension.

IMO, the #2 and #3 are definitely obvious but still worth mentioning.

>
> > +            1. Neither svade nor svadu in DT: default to svade.
> > +            2. Only svade in DT: use svade.
>
> These two are not relevant to Svadu, I'd leave them out.
>
> > +            3. Only svadu in DT: use svadu.
>
> Again, statement of the obvious?
>
> > +            4. Both svade and svadu in DT: default to svade (Linux can switch to
> > +               svadu once the SBI FWFT extension is available).
>
> Same here as in the Svade entry.
>
> Thanks,
> Conor.
>

Regards,
Anup

Alexandre Ghiti June 21, 2024, 7:56 a.m. UTC | #5

On 05/06/2024 14:15, Yong-Xuan Wang wrote:
> Add entries for the Svade and Svadu extensions to the riscv,isa-extensions
> property.
>
> Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> ---
>   .../devicetree/bindings/riscv/extensions.yaml | 30 +++++++++++++++++++
>   1 file changed, 30 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> index 468c646247aa..1e30988826b9 100644
> --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> @@ -153,6 +153,36 @@ properties:
>               ratified at commit 3f9ed34 ("Add ability to manually trigger
>               workflow. (#2)") of riscv-time-compare.
>   
> +        - const: svade
> +          description: |
> +            The standard Svade supervisor-level extension for raising page-fault
> +            exceptions when PTE A/D bits need be set


Maybe something like:

"The standard Svade supervisor-level extension for SW-managed PTE A/D 
bit updates as ratified..."

would be better, WDYT?


> as ratified in the 20240213
> +            version of the privileged ISA specification.
> +
> +            Both Svade and Svadu extensions control the hardware behavior when
> +            the PTE A/D bits need to be set. The default behavior for the four
> +            possible combinations of these extensions in the device tree are:
> +            1. Neither svade nor svadu in DT: default to svade.
> +            2. Only svade in DT: use svade.
> +            3. Only svadu in DT: use svadu.
> +            4. Both svade and svadu in DT: default to svade (Linux can switch to
> +               svadu once the SBI FWFT extension is available).
> +
> +        - const: svadu
> +          description: |
> +            The standard Svadu supervisor-level extension for hardware updating
> +            of PTE A/D bits as ratified at commit c1abccf ("Merge pull request
> +            #25 from ved-rivos/ratified") of riscv-svadu.
> +
> +            Both Svade and Svadu extensions control the hardware behavior when
> +            the PTE A/D bits need to be set. The default behavior for the four
> +            possible combinations of these extensions in the device tree are:
> +            1. Neither svade nor svadu in DT: default to svade.
> +            2. Only svade in DT: use svade.
> +            3. Only svadu in DT: use svadu.
> +            4. Both svade and svadu in DT: default to svade (Linux can switch to
> +               svadu once the SBI FWFT extension is available).


I would not duplicate this text, but rather say something like "Please 
refer to Svade dt-binding description for more details.".


> +
>           - const: svinval
>             description:
>               The standard Svinval supervisor-level extension for fine-grained

Andrew Jones June 21, 2024, 8:33 a.m. UTC | #6

On Thu, Jun 20, 2024 at 11:55:44AM GMT, Anup Patel wrote:
> On Wed, Jun 5, 2024 at 10:25 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Wed, Jun 05, 2024 at 08:15:08PM +0800, Yong-Xuan Wang wrote:
> > > Add entries for the Svade and Svadu extensions to the riscv,isa-extensions
> > > property.
> > >
> > > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> > > ---
> > >  .../devicetree/bindings/riscv/extensions.yaml | 30 +++++++++++++++++++
> > >  1 file changed, 30 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > index 468c646247aa..1e30988826b9 100644
> > > --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > @@ -153,6 +153,36 @@ properties:
> > >              ratified at commit 3f9ed34 ("Add ability to manually trigger
> > >              workflow. (#2)") of riscv-time-compare.
> > >
> > > +        - const: svade
> > > +          description: |
> > > +            The standard Svade supervisor-level extension for raising page-fault
> > > +            exceptions when PTE A/D bits need be set as ratified in the 20240213
> > > +            version of the privileged ISA specification.
> > > +
> > > +            Both Svade and Svadu extensions control the hardware behavior when
> > > +            the PTE A/D bits need to be set. The default behavior for the four
> > > +            possible combinations of these extensions in the device tree are:
> > > +            1. Neither svade nor svadu in DT: default to svade.
> >
> > I think this needs to be expanded on, as to why nothing means svade.
> 
> Actually if both Svade and Svadu are not present in DT then
> it is left to the platform and OpenSBI does nothing.

This is a good point, and maybe it's worth integrating something that
states this case is technically unknown into the final text. (Even though
historically this has been assumed to mean svade.)

> 
> >
> > > +            2. Only svade in DT: use svade.
> >
> > That's a statement of the obvious, right?
> >
> > > +            3. Only svadu in DT: use svadu.
> >
> > This is not relevant for Svade.
> >
> > > +            4. Both svade and svadu in DT: default to svade (Linux can switch to
> > > +               svadu once the SBI FWFT extension is available).
> >
> > "The privilege level to which this devicetree has been provided can switch to
> > Svadu if the SBI FWFT extension is available".
> >
> > > +        - const: svadu
> > > +          description: |
> > > +            The standard Svadu supervisor-level extension for hardware updating
> > > +            of PTE A/D bits as ratified at commit c1abccf ("Merge pull request
> > > +            #25 from ved-rivos/ratified") of riscv-svadu.
> > > +
> > > +            Both Svade and Svadu extensions control the hardware behavior when
> > > +            the PTE A/D bits need to be set. The default behavior for the four
> > > +            possible combinations of these extensions in the device tree are:
> >
> > @Anup/Drew/Alex, are we missing some wording in here about it only being
> > valid to have Svadu in isolation if the provider of the devicetree has
> > actually turned on Svadu? The binding says "the default behaviour", but
> > it is not the "default" behaviour, the behaviour is a must AFAICT. If
> > you set Svadu in isolation, you /must/ have turned it on. If you set
> > Svadu and Svade, you must have Svadu turned off?
> 
> Yes, the wording should be more of requirement style using
> must or may.
> 
> How about this ?

I'm mostly just +1'ing everything below, but with a minor wording change
suggestion

> 1) Both Svade and Svadu not present in DT => Supervisor may

Neither Svade nor Svadu present...

>     assume Svade to be present and enabled or it can discover
>     based on mvendorid, marchid, and mimpid.
> 2) Only Svade present in DT => Supervisor must assume Svade
>     to be always enabled. (Obvious)
> 3) Only Svadu present in DT => Supervisor must assume Svadu
>     to be always enabled. (Obvious)
> 4) Both Svade and Svadu present in DT => Supervisor must
>     assume Svadu turned-off at boot time. To use Svadu, supervisor
>     must explicitly enable it using the SBI FWFT extension.
> 
> IMO, the #2 and #3 are definitely obvious but still worth mentioning.

Thanks,
drew

Alexandre Ghiti June 21, 2024, 8:37 a.m. UTC | #7

On 20/06/2024 08:25, Anup Patel wrote:
> On Wed, Jun 5, 2024 at 10:25 PM Conor Dooley <conor@kernel.org> wrote:
>> On Wed, Jun 05, 2024 at 08:15:08PM +0800, Yong-Xuan Wang wrote:
>>> Add entries for the Svade and Svadu extensions to the riscv,isa-extensions
>>> property.
>>>
>>> Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
>>> ---
>>>   .../devicetree/bindings/riscv/extensions.yaml | 30 +++++++++++++++++++
>>>   1 file changed, 30 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
>>> index 468c646247aa..1e30988826b9 100644
>>> --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
>>> +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
>>> @@ -153,6 +153,36 @@ properties:
>>>               ratified at commit 3f9ed34 ("Add ability to manually trigger
>>>               workflow. (#2)") of riscv-time-compare.
>>>
>>> +        - const: svade
>>> +          description: |
>>> +            The standard Svade supervisor-level extension for raising page-fault
>>> +            exceptions when PTE A/D bits need be set as ratified in the 20240213
>>> +            version of the privileged ISA specification.
>>> +
>>> +            Both Svade and Svadu extensions control the hardware behavior when
>>> +            the PTE A/D bits need to be set. The default behavior for the four
>>> +            possible combinations of these extensions in the device tree are:
>>> +            1. Neither svade nor svadu in DT: default to svade.
>> I think this needs to be expanded on, as to why nothing means svade.
> Actually if both Svade and Svadu are not present in DT then
> it is left to the platform and OpenSBI does nothing.
>
>>> +            2. Only svade in DT: use svade.
>> That's a statement of the obvious, right?
>>
>>> +            3. Only svadu in DT: use svadu.
>> This is not relevant for Svade.
>>
>>> +            4. Both svade and svadu in DT: default to svade (Linux can switch to
>>> +               svadu once the SBI FWFT extension is available).
>> "The privilege level to which this devicetree has been provided can switch to
>> Svadu if the SBI FWFT extension is available".
>>
>>> +        - const: svadu
>>> +          description: |
>>> +            The standard Svadu supervisor-level extension for hardware updating
>>> +            of PTE A/D bits as ratified at commit c1abccf ("Merge pull request
>>> +            #25 from ved-rivos/ratified") of riscv-svadu.
>>> +
>>> +            Both Svade and Svadu extensions control the hardware behavior when
>>> +            the PTE A/D bits need to be set. The default behavior for the four
>>> +            possible combinations of these extensions in the device tree are:
>> @Anup/Drew/Alex, are we missing some wording in here about it only being
>> valid to have Svadu in isolation if the provider of the devicetree has
>> actually turned on Svadu? The binding says "the default behaviour", but
>> it is not the "default" behaviour, the behaviour is a must AFAICT. If
>> you set Svadu in isolation, you /must/ have turned it on. If you set
>> Svadu and Svade, you must have Svadu turned off?
> Yes, the wording should be more of requirement style using
> must or may.
>
> How about this ?
> 1) Both Svade and Svadu not present in DT => Supervisor may
>      assume Svade to be present and enabled or it can discover
>      based on mvendorid, marchid, and mimpid.
> 2) Only Svade present in DT => Supervisor must assume Svade
>      to be always enabled. (Obvious)
> 3) Only Svadu present in DT => Supervisor must assume Svadu
>      to be always enabled. (Obvious)


I agree with all of that, but the problem is how can we guarantee that 
openSBI actually enabled svadu? This is not the case for now.


> 4) Both Svade and Svadu present in DT => Supervisor must
>      assume Svadu turned-off at boot time. To use Svadu, supervisor
>      must explicitly enable it using the SBI FWFT extension.
>
> IMO, the #2 and #3 are definitely obvious but still worth mentioning.
>
>>> +            1. Neither svade nor svadu in DT: default to svade.
>>> +            2. Only svade in DT: use svade.
>> These two are not relevant to Svadu, I'd leave them out.
>>
>>> +            3. Only svadu in DT: use svadu.
>> Again, statement of the obvious?
>>
>>> +            4. Both svade and svadu in DT: default to svade (Linux can switch to
>>> +               svadu once the SBI FWFT extension is available).
>> Same here as in the Svade entry.
>>
>> Thanks,
>> Conor.
>>
> Regards,
> Anup
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Conor Dooley June 21, 2024, 10:11 a.m. UTC | #8

On Fri, Jun 21, 2024 at 10:33:03AM +0200, Andrew Jones wrote:
> On Thu, Jun 20, 2024 at 11:55:44AM GMT, Anup Patel wrote:
> > On Wed, Jun 5, 2024 at 10:25 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Wed, Jun 05, 2024 at 08:15:08PM +0800, Yong-Xuan Wang wrote:
> > > > Add entries for the Svade and Svadu extensions to the riscv,isa-extensions
> > > > property.
> > > >
> > > > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> > > > ---
> > > >  .../devicetree/bindings/riscv/extensions.yaml | 30 +++++++++++++++++++
> > > >  1 file changed, 30 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > index 468c646247aa..1e30988826b9 100644
> > > > --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > @@ -153,6 +153,36 @@ properties:
> > > >              ratified at commit 3f9ed34 ("Add ability to manually trigger
> > > >              workflow. (#2)") of riscv-time-compare.
> > > >
> > > > +        - const: svade
> > > > +          description: |
> > > > +            The standard Svade supervisor-level extension for raising page-fault
> > > > +            exceptions when PTE A/D bits need be set as ratified in the 20240213
> > > > +            version of the privileged ISA specification.
> > > > +
> > > > +            Both Svade and Svadu extensions control the hardware behavior when
> > > > +            the PTE A/D bits need to be set. The default behavior for the four
> > > > +            possible combinations of these extensions in the device tree are:
> > > > +            1. Neither svade nor svadu in DT: default to svade.
> > >
> > > I think this needs to be expanded on, as to why nothing means svade.
> > 
> > Actually if both Svade and Svadu are not present in DT then
> > it is left to the platform and OpenSBI does nothing.
> 
> This is a good point, and maybe it's worth integrating something that
> states this case is technically unknown into the final text. (Even though
> historically this has been assumed to mean svade.)

If that is assumed to mean svade at the moment, then that's what it has
to mean going forwards also.

Conor Dooley June 21, 2024, 10:17 a.m. UTC | #9

On Fri, Jun 21, 2024 at 10:37:21AM +0200, Alexandre Ghiti wrote:
> On 20/06/2024 08:25, Anup Patel wrote:
> > On Wed, Jun 5, 2024 at 10:25 PM Conor Dooley <conor@kernel.org> wrote:
> > > On Wed, Jun 05, 2024 at 08:15:08PM +0800, Yong-Xuan Wang wrote:
> > > > Add entries for the Svade and Svadu extensions to the riscv,isa-extensions
> > > > property.
> > > > 
> > > > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> > > > ---
> > > >   .../devicetree/bindings/riscv/extensions.yaml | 30 +++++++++++++++++++
> > > >   1 file changed, 30 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > index 468c646247aa..1e30988826b9 100644
> > > > --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > @@ -153,6 +153,36 @@ properties:
> > > >               ratified at commit 3f9ed34 ("Add ability to manually trigger
> > > >               workflow. (#2)") of riscv-time-compare.
> > > > 
> > > > +        - const: svade
> > > > +          description: |
> > > > +            The standard Svade supervisor-level extension for raising page-fault
> > > > +            exceptions when PTE A/D bits need be set as ratified in the 20240213
> > > > +            version of the privileged ISA specification.
> > > > +
> > > > +            Both Svade and Svadu extensions control the hardware behavior when
> > > > +            the PTE A/D bits need to be set. The default behavior for the four
> > > > +            possible combinations of these extensions in the device tree are:
> > > > +            1. Neither svade nor svadu in DT: default to svade.
> > > I think this needs to be expanded on, as to why nothing means svade.
> > Actually if both Svade and Svadu are not present in DT then
> > it is left to the platform and OpenSBI does nothing.
> > 
> > > > +            2. Only svade in DT: use svade.
> > > That's a statement of the obvious, right?
> > > 
> > > > +            3. Only svadu in DT: use svadu.
> > > This is not relevant for Svade.
> > > 
> > > > +            4. Both svade and svadu in DT: default to svade (Linux can switch to
> > > > +               svadu once the SBI FWFT extension is available).
> > > "The privilege level to which this devicetree has been provided can switch to
> > > Svadu if the SBI FWFT extension is available".
> > > 
> > > > +        - const: svadu
> > > > +          description: |
> > > > +            The standard Svadu supervisor-level extension for hardware updating
> > > > +            of PTE A/D bits as ratified at commit c1abccf ("Merge pull request
> > > > +            #25 from ved-rivos/ratified") of riscv-svadu.
> > > > +
> > > > +            Both Svade and Svadu extensions control the hardware behavior when
> > > > +            the PTE A/D bits need to be set. The default behavior for the four
> > > > +            possible combinations of these extensions in the device tree are:
> > > @Anup/Drew/Alex, are we missing some wording in here about it only being
> > > valid to have Svadu in isolation if the provider of the devicetree has
> > > actually turned on Svadu? The binding says "the default behaviour", but
> > > it is not the "default" behaviour, the behaviour is a must AFAICT. If
> > > you set Svadu in isolation, you /must/ have turned it on. If you set
> > > Svadu and Svade, you must have Svadu turned off?
> > Yes, the wording should be more of requirement style using
> > must or may.
> > 
> > How about this ?
> > 1) Both Svade and Svadu not present in DT => Supervisor may
> >      assume Svade to be present and enabled or it can discover
> >      based on mvendorid, marchid, and mimpid.
> > 2) Only Svade present in DT => Supervisor must assume Svade
> >      to be always enabled. (Obvious)
> > 3) Only Svadu present in DT => Supervisor must assume Svadu
> >      to be always enabled. (Obvious)
> 
> 
> I agree with all of that, but the problem is how can we guarantee that
> openSBI actually enabled svadu? 
Conflation of an SBI implementation and OpenSBI aside, if the devicetree
property is defined to mean that "the supervisor must assume svadu to be
always enabled", then either it is, or the firmware's description of the
hardware is broken and it's not the supervisor's problem any more. It's
not the kernel's job to validate that the devicetree matches the
hardware.

> This is not the case for now.

What "is not the case for now"? My understanding was that, at the
moment, nothing happens with Svadu in OpenSBI. In turn, this means that
there should be no devicetrees containing Svadu (per this binding's
description) and therefore no problem?

Thanks,
Conor.

Alexandre Ghiti June 21, 2024, 12:42 p.m. UTC | #10

On 21/06/2024 12:17, Conor Dooley wrote:
> On Fri, Jun 21, 2024 at 10:37:21AM +0200, Alexandre Ghiti wrote:
>> On 20/06/2024 08:25, Anup Patel wrote:
>>> On Wed, Jun 5, 2024 at 10:25 PM Conor Dooley <conor@kernel.org> wrote:
>>>> On Wed, Jun 05, 2024 at 08:15:08PM +0800, Yong-Xuan Wang wrote:
>>>>> Add entries for the Svade and Svadu extensions to the riscv,isa-extensions
>>>>> property.
>>>>>
>>>>> Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
>>>>> ---
>>>>>    .../devicetree/bindings/riscv/extensions.yaml | 30 +++++++++++++++++++
>>>>>    1 file changed, 30 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
>>>>> index 468c646247aa..1e30988826b9 100644
>>>>> --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
>>>>> +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
>>>>> @@ -153,6 +153,36 @@ properties:
>>>>>                ratified at commit 3f9ed34 ("Add ability to manually trigger
>>>>>                workflow. (#2)") of riscv-time-compare.
>>>>>
>>>>> +        - const: svade
>>>>> +          description: |
>>>>> +            The standard Svade supervisor-level extension for raising page-fault
>>>>> +            exceptions when PTE A/D bits need be set as ratified in the 20240213
>>>>> +            version of the privileged ISA specification.
>>>>> +
>>>>> +            Both Svade and Svadu extensions control the hardware behavior when
>>>>> +            the PTE A/D bits need to be set. The default behavior for the four
>>>>> +            possible combinations of these extensions in the device tree are:
>>>>> +            1. Neither svade nor svadu in DT: default to svade.
>>>> I think this needs to be expanded on, as to why nothing means svade.
>>> Actually if both Svade and Svadu are not present in DT then
>>> it is left to the platform and OpenSBI does nothing.
>>>
>>>>> +            2. Only svade in DT: use svade.
>>>> That's a statement of the obvious, right?
>>>>
>>>>> +            3. Only svadu in DT: use svadu.
>>>> This is not relevant for Svade.
>>>>
>>>>> +            4. Both svade and svadu in DT: default to svade (Linux can switch to
>>>>> +               svadu once the SBI FWFT extension is available).
>>>> "The privilege level to which this devicetree has been provided can switch to
>>>> Svadu if the SBI FWFT extension is available".
>>>>
>>>>> +        - const: svadu
>>>>> +          description: |
>>>>> +            The standard Svadu supervisor-level extension for hardware updating
>>>>> +            of PTE A/D bits as ratified at commit c1abccf ("Merge pull request
>>>>> +            #25 from ved-rivos/ratified") of riscv-svadu.
>>>>> +
>>>>> +            Both Svade and Svadu extensions control the hardware behavior when
>>>>> +            the PTE A/D bits need to be set. The default behavior for the four
>>>>> +            possible combinations of these extensions in the device tree are:
>>>> @Anup/Drew/Alex, are we missing some wording in here about it only being
>>>> valid to have Svadu in isolation if the provider of the devicetree has
>>>> actually turned on Svadu? The binding says "the default behaviour", but
>>>> it is not the "default" behaviour, the behaviour is a must AFAICT. If
>>>> you set Svadu in isolation, you /must/ have turned it on. If you set
>>>> Svadu and Svade, you must have Svadu turned off?
>>> Yes, the wording should be more of requirement style using
>>> must or may.
>>>
>>> How about this ?
>>> 1) Both Svade and Svadu not present in DT => Supervisor may
>>>       assume Svade to be present and enabled or it can discover
>>>       based on mvendorid, marchid, and mimpid.
>>> 2) Only Svade present in DT => Supervisor must assume Svade
>>>       to be always enabled. (Obvious)
>>> 3) Only Svadu present in DT => Supervisor must assume Svadu
>>>       to be always enabled. (Obvious)
>>
>> I agree with all of that, but the problem is how can we guarantee that
>> openSBI actually enabled svadu?
> Conflation of an SBI implementation and OpenSBI aside, if the devicetree
> property is defined to mean that "the supervisor must assume svadu to be
> always enabled", then either it is, or the firmware's description of the
> hardware is broken and it's not the supervisor's problem any more. It's
> not the kernel's job to validate that the devicetree matches the
> hardware.
>
>> This is not the case for now.
> What "is not the case for now"? My understanding was that, at the
> moment, nothing happens with Svadu in OpenSBI. In turn, this means that
> there should be no devicetrees containing Svadu (per this binding's
> description) and therefore no problem?


What prevents a dtb to be passed with svadu to an old version of opensbi 
which does not support the enablement of svadu? The svadu extension will 
end up being present in the kernel but not enabled right?

Sorry if I'm completely off here, it really feels like I missed something :)


>
> Thanks,
> Conor.

Andrew Jones June 21, 2024, 1:15 p.m. UTC | #11

On Fri, Jun 21, 2024 at 02:42:15PM GMT, Alexandre Ghiti wrote:
> 
> On 21/06/2024 12:17, Conor Dooley wrote:
> > On Fri, Jun 21, 2024 at 10:37:21AM +0200, Alexandre Ghiti wrote:
> > > On 20/06/2024 08:25, Anup Patel wrote:
> > > > On Wed, Jun 5, 2024 at 10:25 PM Conor Dooley <conor@kernel.org> wrote:
> > > > > On Wed, Jun 05, 2024 at 08:15:08PM +0800, Yong-Xuan Wang wrote:
> > > > > > Add entries for the Svade and Svadu extensions to the riscv,isa-extensions
> > > > > > property.
> > > > > > 
> > > > > > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> > > > > > ---
> > > > > >    .../devicetree/bindings/riscv/extensions.yaml | 30 +++++++++++++++++++
> > > > > >    1 file changed, 30 insertions(+)
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > > > index 468c646247aa..1e30988826b9 100644
> > > > > > --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > > > @@ -153,6 +153,36 @@ properties:
> > > > > >                ratified at commit 3f9ed34 ("Add ability to manually trigger
> > > > > >                workflow. (#2)") of riscv-time-compare.
> > > > > > 
> > > > > > +        - const: svade
> > > > > > +          description: |
> > > > > > +            The standard Svade supervisor-level extension for raising page-fault
> > > > > > +            exceptions when PTE A/D bits need be set as ratified in the 20240213
> > > > > > +            version of the privileged ISA specification.
> > > > > > +
> > > > > > +            Both Svade and Svadu extensions control the hardware behavior when
> > > > > > +            the PTE A/D bits need to be set. The default behavior for the four
> > > > > > +            possible combinations of these extensions in the device tree are:
> > > > > > +            1. Neither svade nor svadu in DT: default to svade.
> > > > > I think this needs to be expanded on, as to why nothing means svade.
> > > > Actually if both Svade and Svadu are not present in DT then
> > > > it is left to the platform and OpenSBI does nothing.
> > > > 
> > > > > > +            2. Only svade in DT: use svade.
> > > > > That's a statement of the obvious, right?
> > > > > 
> > > > > > +            3. Only svadu in DT: use svadu.
> > > > > This is not relevant for Svade.
> > > > > 
> > > > > > +            4. Both svade and svadu in DT: default to svade (Linux can switch to
> > > > > > +               svadu once the SBI FWFT extension is available).
> > > > > "The privilege level to which this devicetree has been provided can switch to
> > > > > Svadu if the SBI FWFT extension is available".
> > > > > 
> > > > > > +        - const: svadu
> > > > > > +          description: |
> > > > > > +            The standard Svadu supervisor-level extension for hardware updating
> > > > > > +            of PTE A/D bits as ratified at commit c1abccf ("Merge pull request
> > > > > > +            #25 from ved-rivos/ratified") of riscv-svadu.
> > > > > > +
> > > > > > +            Both Svade and Svadu extensions control the hardware behavior when
> > > > > > +            the PTE A/D bits need to be set. The default behavior for the four
> > > > > > +            possible combinations of these extensions in the device tree are:
> > > > > @Anup/Drew/Alex, are we missing some wording in here about it only being
> > > > > valid to have Svadu in isolation if the provider of the devicetree has
> > > > > actually turned on Svadu? The binding says "the default behaviour", but
> > > > > it is not the "default" behaviour, the behaviour is a must AFAICT. If
> > > > > you set Svadu in isolation, you /must/ have turned it on. If you set
> > > > > Svadu and Svade, you must have Svadu turned off?
> > > > Yes, the wording should be more of requirement style using
> > > > must or may.
> > > > 
> > > > How about this ?
> > > > 1) Both Svade and Svadu not present in DT => Supervisor may
> > > >       assume Svade to be present and enabled or it can discover
> > > >       based on mvendorid, marchid, and mimpid.
> > > > 2) Only Svade present in DT => Supervisor must assume Svade
> > > >       to be always enabled. (Obvious)
> > > > 3) Only Svadu present in DT => Supervisor must assume Svadu
> > > >       to be always enabled. (Obvious)
> > > 
> > > I agree with all of that, but the problem is how can we guarantee that
> > > openSBI actually enabled svadu?
> > Conflation of an SBI implementation and OpenSBI aside, if the devicetree
> > property is defined to mean that "the supervisor must assume svadu to be
> > always enabled", then either it is, or the firmware's description of the
> > hardware is broken and it's not the supervisor's problem any more. It's
> > not the kernel's job to validate that the devicetree matches the
> > hardware.
> > 
> > > This is not the case for now.
> > What "is not the case for now"? My understanding was that, at the
> > moment, nothing happens with Svadu in OpenSBI. In turn, this means that
> > there should be no devicetrees containing Svadu (per this binding's
> > description) and therefore no problem?
> 
> 
> What prevents a dtb to be passed with svadu to an old version of opensbi
> which does not support the enablement of svadu? The svadu extension will end
> up being present in the kernel but not enabled right?
>

I understand the concern; old SBI implementations will leave svadu in the
DT but not actually enable it. Then, since svade may not be in the DT if
the platform doesn't support it or it was left out on purpose, Linux will
only see svadu and get unexpected exceptions. This is something we could
force easily with QEMU and an SBI implementation which doesn't do anything
for svadu. I hope vendors of real platforms, which typically provide their
own firmware and DTs, would get this right, though, especially since Linux
should fail fast in their testing when they get it wrong.

Thanks,
drew

Conor Dooley June 21, 2024, 2:04 p.m. UTC | #12

On Fri, Jun 21, 2024 at 03:15:10PM +0200, Andrew Jones wrote:
> On Fri, Jun 21, 2024 at 02:42:15PM GMT, Alexandre Ghiti wrote:
> > 
> > On 21/06/2024 12:17, Conor Dooley wrote:
> > > On Fri, Jun 21, 2024 at 10:37:21AM +0200, Alexandre Ghiti wrote:
> > > > On 20/06/2024 08:25, Anup Patel wrote:
> > > > > On Wed, Jun 5, 2024 at 10:25 PM Conor Dooley <conor@kernel.org> wrote:
> > > > > > On Wed, Jun 05, 2024 at 08:15:08PM +0800, Yong-Xuan Wang wrote:
> > > > > > > Add entries for the Svade and Svadu extensions to the riscv,isa-extensions
> > > > > > > property.
> > > > > > > 
> > > > > > > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> > > > > > > ---
> > > > > > >    .../devicetree/bindings/riscv/extensions.yaml | 30 +++++++++++++++++++
> > > > > > >    1 file changed, 30 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > > > > index 468c646247aa..1e30988826b9 100644
> > > > > > > --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > > > > +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > > > > @@ -153,6 +153,36 @@ properties:
> > > > > > >                ratified at commit 3f9ed34 ("Add ability to manually trigger
> > > > > > >                workflow. (#2)") of riscv-time-compare.
> > > > > > > 
> > > > > > > +        - const: svade
> > > > > > > +          description: |
> > > > > > > +            The standard Svade supervisor-level extension for raising page-fault
> > > > > > > +            exceptions when PTE A/D bits need be set as ratified in the 20240213
> > > > > > > +            version of the privileged ISA specification.
> > > > > > > +
> > > > > > > +            Both Svade and Svadu extensions control the hardware behavior when
> > > > > > > +            the PTE A/D bits need to be set. The default behavior for the four
> > > > > > > +            possible combinations of these extensions in the device tree are:
> > > > > > > +            1. Neither svade nor svadu in DT: default to svade.
> > > > > > I think this needs to be expanded on, as to why nothing means svade.
> > > > > Actually if both Svade and Svadu are not present in DT then
> > > > > it is left to the platform and OpenSBI does nothing.
> > > > > 
> > > > > > > +            2. Only svade in DT: use svade.
> > > > > > That's a statement of the obvious, right?
> > > > > > 
> > > > > > > +            3. Only svadu in DT: use svadu.
> > > > > > This is not relevant for Svade.
> > > > > > 
> > > > > > > +            4. Both svade and svadu in DT: default to svade (Linux can switch to
> > > > > > > +               svadu once the SBI FWFT extension is available).
> > > > > > "The privilege level to which this devicetree has been provided can switch to
> > > > > > Svadu if the SBI FWFT extension is available".
> > > > > > 
> > > > > > > +        - const: svadu
> > > > > > > +          description: |
> > > > > > > +            The standard Svadu supervisor-level extension for hardware updating
> > > > > > > +            of PTE A/D bits as ratified at commit c1abccf ("Merge pull request
> > > > > > > +            #25 from ved-rivos/ratified") of riscv-svadu.
> > > > > > > +
> > > > > > > +            Both Svade and Svadu extensions control the hardware behavior when
> > > > > > > +            the PTE A/D bits need to be set. The default behavior for the four
> > > > > > > +            possible combinations of these extensions in the device tree are:
> > > > > > @Anup/Drew/Alex, are we missing some wording in here about it only being
> > > > > > valid to have Svadu in isolation if the provider of the devicetree has
> > > > > > actually turned on Svadu? The binding says "the default behaviour", but
> > > > > > it is not the "default" behaviour, the behaviour is a must AFAICT. If
> > > > > > you set Svadu in isolation, you /must/ have turned it on. If you set
> > > > > > Svadu and Svade, you must have Svadu turned off?
> > > > > Yes, the wording should be more of requirement style using
> > > > > must or may.
> > > > > 
> > > > > How about this ?
> > > > > 1) Both Svade and Svadu not present in DT => Supervisor may
> > > > >       assume Svade to be present and enabled or it can discover
> > > > >       based on mvendorid, marchid, and mimpid.
> > > > > 2) Only Svade present in DT => Supervisor must assume Svade
> > > > >       to be always enabled. (Obvious)
> > > > > 3) Only Svadu present in DT => Supervisor must assume Svadu
> > > > >       to be always enabled. (Obvious)
> > > > 
> > > > I agree with all of that, but the problem is how can we guarantee that
> > > > openSBI actually enabled svadu?
> > > Conflation of an SBI implementation and OpenSBI aside, if the devicetree
> > > property is defined to mean that "the supervisor must assume svadu to be
> > > always enabled", then either it is, or the firmware's description of the
> > > hardware is broken and it's not the supervisor's problem any more. It's
> > > not the kernel's job to validate that the devicetree matches the
> > > hardware.
> > > 
> > > > This is not the case for now.
> > > What "is not the case for now"? My understanding was that, at the
> > > moment, nothing happens with Svadu in OpenSBI. In turn, this means that
> > > there should be no devicetrees containing Svadu (per this binding's
> > > description) and therefore no problem?
> > 
> > 
> > What prevents a dtb to be passed with svadu to an old version of opensbi
> > which does not support the enablement of svadu? The svadu extension will end
> > up being present in the kernel but not enabled right?

If you'll allow me use of my high horse, relying on undocumented
(or deprecated I suppose in this case) devicetree properties is always
going to leave people exposed to issues like this. If the property isn't
documented, then you shouldn't be passing it to the kernel.

> I understand the concern; old SBI implementations will leave svadu in the
> DT but not actually enable it. Then, since svade may not be in the DT if
> the platform doesn't support it or it was left out on purpose, Linux will
> only see svadu and get unexpected exceptions. This is something we could
> force easily with QEMU and an SBI implementation which doesn't do anything
> for svadu. I hope vendors of real platforms, which typically provide their
> own firmware and DTs, would get this right, though, especially since Linux
> should fail fast in their testing when they get it wrong.

I'll admit, I wasn't really thinking here about something like QEMU that
puts extensions into the dtb before their exact meanings are decided
upon. I almost only ever think about "real" systems, and in those cases
I would expect that if you can update the representation of the hardware
provided to (or by the firmware to Linux) with new properties, then updating
the firmware itself should be possible.

Does QEMU have the this exact problem at the moment? I know it puts
Svadu in the max cpu, but does it enable the behaviour by default, even
without the SBI implementation asking for it?

Sorta on a related note, I'm completely going head-in-sand here for ACPI,
cos I have no idea how that is being dealt with - other than that Linux
assumes that all ACPI properties have the same meaning as the DT ones. I
don't really think that that is sustainable, but it is what we are doing
at present. Maybe I should put that in boot.rst or in acpi.rst?

Also on the ACPI side of things, and I am going an uber devil's advocate
here, the version of the spec that we documented as defining our parsing
rules never mentions svade or svadu, so is it even valid to use them on
ACPI systems?

Andrew Jones June 21, 2024, 2:52 p.m. UTC | #13

On Fri, Jun 21, 2024 at 03:04:47PM GMT, Conor Dooley wrote:
> On Fri, Jun 21, 2024 at 03:15:10PM +0200, Andrew Jones wrote:
> > On Fri, Jun 21, 2024 at 02:42:15PM GMT, Alexandre Ghiti wrote:
> > > 
> > > On 21/06/2024 12:17, Conor Dooley wrote:
> > > > On Fri, Jun 21, 2024 at 10:37:21AM +0200, Alexandre Ghiti wrote:
> > > > > On 20/06/2024 08:25, Anup Patel wrote:
> > > > > > On Wed, Jun 5, 2024 at 10:25 PM Conor Dooley <conor@kernel.org> wrote:
> > > > > > > On Wed, Jun 05, 2024 at 08:15:08PM +0800, Yong-Xuan Wang wrote:
> > > > > > > > Add entries for the Svade and Svadu extensions to the riscv,isa-extensions
> > > > > > > > property.
> > > > > > > > 
> > > > > > > > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> > > > > > > > ---
> > > > > > > >    .../devicetree/bindings/riscv/extensions.yaml | 30 +++++++++++++++++++
> > > > > > > >    1 file changed, 30 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > > > > > index 468c646247aa..1e30988826b9 100644
> > > > > > > > --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > > > > > +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > > > > > @@ -153,6 +153,36 @@ properties:
> > > > > > > >                ratified at commit 3f9ed34 ("Add ability to manually trigger
> > > > > > > >                workflow. (#2)") of riscv-time-compare.
> > > > > > > > 
> > > > > > > > +        - const: svade
> > > > > > > > +          description: |
> > > > > > > > +            The standard Svade supervisor-level extension for raising page-fault
> > > > > > > > +            exceptions when PTE A/D bits need be set as ratified in the 20240213
> > > > > > > > +            version of the privileged ISA specification.
> > > > > > > > +
> > > > > > > > +            Both Svade and Svadu extensions control the hardware behavior when
> > > > > > > > +            the PTE A/D bits need to be set. The default behavior for the four
> > > > > > > > +            possible combinations of these extensions in the device tree are:
> > > > > > > > +            1. Neither svade nor svadu in DT: default to svade.
> > > > > > > I think this needs to be expanded on, as to why nothing means svade.
> > > > > > Actually if both Svade and Svadu are not present in DT then
> > > > > > it is left to the platform and OpenSBI does nothing.
> > > > > > 
> > > > > > > > +            2. Only svade in DT: use svade.
> > > > > > > That's a statement of the obvious, right?
> > > > > > > 
> > > > > > > > +            3. Only svadu in DT: use svadu.
> > > > > > > This is not relevant for Svade.
> > > > > > > 
> > > > > > > > +            4. Both svade and svadu in DT: default to svade (Linux can switch to
> > > > > > > > +               svadu once the SBI FWFT extension is available).
> > > > > > > "The privilege level to which this devicetree has been provided can switch to
> > > > > > > Svadu if the SBI FWFT extension is available".
> > > > > > > 
> > > > > > > > +        - const: svadu
> > > > > > > > +          description: |
> > > > > > > > +            The standard Svadu supervisor-level extension for hardware updating
> > > > > > > > +            of PTE A/D bits as ratified at commit c1abccf ("Merge pull request
> > > > > > > > +            #25 from ved-rivos/ratified") of riscv-svadu.
> > > > > > > > +
> > > > > > > > +            Both Svade and Svadu extensions control the hardware behavior when
> > > > > > > > +            the PTE A/D bits need to be set. The default behavior for the four
> > > > > > > > +            possible combinations of these extensions in the device tree are:
> > > > > > > @Anup/Drew/Alex, are we missing some wording in here about it only being
> > > > > > > valid to have Svadu in isolation if the provider of the devicetree has
> > > > > > > actually turned on Svadu? The binding says "the default behaviour", but
> > > > > > > it is not the "default" behaviour, the behaviour is a must AFAICT. If
> > > > > > > you set Svadu in isolation, you /must/ have turned it on. If you set
> > > > > > > Svadu and Svade, you must have Svadu turned off?
> > > > > > Yes, the wording should be more of requirement style using
> > > > > > must or may.
> > > > > > 
> > > > > > How about this ?
> > > > > > 1) Both Svade and Svadu not present in DT => Supervisor may
> > > > > >       assume Svade to be present and enabled or it can discover
> > > > > >       based on mvendorid, marchid, and mimpid.
> > > > > > 2) Only Svade present in DT => Supervisor must assume Svade
> > > > > >       to be always enabled. (Obvious)
> > > > > > 3) Only Svadu present in DT => Supervisor must assume Svadu
> > > > > >       to be always enabled. (Obvious)
> > > > > 
> > > > > I agree with all of that, but the problem is how can we guarantee that
> > > > > openSBI actually enabled svadu?
> > > > Conflation of an SBI implementation and OpenSBI aside, if the devicetree
> > > > property is defined to mean that "the supervisor must assume svadu to be
> > > > always enabled", then either it is, or the firmware's description of the
> > > > hardware is broken and it's not the supervisor's problem any more. It's
> > > > not the kernel's job to validate that the devicetree matches the
> > > > hardware.
> > > > 
> > > > > This is not the case for now.
> > > > What "is not the case for now"? My understanding was that, at the
> > > > moment, nothing happens with Svadu in OpenSBI. In turn, this means that
> > > > there should be no devicetrees containing Svadu (per this binding's
> > > > description) and therefore no problem?
> > > 
> > > 
> > > What prevents a dtb to be passed with svadu to an old version of opensbi
> > > which does not support the enablement of svadu? The svadu extension will end
> > > up being present in the kernel but not enabled right?
> 
> If you'll allow me use of my high horse, relying on undocumented
> (or deprecated I suppose in this case) devicetree properties is always
> going to leave people exposed to issues like this. If the property isn't
> documented, then you shouldn't be passing it to the kernel.
> 
> > I understand the concern; old SBI implementations will leave svadu in the
> > DT but not actually enable it. Then, since svade may not be in the DT if
> > the platform doesn't support it or it was left out on purpose, Linux will
> > only see svadu and get unexpected exceptions. This is something we could
> > force easily with QEMU and an SBI implementation which doesn't do anything
> > for svadu. I hope vendors of real platforms, which typically provide their
> > own firmware and DTs, would get this right, though, especially since Linux
> > should fail fast in their testing when they get it wrong.
> 
> I'll admit, I wasn't really thinking here about something like QEMU that
> puts extensions into the dtb before their exact meanings are decided
> upon. I almost only ever think about "real" systems, and in those cases
> I would expect that if you can update the representation of the hardware
> provided to (or by the firmware to Linux) with new properties, then updating
> the firmware itself should be possible.
> 
> Does QEMU have the this exact problem at the moment? I know it puts
> Svadu in the max cpu, but does it enable the behaviour by default, even
> without the SBI implementation asking for it?

Yes, because QEMU has done hardware A/D updating since it first started
supporting riscv, which means it did svadu when neither svadu nor svade
were in the DT. The "fix" for that was to ensure we have svadu and !svade
by default, which means we've perfectly realized Alexandre's concern...
We should be able to change the named cpu types that don't support svadu
to only have svade in their DTs, since that would actually be fixing those
cpu types, but we'll need to discuss how to proceed with the generic cpu
types like 'max'.

> 
> Sorta on a related note, I'm completely going head-in-sand here for ACPI,
> cos I have no idea how that is being dealt with - other than that Linux
> assumes that all ACPI properties have the same meaning as the DT ones. I
> don't really think that that is sustainable, but it is what we are doing
> at present. Maybe I should put that in boot.rst or in acpi.rst?

Yes, I think that's what we're doing right now and documenting that
assumption is a good idea.

> 
> Also on the ACPI side of things, and I am going an uber devil's advocate
> here, the version of the spec that we documented as defining our parsing
> rules never mentions svade or svadu, so is it even valid to use them on
> ACPI systems?

I think that ISA string format chapter implies that any extension name
that is in the specified format can be parsed, which implies it can be
interpreted as an available extension, even if it's not mentioned in
the spec. But maybe I'm just pushing a big foot into a small shoe since
I don't really want to try and figure out how to get that chapter
changed...

Thanks,
drew

Conor Dooley June 21, 2024, 2:58 p.m. UTC | #14

On Fri, Jun 21, 2024 at 04:52:09PM +0200, Andrew Jones wrote:
> On Fri, Jun 21, 2024 at 03:04:47PM GMT, Conor Dooley wrote:
> > On Fri, Jun 21, 2024 at 03:15:10PM +0200, Andrew Jones wrote:
> > > On Fri, Jun 21, 2024 at 02:42:15PM GMT, Alexandre Ghiti wrote:

> > > I understand the concern; old SBI implementations will leave svadu in the
> > > DT but not actually enable it. Then, since svade may not be in the DT if
> > > the platform doesn't support it or it was left out on purpose, Linux will
> > > only see svadu and get unexpected exceptions. This is something we could
> > > force easily with QEMU and an SBI implementation which doesn't do anything
> > > for svadu. I hope vendors of real platforms, which typically provide their
> > > own firmware and DTs, would get this right, though, especially since Linux
> > > should fail fast in their testing when they get it wrong.
> > 
> > I'll admit, I wasn't really thinking here about something like QEMU that
> > puts extensions into the dtb before their exact meanings are decided
> > upon. I almost only ever think about "real" systems, and in those cases
> > I would expect that if you can update the representation of the hardware
> > provided to (or by the firmware to Linux) with new properties, then updating
> > the firmware itself should be possible.
> > 
> > Does QEMU have the this exact problem at the moment? I know it puts
> > Svadu in the max cpu, but does it enable the behaviour by default, even
> > without the SBI implementation asking for it?
> 
> Yes, because QEMU has done hardware A/D updating since it first started
> supporting riscv, which means it did svadu when neither svadu nor svade
> were in the DT. The "fix" for that was to ensure we have svadu and !svade
> by default, which means we've perfectly realized Alexandre's concern...
> We should be able to change the named cpu types that don't support svadu
> to only have svade in their DTs, since that would actually be fixing those
> cpu types, but we'll need to discuss how to proceed with the generic cpu
> types like 'max'.

Correct me please, since I think I am misunderstanding: At the moment
QEMU does A/D updating whether or not the SBI implantation asks for it,
with the max CPU. The SBI implementation doesn't understand Svadu and
won't strip it. The kernel will get a DT with Svadu in it, but Svadu will
be enabled, so it is not a problem.

Andrew Jones June 21, 2024, 3:08 p.m. UTC | #15

On Fri, Jun 21, 2024 at 03:58:18PM GMT, Conor Dooley wrote:
> On Fri, Jun 21, 2024 at 04:52:09PM +0200, Andrew Jones wrote:
> > On Fri, Jun 21, 2024 at 03:04:47PM GMT, Conor Dooley wrote:
> > > On Fri, Jun 21, 2024 at 03:15:10PM +0200, Andrew Jones wrote:
> > > > On Fri, Jun 21, 2024 at 02:42:15PM GMT, Alexandre Ghiti wrote:
> 
> > > > I understand the concern; old SBI implementations will leave svadu in the
> > > > DT but not actually enable it. Then, since svade may not be in the DT if
> > > > the platform doesn't support it or it was left out on purpose, Linux will
> > > > only see svadu and get unexpected exceptions. This is something we could
> > > > force easily with QEMU and an SBI implementation which doesn't do anything
> > > > for svadu. I hope vendors of real platforms, which typically provide their
> > > > own firmware and DTs, would get this right, though, especially since Linux
> > > > should fail fast in their testing when they get it wrong.
> > > 
> > > I'll admit, I wasn't really thinking here about something like QEMU that
> > > puts extensions into the dtb before their exact meanings are decided
> > > upon. I almost only ever think about "real" systems, and in those cases
> > > I would expect that if you can update the representation of the hardware
> > > provided to (or by the firmware to Linux) with new properties, then updating
> > > the firmware itself should be possible.
> > > 
> > > Does QEMU have the this exact problem at the moment? I know it puts
> > > Svadu in the max cpu, but does it enable the behaviour by default, even
> > > without the SBI implementation asking for it?
> > 
> > Yes, because QEMU has done hardware A/D updating since it first started
> > supporting riscv, which means it did svadu when neither svadu nor svade
> > were in the DT. The "fix" for that was to ensure we have svadu and !svade
> > by default, which means we've perfectly realized Alexandre's concern...
> > We should be able to change the named cpu types that don't support svadu
> > to only have svade in their DTs, since that would actually be fixing those
> > cpu types, but we'll need to discuss how to proceed with the generic cpu
> > types like 'max'.
> 
> Correct me please, since I think I am misunderstanding: At the moment
> QEMU does A/D updating whether or not the SBI implantation asks for it,
> with the max CPU. The SBI implementation doesn't understand Svadu and
> won't strip it. The kernel will get a DT with Svadu in it, but Svadu will
> be enabled, so it is not a problem.

Oh, of course you're right! I managed to reverse things some odd number of
times (more than once!) in my head and ended up backwards...

Thanks,
drew

Conor Dooley June 22, 2024, 12:01 p.m. UTC | #16

On Fri, Jun 21, 2024 at 05:08:01PM +0200, Andrew Jones wrote:
> On Fri, Jun 21, 2024 at 03:58:18PM GMT, Conor Dooley wrote:
> > On Fri, Jun 21, 2024 at 04:52:09PM +0200, Andrew Jones wrote:
> > > On Fri, Jun 21, 2024 at 03:04:47PM GMT, Conor Dooley wrote:
> > > > On Fri, Jun 21, 2024 at 03:15:10PM +0200, Andrew Jones wrote:
> > > > > On Fri, Jun 21, 2024 at 02:42:15PM GMT, Alexandre Ghiti wrote:
> > 
> > > > > I understand the concern; old SBI implementations will leave svadu in the
> > > > > DT but not actually enable it. Then, since svade may not be in the DT if
> > > > > the platform doesn't support it or it was left out on purpose, Linux will
> > > > > only see svadu and get unexpected exceptions. This is something we could
> > > > > force easily with QEMU and an SBI implementation which doesn't do anything
> > > > > for svadu. I hope vendors of real platforms, which typically provide their
> > > > > own firmware and DTs, would get this right, though, especially since Linux
> > > > > should fail fast in their testing when they get it wrong.
> > > > 
> > > > I'll admit, I wasn't really thinking here about something like QEMU that
> > > > puts extensions into the dtb before their exact meanings are decided
> > > > upon. I almost only ever think about "real" systems, and in those cases
> > > > I would expect that if you can update the representation of the hardware
> > > > provided to (or by the firmware to Linux) with new properties, then updating
> > > > the firmware itself should be possible.
> > > > 
> > > > Does QEMU have the this exact problem at the moment? I know it puts
> > > > Svadu in the max cpu, but does it enable the behaviour by default, even
> > > > without the SBI implementation asking for it?
> > > 
> > > Yes, because QEMU has done hardware A/D updating since it first started
> > > supporting riscv, which means it did svadu when neither svadu nor svade
> > > were in the DT. The "fix" for that was to ensure we have svadu and !svade
> > > by default, which means we've perfectly realized Alexandre's concern...
> > > We should be able to change the named cpu types that don't support svadu
> > > to only have svade in their DTs, since that would actually be fixing those
> > > cpu types, but we'll need to discuss how to proceed with the generic cpu
> > > types like 'max'.
> > 
> > Correct me please, since I think I am misunderstanding: At the moment
> > QEMU does A/D updating whether or not the SBI implantation asks for it,
> > with the max CPU. The SBI implementation doesn't understand Svadu and
> > won't strip it. The kernel will get a DT with Svadu in it, but Svadu will
> > be enabled, so it is not a problem.
> 
> Oh, of course you're right! I managed to reverse things some odd number of
> times (more than once!) in my head and ended up backwards...

I mean, I've been really confused about this whole thing the entire
time, so ye..

Speaking of QEMU, what happens if I try to turn on svade and svadu in
QEMU? It looks like there's some handling of it that does things
conditionally based !svade && svade, but I couldn't tell if it would do
what we are describing in this thread.

Yong-Xuan Wang June 25, 2024, 10:15 a.m. UTC | #17

Hi Anup/Andrew/Alexandre,

Thanks a lot! So the description of dt-binding of Svade would be:

The standard Svade supervisor-level extension for SW-managed PTE A/D
bit updates as ratified in the 20240213 version of the privileged
ISA specification.
1) Neither Svade nor Svadu present in DT => It is technically
    unknown whether the platform uses Svade or Svadu. Supervisor may
    assume Svade to be present and enabled or it can discover based
    on mvendorid, marchid, and mimpid.
2) Only Svade present in DT => Supervisor must assume Svade
    to be always enabled. (Obvious)
3) Only Svadu present in DT => Supervisor must assume Svadu
    to be always enabled. (Obvious)
4) Both Svade and Svadu present in DT => Supervisor must
    assume Svadu turned-off at boot time. To use Svadu, supervisor
    must explicitly enable it using the SBI FWFT extension.

Would you mind providing a Co-developed-by for this patch?

Regards,
Yong-Xuan


On Fri, Jun 21, 2024 at 4:33 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Thu, Jun 20, 2024 at 11:55:44AM GMT, Anup Patel wrote:
> > On Wed, Jun 5, 2024 at 10:25 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Wed, Jun 05, 2024 at 08:15:08PM +0800, Yong-Xuan Wang wrote:
> > > > Add entries for the Svade and Svadu extensions to the riscv,isa-extensions
> > > > property.
> > > >
> > > > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> > > > ---
> > > >  .../devicetree/bindings/riscv/extensions.yaml | 30 +++++++++++++++++++
> > > >  1 file changed, 30 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > index 468c646247aa..1e30988826b9 100644
> > > > --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > @@ -153,6 +153,36 @@ properties:
> > > >              ratified at commit 3f9ed34 ("Add ability to manually trigger
> > > >              workflow. (#2)") of riscv-time-compare.
> > > >
> > > > +        - const: svade
> > > > +          description: |
> > > > +            The standard Svade supervisor-level extension for raising page-fault
> > > > +            exceptions when PTE A/D bits need be set as ratified in the 20240213
> > > > +            version of the privileged ISA specification.
> > > > +
> > > > +            Both Svade and Svadu extensions control the hardware behavior when
> > > > +            the PTE A/D bits need to be set. The default behavior for the four
> > > > +            possible combinations of these extensions in the device tree are:
> > > > +            1. Neither svade nor svadu in DT: default to svade.
> > >
> > > I think this needs to be expanded on, as to why nothing means svade.
> >
> > Actually if both Svade and Svadu are not present in DT then
> > it is left to the platform and OpenSBI does nothing.
>
> This is a good point, and maybe it's worth integrating something that
> states this case is technically unknown into the final text. (Even though
> historically this has been assumed to mean svade.)
>
> >
> > >
> > > > +            2. Only svade in DT: use svade.
> > >
> > > That's a statement of the obvious, right?
> > >
> > > > +            3. Only svadu in DT: use svadu.
> > >
> > > This is not relevant for Svade.
> > >
> > > > +            4. Both svade and svadu in DT: default to svade (Linux can switch to
> > > > +               svadu once the SBI FWFT extension is available).
> > >
> > > "The privilege level to which this devicetree has been provided can switch to
> > > Svadu if the SBI FWFT extension is available".
> > >
> > > > +        - const: svadu
> > > > +          description: |
> > > > +            The standard Svadu supervisor-level extension for hardware updating
> > > > +            of PTE A/D bits as ratified at commit c1abccf ("Merge pull request
> > > > +            #25 from ved-rivos/ratified") of riscv-svadu.
> > > > +
> > > > +            Both Svade and Svadu extensions control the hardware behavior when
> > > > +            the PTE A/D bits need to be set. The default behavior for the four
> > > > +            possible combinations of these extensions in the device tree are:
> > >
> > > @Anup/Drew/Alex, are we missing some wording in here about it only being
> > > valid to have Svadu in isolation if the provider of the devicetree has
> > > actually turned on Svadu? The binding says "the default behaviour", but
> > > it is not the "default" behaviour, the behaviour is a must AFAICT. If
> > > you set Svadu in isolation, you /must/ have turned it on. If you set
> > > Svadu and Svade, you must have Svadu turned off?
> >
> > Yes, the wording should be more of requirement style using
> > must or may.
> >
> > How about this ?
>
> I'm mostly just +1'ing everything below, but with a minor wording change
> suggestion
>
> > 1) Both Svade and Svadu not present in DT => Supervisor may
>
> Neither Svade nor Svadu present...
>
> >     assume Svade to be present and enabled or it can discover
> >     based on mvendorid, marchid, and mimpid.
> > 2) Only Svade present in DT => Supervisor must assume Svade
> >     to be always enabled. (Obvious)
> > 3) Only Svadu present in DT => Supervisor must assume Svadu
> >     to be always enabled. (Obvious)
> > 4) Both Svade and Svadu present in DT => Supervisor must
> >     assume Svadu turned-off at boot time. To use Svadu, supervisor
> >     must explicitly enable it using the SBI FWFT extension.
> >
> > IMO, the #2 and #3 are definitely obvious but still worth mentioning.
>
> Thanks,
> drew

Yong-Xuan Wang June 25, 2024, 10:17 a.m. UTC | #18

Hi Conor,

On Sat, Jun 22, 2024 at 8:01 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Fri, Jun 21, 2024 at 05:08:01PM +0200, Andrew Jones wrote:
> > On Fri, Jun 21, 2024 at 03:58:18PM GMT, Conor Dooley wrote:
> > > On Fri, Jun 21, 2024 at 04:52:09PM +0200, Andrew Jones wrote:
> > > > On Fri, Jun 21, 2024 at 03:04:47PM GMT, Conor Dooley wrote:
> > > > > On Fri, Jun 21, 2024 at 03:15:10PM +0200, Andrew Jones wrote:
> > > > > > On Fri, Jun 21, 2024 at 02:42:15PM GMT, Alexandre Ghiti wrote:
> > >
> > > > > > I understand the concern; old SBI implementations will leave svadu in the
> > > > > > DT but not actually enable it. Then, since svade may not be in the DT if
> > > > > > the platform doesn't support it or it was left out on purpose, Linux will
> > > > > > only see svadu and get unexpected exceptions. This is something we could
> > > > > > force easily with QEMU and an SBI implementation which doesn't do anything
> > > > > > for svadu. I hope vendors of real platforms, which typically provide their
> > > > > > own firmware and DTs, would get this right, though, especially since Linux
> > > > > > should fail fast in their testing when they get it wrong.
> > > > >
> > > > > I'll admit, I wasn't really thinking here about something like QEMU that
> > > > > puts extensions into the dtb before their exact meanings are decided
> > > > > upon. I almost only ever think about "real" systems, and in those cases
> > > > > I would expect that if you can update the representation of the hardware
> > > > > provided to (or by the firmware to Linux) with new properties, then updating
> > > > > the firmware itself should be possible.
> > > > >
> > > > > Does QEMU have the this exact problem at the moment? I know it puts
> > > > > Svadu in the max cpu, but does it enable the behaviour by default, even
> > > > > without the SBI implementation asking for it?
> > > >
> > > > Yes, because QEMU has done hardware A/D updating since it first started
> > > > supporting riscv, which means it did svadu when neither svadu nor svade
> > > > were in the DT. The "fix" for that was to ensure we have svadu and !svade
> > > > by default, which means we've perfectly realized Alexandre's concern...
> > > > We should be able to change the named cpu types that don't support svadu
> > > > to only have svade in their DTs, since that would actually be fixing those
> > > > cpu types, but we'll need to discuss how to proceed with the generic cpu
> > > > types like 'max'.
> > >
> > > Correct me please, since I think I am misunderstanding: At the moment
> > > QEMU does A/D updating whether or not the SBI implantation asks for it,
> > > with the max CPU. The SBI implementation doesn't understand Svadu and
> > > won't strip it. The kernel will get a DT with Svadu in it, but Svadu will
> > > be enabled, so it is not a problem.
> >
> > Oh, of course you're right! I managed to reverse things some odd number of
> > times (more than once!) in my head and ended up backwards...
>
> I mean, I've been really confused about this whole thing the entire
> time, so ye..
>
> Speaking of QEMU, what happens if I try to turn on svade and svadu in
> QEMU? It looks like there's some handling of it that does things
> conditionally based !svade && svade, but I couldn't tell if it would do
> what we are describing in this thread.

When both Svadu and Svade are specified in QEMU, the reset value of
menvcfg.ADUE is 0:

env->menvcfg = (cpu->cfg.ext_svpbmt ? MENVCFG_PBMTE : 0) |
                (!cpu->cfg.ext_svade && cpu->cfg.ext_svadu ?
                MENVCFG_ADUE : 0);

The runtime behavior depends on menvcfg.ADUE:

    bool svade = riscv_cpu_cfg(env)->ext_svade;
    bool svadu = riscv_cpu_cfg(env)->ext_svadu;
    bool adue = svadu ? env->menvcfg & MENVCFG_ADUE : !svade;

Regardless of whether OpenSBI supports the Svadu enablement,
Supervisor can assume that QEMU uses Svade when it doesn't
explicitly turn on Svadu.

Regards,
Yong-Xuan

Andrew Jones June 25, 2024, 10:19 a.m. UTC | #19

On Sat, Jun 22, 2024 at 01:01:30PM GMT, Conor Dooley wrote:
> On Fri, Jun 21, 2024 at 05:08:01PM +0200, Andrew Jones wrote:
> > On Fri, Jun 21, 2024 at 03:58:18PM GMT, Conor Dooley wrote:
> > > On Fri, Jun 21, 2024 at 04:52:09PM +0200, Andrew Jones wrote:
> > > > On Fri, Jun 21, 2024 at 03:04:47PM GMT, Conor Dooley wrote:
> > > > > On Fri, Jun 21, 2024 at 03:15:10PM +0200, Andrew Jones wrote:
> > > > > > On Fri, Jun 21, 2024 at 02:42:15PM GMT, Alexandre Ghiti wrote:
> > > 
> > > > > > I understand the concern; old SBI implementations will leave svadu in the
> > > > > > DT but not actually enable it. Then, since svade may not be in the DT if
> > > > > > the platform doesn't support it or it was left out on purpose, Linux will
> > > > > > only see svadu and get unexpected exceptions. This is something we could
> > > > > > force easily with QEMU and an SBI implementation which doesn't do anything
> > > > > > for svadu. I hope vendors of real platforms, which typically provide their
> > > > > > own firmware and DTs, would get this right, though, especially since Linux
> > > > > > should fail fast in their testing when they get it wrong.
> > > > > 
> > > > > I'll admit, I wasn't really thinking here about something like QEMU that
> > > > > puts extensions into the dtb before their exact meanings are decided
> > > > > upon. I almost only ever think about "real" systems, and in those cases
> > > > > I would expect that if you can update the representation of the hardware
> > > > > provided to (or by the firmware to Linux) with new properties, then updating
> > > > > the firmware itself should be possible.
> > > > > 
> > > > > Does QEMU have the this exact problem at the moment? I know it puts
> > > > > Svadu in the max cpu, but does it enable the behaviour by default, even
> > > > > without the SBI implementation asking for it?
> > > > 
> > > > Yes, because QEMU has done hardware A/D updating since it first started
> > > > supporting riscv, which means it did svadu when neither svadu nor svade
> > > > were in the DT. The "fix" for that was to ensure we have svadu and !svade
> > > > by default, which means we've perfectly realized Alexandre's concern...
> > > > We should be able to change the named cpu types that don't support svadu
> > > > to only have svade in their DTs, since that would actually be fixing those
> > > > cpu types, but we'll need to discuss how to proceed with the generic cpu
> > > > types like 'max'.
> > > 
> > > Correct me please, since I think I am misunderstanding: At the moment
> > > QEMU does A/D updating whether or not the SBI implantation asks for it,
> > > with the max CPU. The SBI implementation doesn't understand Svadu and
> > > won't strip it. The kernel will get a DT with Svadu in it, but Svadu will
> > > be enabled, so it is not a problem.
> > 
> > Oh, of course you're right! I managed to reverse things some odd number of
> > times (more than once!) in my head and ended up backwards...
> 
> I mean, I've been really confused about this whole thing the entire
> time, so ye..
> 
> Speaking of QEMU, what happens if I try to turn on svade and svadu in
> QEMU? It looks like there's some handling of it that does things
> conditionally based !svade && svade, but I couldn't tell if it would do
> what we are describing in this thread.

It'll use exception mode, but {m,h}envcfg.ADUE is still provided to allow
software to switch to hardware updating when FWFT exists. So I think we're
good to go.

Thanks,
drew

[v5,2/4] dt-bindings: riscv: Add Svade and Svadu Entries

Commit Message

Comments

Patch