[PATCH v5 0/4] Add Svade and Svadu Extensions Support

[PATCH v5 0/4] Add Svade and Svadu Extensions Support

[Yong-Xuan Wang]

Svade and Svadu extensions represent two schemes for managing the PTE A/D
bit. When the PTE A/D bits need to be set, Svade extension intdicates that
a related page fault will be raised. In contrast, the Svadu extension
supports hardware updating of PTE A/D bits. This series enables Svade and
Svadu extensions for both host and guest OS.

Regrading the mailing thread[1], we have 4 possible combinations of
these extensions in the device tree, the default hardware behavior for
these possibilities 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.

The Svade extension is mandatory and the Svadu extension is optional in
RVA23 profile. Platforms want to take the advantage of Svadu can choose
3. Those are aware of the profile can choose 4, and Linux won't get the
benefit of svadu until the SBI FWFT extension is available.

[1] https://lore.kernel.org/linux-kernel/20240527-e9845c06619bca5cd285098c@orel/T/#m29644eb88e241ec282df4ccd5199514e913b06ee

---
v5:
- remove all Acked-by and Reviewed-by (Conor, Andrew)
- add Svade support
- update the arch_has_hw_pte_young() in PATCH1
- update the dtbinding in PATCH2 (Alexandre, Andrew)
- check the availibility of Svadu for Guest/VM based on
  arch_has_hw_pte_young() in PATCH3

v4:
- fix 32bit kernel build error in PATCH1 (Conor)
- update the status of Svadu extension to ratified in PATCH2
- add the PATCH4 to suporrt SBI_FWFT_PTE_AD_HW_UPDATING for guest OS
- update the PATCH1 and PATCH3 to integrate with FWFT extension
- rebase PATCH5 on the lastest get-reg-list test (Andrew)

v3:
- fix the control bit name to ADUE in PATCH1 and PATCH3
- update get-reg-list in PATCH4

v2:
- add Co-developed-by: in PATCH1
- use riscv_has_extension_unlikely() to runtime patch the branch in PATCH1
- update dt-binding

Yong-Xuan Wang (4):
  RISC-V: Add Svade and Svadu Extensions Support
  dt-bindings: riscv: Add Svade and Svadu Entries
  RISC-V: KVM: Add Svade and Svadu Extensions Support for Guest/VM
  KVM: riscv: selftests: Add Svade and Svadu Extension to get-reg-list
    test

 .../devicetree/bindings/riscv/extensions.yaml | 30 +++++++++++++++++++
 arch/riscv/Kconfig                            |  1 +
 arch/riscv/include/asm/csr.h                  |  1 +
 arch/riscv/include/asm/hwcap.h                |  2 ++
 arch/riscv/include/asm/pgtable.h              | 14 ++++++++-
 arch/riscv/include/uapi/asm/kvm.h             |  2 ++
 arch/riscv/kernel/cpufeature.c                |  2 ++
 arch/riscv/kvm/vcpu.c                         |  6 ++++
 arch/riscv/kvm/vcpu_onereg.c                  |  6 ++++
 .../selftests/kvm/riscv/get-reg-list.c        |  8 +++++
 10 files changed, 71 insertions(+), 1 deletion(-)

-- 
2.17.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v5 1/4] RISC-V: Add Svade and Svadu Extensions Support

[Yong-Xuan Wang]

Svade and Svadu extensions represent two schemes for managing the PTE A/D
bits. When the PTE A/D bits need to be set, Svade extension intdicates
that a related page fault will be raised. In contrast, the Svadu extension
supports hardware updating of PTE A/D bits. Since the Svade extension is
mandatory and the Svadu extension is optional in RVA23 profile, by default
the M-mode firmware will enable the Svadu extension in the menvcfg CSR
when only Svadu is present in DT.

This patch detects Svade and Svadu extensions from DT and adds
arch_has_hw_pte_young() to enable optimization in MGLRU and
__wp_page_copy_user() when we have the PTE A/D bits hardware updating
support.

Co-developed-by: Jinyu Tang <tjytimi@163.com>
Signed-off-by: Jinyu Tang <tjytimi@163.com>
Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
---
 arch/riscv/Kconfig               |  1 +
 arch/riscv/include/asm/csr.h     |  1 +
 arch/riscv/include/asm/hwcap.h   |  2 ++
 arch/riscv/include/asm/pgtable.h | 14 +++++++++++++-
 arch/riscv/kernel/cpufeature.c   |  2 ++
 5 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index b94176e25be1..dbfe2be99bf9 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -36,6 +36,7 @@ config RISCV
 	select ARCH_HAS_PMEM_API
 	select ARCH_HAS_PREPARE_SYNC_CORE_CMD
 	select ARCH_HAS_PTE_SPECIAL
+	select ARCH_HAS_HW_PTE_YOUNG
 	select ARCH_HAS_SET_DIRECT_MAP if MMU
 	select ARCH_HAS_SET_MEMORY if MMU
 	select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
index 25966995da04..524cd4131c71 100644
--- a/arch/riscv/include/asm/csr.h
+++ b/arch/riscv/include/asm/csr.h
@@ -195,6 +195,7 @@
 /* xENVCFG flags */
 #define ENVCFG_STCE			(_AC(1, ULL) << 63)
 #define ENVCFG_PBMTE			(_AC(1, ULL) << 62)
+#define ENVCFG_ADUE			(_AC(1, ULL) << 61)
 #define ENVCFG_CBZE			(_AC(1, UL) << 7)
 #define ENVCFG_CBCFE			(_AC(1, UL) << 6)
 #define ENVCFG_CBIE_SHIFT		4
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index e17d0078a651..35d7aa49785d 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -81,6 +81,8 @@
 #define RISCV_ISA_EXT_ZTSO		72
 #define RISCV_ISA_EXT_ZACAS		73
 #define RISCV_ISA_EXT_XANDESPMU		74
+#define RISCV_ISA_EXT_SVADE             75
+#define RISCV_ISA_EXT_SVADU		76
 
 #define RISCV_ISA_EXT_XLINUXENVCFG	127
 
diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index aad8b8ca51f1..7287ea4a6160 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -120,6 +120,7 @@
 #include <asm/tlbflush.h>
 #include <linux/mm_types.h>
 #include <asm/compat.h>
+#include <asm/cpufeature.h>
 
 #define __page_val_to_pfn(_val)  (((_val) & _PAGE_PFN_MASK) >> _PAGE_PFN_SHIFT)
 
@@ -288,7 +289,6 @@ static inline pte_t pud_pte(pud_t pud)
 }
 
 #ifdef CONFIG_RISCV_ISA_SVNAPOT
-#include <asm/cpufeature.h>
 
 static __always_inline bool has_svnapot(void)
 {
@@ -624,6 +624,18 @@ static inline pgprot_t pgprot_writecombine(pgprot_t _prot)
 	return __pgprot(prot);
 }
 
+/*
+ * Both Svade and Svadu control the hardware behavior when the PTE A/D bits need to be set. By
+ * default the M-mode firmware enables the hardware updating scheme when only Svadu is present in
+ * DT.
+ */
+#define arch_has_hw_pte_young arch_has_hw_pte_young
+static inline bool arch_has_hw_pte_young(void)
+{
+	return riscv_has_extension_unlikely(RISCV_ISA_EXT_SVADU) &&
+	       !riscv_has_extension_likely(RISCV_ISA_EXT_SVADE);
+}
+
 /*
  * THP functions
  */
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 5ef48cb20ee1..58565798cea0 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -301,6 +301,8 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
 	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
 	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
 	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
+	__RISCV_ISA_EXT_DATA(svade, RISCV_ISA_EXT_SVADE),
+	__RISCV_ISA_EXT_DATA(svadu, RISCV_ISA_EXT_SVADU),
 	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
 	__RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT),
 	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
-- 
2.17.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Yong-Xuan Wang]

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.
+            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).
+
         - const: svinval
           description:
             The standard Svinval supervisor-level extension for fine-grained
-- 
2.17.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v5 3/4] RISC-V: KVM: Add Svade and Svadu Extensions Support for Guest/VM

[Yong-Xuan Wang]

We extend the KVM ISA extension ONE_REG interface to allow VMM tools to
detect and enable Svade and Svadu extensions for Guest/VM. Since the
henvcfg.ADUE is read-only zero if the menvcfg.ADUE is zero, the Svadu
extension is available for Guest/VM only when arch_has_hw_pte_young()
is true.

Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
---
 arch/riscv/include/uapi/asm/kvm.h | 2 ++
 arch/riscv/kvm/vcpu.c             | 6 ++++++
 arch/riscv/kvm/vcpu_onereg.c      | 6 ++++++
 3 files changed, 14 insertions(+)

diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
index e878e7cc3978..a5e0c35d7e9a 100644
--- a/arch/riscv/include/uapi/asm/kvm.h
+++ b/arch/riscv/include/uapi/asm/kvm.h
@@ -168,6 +168,8 @@ enum KVM_RISCV_ISA_EXT_ID {
 	KVM_RISCV_ISA_EXT_ZTSO,
 	KVM_RISCV_ISA_EXT_ZACAS,
 	KVM_RISCV_ISA_EXT_SSCOFPMF,
+	KVM_RISCV_ISA_EXT_SVADE,
+	KVM_RISCV_ISA_EXT_SVADU,
 	KVM_RISCV_ISA_EXT_MAX,
 };
 
diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
index 17e21df36cc1..21edd60c4756 100644
--- a/arch/riscv/kvm/vcpu.c
+++ b/arch/riscv/kvm/vcpu.c
@@ -540,6 +540,12 @@ static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
 	if (riscv_isa_extension_available(isa, ZICBOZ))
 		cfg->henvcfg |= ENVCFG_CBZE;
 
+	if (riscv_isa_extension_available(isa, SVADU))
+		cfg->henvcfg |= ENVCFG_ADUE;
+
+	if (riscv_isa_extension_available(isa, SVADE))
+		cfg->henvcfg &= ~ENVCFG_ADUE;
+
 	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SMSTATEEN)) {
 		cfg->hstateen0 |= SMSTATEEN0_HSENVCFG;
 		if (riscv_isa_extension_available(isa, SSAIA))
diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
index c676275ea0a0..06e930f1e206 100644
--- a/arch/riscv/kvm/vcpu_onereg.c
+++ b/arch/riscv/kvm/vcpu_onereg.c
@@ -15,6 +15,7 @@
 #include <asm/cacheflush.h>
 #include <asm/cpufeature.h>
 #include <asm/kvm_vcpu_vector.h>
+#include <asm/pgtable.h>
 #include <asm/vector.h>
 
 #define KVM_RISCV_BASE_ISA_MASK		GENMASK(25, 0)
@@ -38,6 +39,8 @@ static const unsigned long kvm_isa_ext_arr[] = {
 	KVM_ISA_EXT_ARR(SSAIA),
 	KVM_ISA_EXT_ARR(SSCOFPMF),
 	KVM_ISA_EXT_ARR(SSTC),
+	KVM_ISA_EXT_ARR(SVADE),
+	KVM_ISA_EXT_ARR(SVADU),
 	KVM_ISA_EXT_ARR(SVINVAL),
 	KVM_ISA_EXT_ARR(SVNAPOT),
 	KVM_ISA_EXT_ARR(SVPBMT),
@@ -105,6 +108,9 @@ static bool kvm_riscv_vcpu_isa_enable_allowed(unsigned long ext)
 		return __riscv_isa_extension_available(NULL, RISCV_ISA_EXT_SSAIA);
 	case KVM_RISCV_ISA_EXT_V:
 		return riscv_v_vstate_ctrl_user_allowed();
+	case KVM_RISCV_ISA_EXT_SVADU:
+		/* The henvcfg.ADUE is read-only zero if menvcfg.ADUE is zero. */
+		return arch_has_hw_pte_young();
 	default:
 		break;
 	}
-- 
2.17.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v5 4/4] KVM: riscv: selftests: Add Svade and Svadu Extension to get-reg-list test

[Yong-Xuan Wang]

Update the get-reg-list test to test the Svade and Svadu Extensions are
available for guest OS.

Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
---
 tools/testing/selftests/kvm/riscv/get-reg-list.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
index 222198dd6d04..1d32351ad55e 100644
--- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
+++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
@@ -45,6 +45,8 @@ bool filter_reg(__u64 reg)
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SSAIA:
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SSCOFPMF:
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SSTC:
+	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SVADE:
+	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SVADU:
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SVINVAL:
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SVNAPOT:
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SVPBMT:
@@ -411,6 +413,8 @@ static const char *isa_ext_single_id_to_str(__u64 reg_off)
 		KVM_ISA_EXT_ARR(SSAIA),
 		KVM_ISA_EXT_ARR(SSCOFPMF),
 		KVM_ISA_EXT_ARR(SSTC),
+		KVM_ISA_EXT_ARR(SVADE),
+		KVM_ISA_EXT_ARR(SVADU),
 		KVM_ISA_EXT_ARR(SVINVAL),
 		KVM_ISA_EXT_ARR(SVNAPOT),
 		KVM_ISA_EXT_ARR(SVPBMT),
@@ -935,6 +939,8 @@ KVM_ISA_EXT_SIMPLE_CONFIG(h, H);
 KVM_ISA_EXT_SUBLIST_CONFIG(smstateen, SMSTATEEN);
 KVM_ISA_EXT_SIMPLE_CONFIG(sscofpmf, SSCOFPMF);
 KVM_ISA_EXT_SIMPLE_CONFIG(sstc, SSTC);
+KVM_ISA_EXT_SIMPLE_CONFIG(svade, SVADE);
+KVM_ISA_EXT_SIMPLE_CONFIG(svadu, SVADU);
 KVM_ISA_EXT_SIMPLE_CONFIG(svinval, SVINVAL);
 KVM_ISA_EXT_SIMPLE_CONFIG(svnapot, SVNAPOT);
 KVM_ISA_EXT_SIMPLE_CONFIG(svpbmt, SVPBMT);
@@ -991,6 +997,8 @@ struct vcpu_reg_list *vcpu_configs[] = {
 	&config_smstateen,
 	&config_sscofpmf,
 	&config_sstc,
+	&config_svade,
+	&config_svadu,
 	&config_svinval,
 	&config_svnapot,
 	&config_svpbmt,
-- 
2.17.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 3392 bytes --]

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.


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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Yong-Xuan Wang]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 4133 bytes --]

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.


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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Anup Patel]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v5 1/4] RISC-V: Add Svade and Svadu Extensions Support

[Alexandre Ghiti]

Hi Yong-Xuan,

On 05/06/2024 14:15, Yong-Xuan Wang wrote:
> Svade and Svadu extensions represent two schemes for managing the PTE A/D
> bits. When the PTE A/D bits need to be set, Svade extension intdicates
> that a related page fault will be raised. In contrast, the Svadu extension
> supports hardware updating of PTE A/D bits. Since the Svade extension is
> mandatory and the Svadu extension is optional in RVA23 profile, by default
> the M-mode firmware will enable the Svadu extension in the menvcfg CSR
> when only Svadu is present in DT.
>
> This patch detects Svade and Svadu extensions from DT and adds
> arch_has_hw_pte_young() to enable optimization in MGLRU and
> __wp_page_copy_user() when we have the PTE A/D bits hardware updating
> support.
>
> Co-developed-by: Jinyu Tang <tjytimi@163.com>
> Signed-off-by: Jinyu Tang <tjytimi@163.com>
> Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> ---
>   arch/riscv/Kconfig               |  1 +
>   arch/riscv/include/asm/csr.h     |  1 +
>   arch/riscv/include/asm/hwcap.h   |  2 ++
>   arch/riscv/include/asm/pgtable.h | 14 +++++++++++++-
>   arch/riscv/kernel/cpufeature.c   |  2 ++
>   5 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index b94176e25be1..dbfe2be99bf9 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -36,6 +36,7 @@ config RISCV
>   	select ARCH_HAS_PMEM_API
>   	select ARCH_HAS_PREPARE_SYNC_CORE_CMD
>   	select ARCH_HAS_PTE_SPECIAL
> +	select ARCH_HAS_HW_PTE_YOUNG
>   	select ARCH_HAS_SET_DIRECT_MAP if MMU
>   	select ARCH_HAS_SET_MEMORY if MMU
>   	select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
> diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
> index 25966995da04..524cd4131c71 100644
> --- a/arch/riscv/include/asm/csr.h
> +++ b/arch/riscv/include/asm/csr.h
> @@ -195,6 +195,7 @@
>   /* xENVCFG flags */
>   #define ENVCFG_STCE			(_AC(1, ULL) << 63)
>   #define ENVCFG_PBMTE			(_AC(1, ULL) << 62)
> +#define ENVCFG_ADUE			(_AC(1, ULL) << 61)
>   #define ENVCFG_CBZE			(_AC(1, UL) << 7)
>   #define ENVCFG_CBCFE			(_AC(1, UL) << 6)
>   #define ENVCFG_CBIE_SHIFT		4
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index e17d0078a651..35d7aa49785d 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -81,6 +81,8 @@
>   #define RISCV_ISA_EXT_ZTSO		72
>   #define RISCV_ISA_EXT_ZACAS		73
>   #define RISCV_ISA_EXT_XANDESPMU		74
> +#define RISCV_ISA_EXT_SVADE             75
> +#define RISCV_ISA_EXT_SVADU		76
>   
>   #define RISCV_ISA_EXT_XLINUXENVCFG	127
>   
> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index aad8b8ca51f1..7287ea4a6160 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -120,6 +120,7 @@
>   #include <asm/tlbflush.h>
>   #include <linux/mm_types.h>
>   #include <asm/compat.h>
> +#include <asm/cpufeature.h>
>   
>   #define __page_val_to_pfn(_val)  (((_val) & _PAGE_PFN_MASK) >> _PAGE_PFN_SHIFT)
>   
> @@ -288,7 +289,6 @@ static inline pte_t pud_pte(pud_t pud)
>   }
>   
>   #ifdef CONFIG_RISCV_ISA_SVNAPOT
> -#include <asm/cpufeature.h>
>   
>   static __always_inline bool has_svnapot(void)
>   {
> @@ -624,6 +624,18 @@ static inline pgprot_t pgprot_writecombine(pgprot_t _prot)
>   	return __pgprot(prot);
>   }
>   
> +/*
> + * Both Svade and Svadu control the hardware behavior when the PTE A/D bits need to be set. By
> + * default the M-mode firmware enables the hardware updating scheme when only Svadu is present in
> + * DT.
> + */
> +#define arch_has_hw_pte_young arch_has_hw_pte_young
> +static inline bool arch_has_hw_pte_young(void)
> +{
> +	return riscv_has_extension_unlikely(RISCV_ISA_EXT_SVADU) &&
> +	       !riscv_has_extension_likely(RISCV_ISA_EXT_SVADE);
> +}


AFAIK, the riscv_has_extension_*() macros will use the content of the 
riscv,isa string. So this works fine for now with the description you 
provided in the cover letter, as long as we don't have the FWFT SBI 
extension. I'm wondering if we should not make sure it works when FWFT 
lands because I'm pretty sure we will forget about this.

So I think we should check the availability of SBI FWFT and use some 
global variable that tells if svadu is enabled or not instead of this check.


> +
>   /*
>    * THP functions
>    */
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 5ef48cb20ee1..58565798cea0 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -301,6 +301,8 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>   	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
>   	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>   	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> +	__RISCV_ISA_EXT_DATA(svade, RISCV_ISA_EXT_SVADE),
> +	__RISCV_ISA_EXT_DATA(svadu, RISCV_ISA_EXT_SVADU),
>   	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
>   	__RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT),
>   	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Alexandre Ghiti]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v5 1/4] RISC-V: Add Svade and Svadu Extensions Support

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1505 bytes --]

On Fri, Jun 21, 2024 at 09:52:01AM +0200, Alexandre Ghiti wrote:
> On 05/06/2024 14:15, Yong-Xuan Wang wrote:
> > Svade and Svadu extensions represent two schemes for managing the PTE A/D
> > +/*
> > + * Both Svade and Svadu control the hardware behavior when the PTE A/D bits need to be set. By
> > + * default the M-mode firmware enables the hardware updating scheme when only Svadu is present in
> > + * DT.
> > + */
> > +#define arch_has_hw_pte_young arch_has_hw_pte_young
> > +static inline bool arch_has_hw_pte_young(void)
> > +{
> > +	return riscv_has_extension_unlikely(RISCV_ISA_EXT_SVADU) &&
> > +	       !riscv_has_extension_likely(RISCV_ISA_EXT_SVADE);
> > +}
> 
> 
> AFAIK, the riscv_has_extension_*() macros will use the content of the
> riscv,isa string. So this works fine for now with the description you
> provided in the cover letter, as long as we don't have the FWFT SBI
> extension. I'm wondering if we should not make sure it works when FWFT lands
> because I'm pretty sure we will forget about this.
> 
> So I think we should check the availability of SBI FWFT and use some global
> variable that tells if svadu is enabled or not instead of this check.

No. If FWFT stuff arrives, it should be plumbed into exactly the same
interface. "Users" should not have to think about where the extension is
probed from and be able to use the same interface regardless.

The interfaces we have have already caused some confusion, we should not
make them worse.

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v5 1/4] RISC-V: Add Svade and Svadu Extensions Support

[Alexandre Ghiti]

Hi Conor,

On 21/06/2024 10:01, Conor Dooley wrote:
> On Fri, Jun 21, 2024 at 09:52:01AM +0200, Alexandre Ghiti wrote:
>> On 05/06/2024 14:15, Yong-Xuan Wang wrote:
>>> Svade and Svadu extensions represent two schemes for managing the PTE A/D
>>> +/*
>>> + * Both Svade and Svadu control the hardware behavior when the PTE A/D bits need to be set. By
>>> + * default the M-mode firmware enables the hardware updating scheme when only Svadu is present in
>>> + * DT.
>>> + */
>>> +#define arch_has_hw_pte_young arch_has_hw_pte_young
>>> +static inline bool arch_has_hw_pte_young(void)
>>> +{
>>> +	return riscv_has_extension_unlikely(RISCV_ISA_EXT_SVADU) &&
>>> +	       !riscv_has_extension_likely(RISCV_ISA_EXT_SVADE);
>>> +}
>>
>> AFAIK, the riscv_has_extension_*() macros will use the content of the
>> riscv,isa string. So this works fine for now with the description you
>> provided in the cover letter, as long as we don't have the FWFT SBI
>> extension. I'm wondering if we should not make sure it works when FWFT lands
>> because I'm pretty sure we will forget about this.
>>
>> So I think we should check the availability of SBI FWFT and use some global
>> variable that tells if svadu is enabled or not instead of this check.
> No. If FWFT stuff arrives, it should be plumbed into exactly the same
> interface. "Users" should not have to think about where the extension is
> probed from and be able to use the same interface regardless.
>
> The interfaces we have have already caused some confusion, we should not
> make them worse.


Understood, we need to keep that in mind then.


>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Andrew Jones]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Alexandre Ghiti]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v5 1/4] RISC-V: Add Svade and Svadu Extensions Support

[Andrew Jones]

On Wed, Jun 05, 2024 at 08:15:07PM GMT, Yong-Xuan Wang wrote:
> Svade and Svadu extensions represent two schemes for managing the PTE A/D
> bits. When the PTE A/D bits need to be set, Svade extension intdicates
> that a related page fault will be raised. In contrast, the Svadu extension
> supports hardware updating of PTE A/D bits. Since the Svade extension is
> mandatory and the Svadu extension is optional in RVA23 profile, by default
> the M-mode firmware will enable the Svadu extension in the menvcfg CSR
> when only Svadu is present in DT.
> 
> This patch detects Svade and Svadu extensions from DT and adds
> arch_has_hw_pte_young() to enable optimization in MGLRU and
> __wp_page_copy_user() when we have the PTE A/D bits hardware updating
> support.
> 
> Co-developed-by: Jinyu Tang <tjytimi@163.com>
> Signed-off-by: Jinyu Tang <tjytimi@163.com>
> Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> ---
>  arch/riscv/Kconfig               |  1 +
>  arch/riscv/include/asm/csr.h     |  1 +
>  arch/riscv/include/asm/hwcap.h   |  2 ++
>  arch/riscv/include/asm/pgtable.h | 14 +++++++++++++-
>  arch/riscv/kernel/cpufeature.c   |  2 ++
>  5 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index b94176e25be1..dbfe2be99bf9 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -36,6 +36,7 @@ config RISCV
>  	select ARCH_HAS_PMEM_API
>  	select ARCH_HAS_PREPARE_SYNC_CORE_CMD
>  	select ARCH_HAS_PTE_SPECIAL
> +	select ARCH_HAS_HW_PTE_YOUNG
>  	select ARCH_HAS_SET_DIRECT_MAP if MMU
>  	select ARCH_HAS_SET_MEMORY if MMU
>  	select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
> diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
> index 25966995da04..524cd4131c71 100644
> --- a/arch/riscv/include/asm/csr.h
> +++ b/arch/riscv/include/asm/csr.h
> @@ -195,6 +195,7 @@
>  /* xENVCFG flags */
>  #define ENVCFG_STCE			(_AC(1, ULL) << 63)
>  #define ENVCFG_PBMTE			(_AC(1, ULL) << 62)
> +#define ENVCFG_ADUE			(_AC(1, ULL) << 61)
>  #define ENVCFG_CBZE			(_AC(1, UL) << 7)
>  #define ENVCFG_CBCFE			(_AC(1, UL) << 6)
>  #define ENVCFG_CBIE_SHIFT		4
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index e17d0078a651..35d7aa49785d 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -81,6 +81,8 @@
>  #define RISCV_ISA_EXT_ZTSO		72
>  #define RISCV_ISA_EXT_ZACAS		73
>  #define RISCV_ISA_EXT_XANDESPMU		74
> +#define RISCV_ISA_EXT_SVADE             75
> +#define RISCV_ISA_EXT_SVADU		76
>  
>  #define RISCV_ISA_EXT_XLINUXENVCFG	127
>  
> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index aad8b8ca51f1..7287ea4a6160 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -120,6 +120,7 @@
>  #include <asm/tlbflush.h>
>  #include <linux/mm_types.h>
>  #include <asm/compat.h>
> +#include <asm/cpufeature.h>
>  
>  #define __page_val_to_pfn(_val)  (((_val) & _PAGE_PFN_MASK) >> _PAGE_PFN_SHIFT)
>  
> @@ -288,7 +289,6 @@ static inline pte_t pud_pte(pud_t pud)
>  }
>  
>  #ifdef CONFIG_RISCV_ISA_SVNAPOT
> -#include <asm/cpufeature.h>
>  
>  static __always_inline bool has_svnapot(void)
>  {
> @@ -624,6 +624,18 @@ static inline pgprot_t pgprot_writecombine(pgprot_t _prot)
>  	return __pgprot(prot);
>  }
>  
> +/*
> + * Both Svade and Svadu control the hardware behavior when the PTE A/D bits need to be set. By
> + * default the M-mode firmware enables the hardware updating scheme when only Svadu is present in
> + * DT.
> + */
> +#define arch_has_hw_pte_young arch_has_hw_pte_young
> +static inline bool arch_has_hw_pte_young(void)
> +{
> +	return riscv_has_extension_unlikely(RISCV_ISA_EXT_SVADU) &&
> +	       !riscv_has_extension_likely(RISCV_ISA_EXT_SVADE);

It's hard to guess what is, or will be, more likely to be the correct
choice of call between the _unlikely and _likely variants. But, while we
assume svade is most prevalent right now, it's actually quite unlikely
that 'svade' will be in the DT, since DTs haven't been putting it there
yet. Anyway, it doesn't really matter much and maybe the _unlikely vs.
_likely variants are better for documenting expectations than for
performance.

> +}
> +
>  /*
>   * THP functions
>   */
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 5ef48cb20ee1..58565798cea0 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -301,6 +301,8 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>  	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
>  	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>  	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> +	__RISCV_ISA_EXT_DATA(svade, RISCV_ISA_EXT_SVADE),
> +	__RISCV_ISA_EXT_DATA(svadu, RISCV_ISA_EXT_SVADU),
>  	__RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
>  	__RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT),
>  	__RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
> -- 
> 2.17.1
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v5 3/4] RISC-V: KVM: Add Svade and Svadu Extensions Support for Guest/VM

[Andrew Jones]

On Wed, Jun 05, 2024 at 08:15:09PM GMT, Yong-Xuan Wang wrote:
> We extend the KVM ISA extension ONE_REG interface to allow VMM tools to
> detect and enable Svade and Svadu extensions for Guest/VM. Since the
> henvcfg.ADUE is read-only zero if the menvcfg.ADUE is zero, the Svadu
> extension is available for Guest/VM only when arch_has_hw_pte_young()
> is true.
> 
> Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> ---
>  arch/riscv/include/uapi/asm/kvm.h | 2 ++
>  arch/riscv/kvm/vcpu.c             | 6 ++++++
>  arch/riscv/kvm/vcpu_onereg.c      | 6 ++++++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> index e878e7cc3978..a5e0c35d7e9a 100644
> --- a/arch/riscv/include/uapi/asm/kvm.h
> +++ b/arch/riscv/include/uapi/asm/kvm.h
> @@ -168,6 +168,8 @@ enum KVM_RISCV_ISA_EXT_ID {
>  	KVM_RISCV_ISA_EXT_ZTSO,
>  	KVM_RISCV_ISA_EXT_ZACAS,
>  	KVM_RISCV_ISA_EXT_SSCOFPMF,
> +	KVM_RISCV_ISA_EXT_SVADE,
> +	KVM_RISCV_ISA_EXT_SVADU,
>  	KVM_RISCV_ISA_EXT_MAX,
>  };
>  
> diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> index 17e21df36cc1..21edd60c4756 100644
> --- a/arch/riscv/kvm/vcpu.c
> +++ b/arch/riscv/kvm/vcpu.c
> @@ -540,6 +540,12 @@ static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
>  	if (riscv_isa_extension_available(isa, ZICBOZ))
>  		cfg->henvcfg |= ENVCFG_CBZE;
>  
> +	if (riscv_isa_extension_available(isa, SVADU))
> +		cfg->henvcfg |= ENVCFG_ADUE;
> +
> +	if (riscv_isa_extension_available(isa, SVADE))
> +		cfg->henvcfg &= ~ENVCFG_ADUE;

nit: I'd write this as

	if (!riscv_isa_extension_available(isa, SVADE) &&
	    riscv_isa_extension_available(isa, SVADU))
		cfg->henvcfg |= ENVCFG_ADUE;

> +
>  	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SMSTATEEN)) {
>  		cfg->hstateen0 |= SMSTATEEN0_HSENVCFG;
>  		if (riscv_isa_extension_available(isa, SSAIA))
> diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
> index c676275ea0a0..06e930f1e206 100644
> --- a/arch/riscv/kvm/vcpu_onereg.c
> +++ b/arch/riscv/kvm/vcpu_onereg.c
> @@ -15,6 +15,7 @@
>  #include <asm/cacheflush.h>
>  #include <asm/cpufeature.h>
>  #include <asm/kvm_vcpu_vector.h>
> +#include <asm/pgtable.h>
>  #include <asm/vector.h>
>  
>  #define KVM_RISCV_BASE_ISA_MASK		GENMASK(25, 0)
> @@ -38,6 +39,8 @@ static const unsigned long kvm_isa_ext_arr[] = {
>  	KVM_ISA_EXT_ARR(SSAIA),
>  	KVM_ISA_EXT_ARR(SSCOFPMF),
>  	KVM_ISA_EXT_ARR(SSTC),
> +	KVM_ISA_EXT_ARR(SVADE),
> +	KVM_ISA_EXT_ARR(SVADU),
>  	KVM_ISA_EXT_ARR(SVINVAL),
>  	KVM_ISA_EXT_ARR(SVNAPOT),
>  	KVM_ISA_EXT_ARR(SVPBMT),
> @@ -105,6 +108,9 @@ static bool kvm_riscv_vcpu_isa_enable_allowed(unsigned long ext)
>  		return __riscv_isa_extension_available(NULL, RISCV_ISA_EXT_SSAIA);
>  	case KVM_RISCV_ISA_EXT_V:
>  		return riscv_v_vstate_ctrl_user_allowed();
> +	case KVM_RISCV_ISA_EXT_SVADU:
> +		/* The henvcfg.ADUE is read-only zero if menvcfg.ADUE is zero. */
> +		return arch_has_hw_pte_young();
>  	default:
>  		break;
>  	}
> -- 
> 2.17.1
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v5 4/4] KVM: riscv: selftests: Add Svade and Svadu Extension to get-reg-list test

[Andrew Jones]

On Wed, Jun 05, 2024 at 08:15:10PM GMT, Yong-Xuan Wang wrote:
> Update the get-reg-list test to test the Svade and Svadu Extensions are
> available for guest OS.
> 
> Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> ---
>  tools/testing/selftests/kvm/riscv/get-reg-list.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> index 222198dd6d04..1d32351ad55e 100644
> --- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
> +++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
> @@ -45,6 +45,8 @@ bool filter_reg(__u64 reg)
>  	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SSAIA:
>  	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SSCOFPMF:
>  	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SSTC:
> +	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SVADE:
> +	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SVADU:
>  	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SVINVAL:
>  	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SVNAPOT:
>  	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SVPBMT:
> @@ -411,6 +413,8 @@ static const char *isa_ext_single_id_to_str(__u64 reg_off)
>  		KVM_ISA_EXT_ARR(SSAIA),
>  		KVM_ISA_EXT_ARR(SSCOFPMF),
>  		KVM_ISA_EXT_ARR(SSTC),
> +		KVM_ISA_EXT_ARR(SVADE),
> +		KVM_ISA_EXT_ARR(SVADU),
>  		KVM_ISA_EXT_ARR(SVINVAL),
>  		KVM_ISA_EXT_ARR(SVNAPOT),
>  		KVM_ISA_EXT_ARR(SVPBMT),
> @@ -935,6 +939,8 @@ KVM_ISA_EXT_SIMPLE_CONFIG(h, H);
>  KVM_ISA_EXT_SUBLIST_CONFIG(smstateen, SMSTATEEN);
>  KVM_ISA_EXT_SIMPLE_CONFIG(sscofpmf, SSCOFPMF);
>  KVM_ISA_EXT_SIMPLE_CONFIG(sstc, SSTC);
> +KVM_ISA_EXT_SIMPLE_CONFIG(svade, SVADE);
> +KVM_ISA_EXT_SIMPLE_CONFIG(svadu, SVADU);
>  KVM_ISA_EXT_SIMPLE_CONFIG(svinval, SVINVAL);
>  KVM_ISA_EXT_SIMPLE_CONFIG(svnapot, SVNAPOT);
>  KVM_ISA_EXT_SIMPLE_CONFIG(svpbmt, SVPBMT);
> @@ -991,6 +997,8 @@ struct vcpu_reg_list *vcpu_configs[] = {
>  	&config_smstateen,
>  	&config_sscofpmf,
>  	&config_sstc,
> +	&config_svade,
> +	&config_svadu,
>  	&config_svinval,
>  	&config_svnapot,
>  	&config_svpbmt,
> -- 
> 2.17.1
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 2320 bytes --]

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.

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 4849 bytes --]

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.

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v5 1/4] RISC-V: Add Svade and Svadu Extensions Support

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 5278 bytes --]

On Fri, Jun 21, 2024 at 10:43:58AM +0200, Andrew Jones wrote:
> On Wed, Jun 05, 2024 at 08:15:07PM GMT, Yong-Xuan Wang wrote:
> > Svade and Svadu extensions represent two schemes for managing the PTE A/D
> > bits. When the PTE A/D bits need to be set, Svade extension intdicates
> > that a related page fault will be raised. In contrast, the Svadu extension
> > supports hardware updating of PTE A/D bits. Since the Svade extension is
> > mandatory and the Svadu extension is optional in RVA23 profile, by default
> > the M-mode firmware will enable the Svadu extension in the menvcfg CSR
> > when only Svadu is present in DT.
> > 
> > This patch detects Svade and Svadu extensions from DT and adds
> > arch_has_hw_pte_young() to enable optimization in MGLRU and
> > __wp_page_copy_user() when we have the PTE A/D bits hardware updating
> > support.
> > 
> > Co-developed-by: Jinyu Tang <tjytimi@163.com>
> > Signed-off-by: Jinyu Tang <tjytimi@163.com>
> > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> > ---
> >  arch/riscv/Kconfig               |  1 +
> >  arch/riscv/include/asm/csr.h     |  1 +
> >  arch/riscv/include/asm/hwcap.h   |  2 ++
> >  arch/riscv/include/asm/pgtable.h | 14 +++++++++++++-
> >  arch/riscv/kernel/cpufeature.c   |  2 ++
> >  5 files changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index b94176e25be1..dbfe2be99bf9 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -36,6 +36,7 @@ config RISCV
> >  	select ARCH_HAS_PMEM_API
> >  	select ARCH_HAS_PREPARE_SYNC_CORE_CMD
> >  	select ARCH_HAS_PTE_SPECIAL
> > +	select ARCH_HAS_HW_PTE_YOUNG
> >  	select ARCH_HAS_SET_DIRECT_MAP if MMU
> >  	select ARCH_HAS_SET_MEMORY if MMU
> >  	select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
> > diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
> > index 25966995da04..524cd4131c71 100644
> > --- a/arch/riscv/include/asm/csr.h
> > +++ b/arch/riscv/include/asm/csr.h
> > @@ -195,6 +195,7 @@
> >  /* xENVCFG flags */
> >  #define ENVCFG_STCE			(_AC(1, ULL) << 63)
> >  #define ENVCFG_PBMTE			(_AC(1, ULL) << 62)
> > +#define ENVCFG_ADUE			(_AC(1, ULL) << 61)
> >  #define ENVCFG_CBZE			(_AC(1, UL) << 7)
> >  #define ENVCFG_CBCFE			(_AC(1, UL) << 6)
> >  #define ENVCFG_CBIE_SHIFT		4
> > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > index e17d0078a651..35d7aa49785d 100644
> > --- a/arch/riscv/include/asm/hwcap.h
> > +++ b/arch/riscv/include/asm/hwcap.h
> > @@ -81,6 +81,8 @@
> >  #define RISCV_ISA_EXT_ZTSO		72
> >  #define RISCV_ISA_EXT_ZACAS		73
> >  #define RISCV_ISA_EXT_XANDESPMU		74
> > +#define RISCV_ISA_EXT_SVADE             75
> > +#define RISCV_ISA_EXT_SVADU		76
> >  
> >  #define RISCV_ISA_EXT_XLINUXENVCFG	127
> >  
> > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > index aad8b8ca51f1..7287ea4a6160 100644
> > --- a/arch/riscv/include/asm/pgtable.h
> > +++ b/arch/riscv/include/asm/pgtable.h
> > @@ -120,6 +120,7 @@
> >  #include <asm/tlbflush.h>
> >  #include <linux/mm_types.h>
> >  #include <asm/compat.h>
> > +#include <asm/cpufeature.h>
> >  
> >  #define __page_val_to_pfn(_val)  (((_val) & _PAGE_PFN_MASK) >> _PAGE_PFN_SHIFT)
> >  
> > @@ -288,7 +289,6 @@ static inline pte_t pud_pte(pud_t pud)
> >  }
> >  
> >  #ifdef CONFIG_RISCV_ISA_SVNAPOT
> > -#include <asm/cpufeature.h>
> >  
> >  static __always_inline bool has_svnapot(void)
> >  {
> > @@ -624,6 +624,18 @@ static inline pgprot_t pgprot_writecombine(pgprot_t _prot)
> >  	return __pgprot(prot);
> >  }
> >  
> > +/*
> > + * Both Svade and Svadu control the hardware behavior when the PTE A/D bits need to be set. By
> > + * default the M-mode firmware enables the hardware updating scheme when only Svadu is present in
> > + * DT.
> > + */
> > +#define arch_has_hw_pte_young arch_has_hw_pte_young
> > +static inline bool arch_has_hw_pte_young(void)
> > +{
> > +	return riscv_has_extension_unlikely(RISCV_ISA_EXT_SVADU) &&
> > +	       !riscv_has_extension_likely(RISCV_ISA_EXT_SVADE);
> 
> It's hard to guess what is, or will be, more likely to be the correct
> choice of call between the _unlikely and _likely variants. But, while we
> assume svade is most prevalent right now, it's actually quite unlikely
> that 'svade' will be in the DT, since DTs haven't been putting it there
> yet. Anyway, it doesn't really matter much and maybe the _unlikely vs.
> _likely variants are better for documenting expectations than for
> performance.

binding hat off, and kernel hat on, what do we actually do if there's
neither Svadu or Svade in the firmware's description of the hardware?
Do we just arbitrarily turn on Svade, like we already do for some
extensions:
	/*
	 * These ones were as they were part of the base ISA when the
	 * port & dt-bindings were upstreamed, and so can be set
	 * unconditionally where `i` is in riscv,isa on DT systems.
	 */
	if (acpi_disabled) {
		set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa);
		set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa);
		set_bit(RISCV_ISA_EXT_ZICNTR, isainfo->isa);
		set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
	}


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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v5 1/4] RISC-V: Add Svade and Svadu Extensions Support

[Alexandre Ghiti]

On 21/06/2024 12:24, Conor Dooley wrote:
> On Fri, Jun 21, 2024 at 10:43:58AM +0200, Andrew Jones wrote:
>> On Wed, Jun 05, 2024 at 08:15:07PM GMT, Yong-Xuan Wang wrote:
>>> Svade and Svadu extensions represent two schemes for managing the PTE A/D
>>> bits. When the PTE A/D bits need to be set, Svade extension intdicates
>>> that a related page fault will be raised. In contrast, the Svadu extension
>>> supports hardware updating of PTE A/D bits. Since the Svade extension is
>>> mandatory and the Svadu extension is optional in RVA23 profile, by default
>>> the M-mode firmware will enable the Svadu extension in the menvcfg CSR
>>> when only Svadu is present in DT.
>>>
>>> This patch detects Svade and Svadu extensions from DT and adds
>>> arch_has_hw_pte_young() to enable optimization in MGLRU and
>>> __wp_page_copy_user() when we have the PTE A/D bits hardware updating
>>> support.
>>>
>>> Co-developed-by: Jinyu Tang <tjytimi@163.com>
>>> Signed-off-by: Jinyu Tang <tjytimi@163.com>
>>> Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
>>> ---
>>>   arch/riscv/Kconfig               |  1 +
>>>   arch/riscv/include/asm/csr.h     |  1 +
>>>   arch/riscv/include/asm/hwcap.h   |  2 ++
>>>   arch/riscv/include/asm/pgtable.h | 14 +++++++++++++-
>>>   arch/riscv/kernel/cpufeature.c   |  2 ++
>>>   5 files changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>>> index b94176e25be1..dbfe2be99bf9 100644
>>> --- a/arch/riscv/Kconfig
>>> +++ b/arch/riscv/Kconfig
>>> @@ -36,6 +36,7 @@ config RISCV
>>>   	select ARCH_HAS_PMEM_API
>>>   	select ARCH_HAS_PREPARE_SYNC_CORE_CMD
>>>   	select ARCH_HAS_PTE_SPECIAL
>>> +	select ARCH_HAS_HW_PTE_YOUNG
>>>   	select ARCH_HAS_SET_DIRECT_MAP if MMU
>>>   	select ARCH_HAS_SET_MEMORY if MMU
>>>   	select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
>>> diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
>>> index 25966995da04..524cd4131c71 100644
>>> --- a/arch/riscv/include/asm/csr.h
>>> +++ b/arch/riscv/include/asm/csr.h
>>> @@ -195,6 +195,7 @@
>>>   /* xENVCFG flags */
>>>   #define ENVCFG_STCE			(_AC(1, ULL) << 63)
>>>   #define ENVCFG_PBMTE			(_AC(1, ULL) << 62)
>>> +#define ENVCFG_ADUE			(_AC(1, ULL) << 61)
>>>   #define ENVCFG_CBZE			(_AC(1, UL) << 7)
>>>   #define ENVCFG_CBCFE			(_AC(1, UL) << 6)
>>>   #define ENVCFG_CBIE_SHIFT		4
>>> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
>>> index e17d0078a651..35d7aa49785d 100644
>>> --- a/arch/riscv/include/asm/hwcap.h
>>> +++ b/arch/riscv/include/asm/hwcap.h
>>> @@ -81,6 +81,8 @@
>>>   #define RISCV_ISA_EXT_ZTSO		72
>>>   #define RISCV_ISA_EXT_ZACAS		73
>>>   #define RISCV_ISA_EXT_XANDESPMU		74
>>> +#define RISCV_ISA_EXT_SVADE             75
>>> +#define RISCV_ISA_EXT_SVADU		76
>>>   
>>>   #define RISCV_ISA_EXT_XLINUXENVCFG	127
>>>   
>>> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
>>> index aad8b8ca51f1..7287ea4a6160 100644
>>> --- a/arch/riscv/include/asm/pgtable.h
>>> +++ b/arch/riscv/include/asm/pgtable.h
>>> @@ -120,6 +120,7 @@
>>>   #include <asm/tlbflush.h>
>>>   #include <linux/mm_types.h>
>>>   #include <asm/compat.h>
>>> +#include <asm/cpufeature.h>
>>>   
>>>   #define __page_val_to_pfn(_val)  (((_val) & _PAGE_PFN_MASK) >> _PAGE_PFN_SHIFT)
>>>   
>>> @@ -288,7 +289,6 @@ static inline pte_t pud_pte(pud_t pud)
>>>   }
>>>   
>>>   #ifdef CONFIG_RISCV_ISA_SVNAPOT
>>> -#include <asm/cpufeature.h>
>>>   
>>>   static __always_inline bool has_svnapot(void)
>>>   {
>>> @@ -624,6 +624,18 @@ static inline pgprot_t pgprot_writecombine(pgprot_t _prot)
>>>   	return __pgprot(prot);
>>>   }
>>>   
>>> +/*
>>> + * Both Svade and Svadu control the hardware behavior when the PTE A/D bits need to be set. By
>>> + * default the M-mode firmware enables the hardware updating scheme when only Svadu is present in
>>> + * DT.
>>> + */
>>> +#define arch_has_hw_pte_young arch_has_hw_pte_young
>>> +static inline bool arch_has_hw_pte_young(void)
>>> +{
>>> +	return riscv_has_extension_unlikely(RISCV_ISA_EXT_SVADU) &&
>>> +	       !riscv_has_extension_likely(RISCV_ISA_EXT_SVADE);
>> It's hard to guess what is, or will be, more likely to be the correct
>> choice of call between the _unlikely and _likely variants. But, while we
>> assume svade is most prevalent right now, it's actually quite unlikely
>> that 'svade' will be in the DT, since DTs haven't been putting it there
>> yet. Anyway, it doesn't really matter much and maybe the _unlikely vs.
>> _likely variants are better for documenting expectations than for
>> performance.
> binding hat off, and kernel hat on, what do we actually do if there's
> neither Svadu or Svade in the firmware's description of the hardware?
> Do we just arbitrarily turn on Svade, like we already do for some
> extensions:
> 	/*
> 	 * These ones were as they were part of the base ISA when the
> 	 * port & dt-bindings were upstreamed, and so can be set
> 	 * unconditionally where `i` is in riscv,isa on DT systems.
> 	 */
> 	if (acpi_disabled) {
> 		set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa);
> 		set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa);
> 		set_bit(RISCV_ISA_EXT_ZICNTR, isainfo->isa);
> 		set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
> 	}


I'd say yes, svade just put a name on a HW mechanism that is required to 
make an OS work properly (if Svadu is not present). So if a platform 
only supports Svadu and it's not in the device tree, that's a bug on 
their hand.

So if neither Svadu nor Svade are present in the device tree, we can 
legitimately assume that Svade is enabled.


>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v5 1/4] RISC-V: Add Svade and Svadu Extensions Support

[Andrew Jones]

On Fri, Jun 21, 2024 at 11:24:19AM GMT, Conor Dooley wrote:
> On Fri, Jun 21, 2024 at 10:43:58AM +0200, Andrew Jones wrote:
> > On Wed, Jun 05, 2024 at 08:15:07PM GMT, Yong-Xuan Wang wrote:
> > > Svade and Svadu extensions represent two schemes for managing the PTE A/D
> > > bits. When the PTE A/D bits need to be set, Svade extension intdicates
> > > that a related page fault will be raised. In contrast, the Svadu extension
> > > supports hardware updating of PTE A/D bits. Since the Svade extension is
> > > mandatory and the Svadu extension is optional in RVA23 profile, by default
> > > the M-mode firmware will enable the Svadu extension in the menvcfg CSR
> > > when only Svadu is present in DT.
> > > 
> > > This patch detects Svade and Svadu extensions from DT and adds
> > > arch_has_hw_pte_young() to enable optimization in MGLRU and
> > > __wp_page_copy_user() when we have the PTE A/D bits hardware updating
> > > support.
> > > 
> > > Co-developed-by: Jinyu Tang <tjytimi@163.com>
> > > Signed-off-by: Jinyu Tang <tjytimi@163.com>
> > > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> > > ---
> > >  arch/riscv/Kconfig               |  1 +
> > >  arch/riscv/include/asm/csr.h     |  1 +
> > >  arch/riscv/include/asm/hwcap.h   |  2 ++
> > >  arch/riscv/include/asm/pgtable.h | 14 +++++++++++++-
> > >  arch/riscv/kernel/cpufeature.c   |  2 ++
> > >  5 files changed, 19 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > index b94176e25be1..dbfe2be99bf9 100644
> > > --- a/arch/riscv/Kconfig
> > > +++ b/arch/riscv/Kconfig
> > > @@ -36,6 +36,7 @@ config RISCV
> > >  	select ARCH_HAS_PMEM_API
> > >  	select ARCH_HAS_PREPARE_SYNC_CORE_CMD
> > >  	select ARCH_HAS_PTE_SPECIAL
> > > +	select ARCH_HAS_HW_PTE_YOUNG
> > >  	select ARCH_HAS_SET_DIRECT_MAP if MMU
> > >  	select ARCH_HAS_SET_MEMORY if MMU
> > >  	select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
> > > diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
> > > index 25966995da04..524cd4131c71 100644
> > > --- a/arch/riscv/include/asm/csr.h
> > > +++ b/arch/riscv/include/asm/csr.h
> > > @@ -195,6 +195,7 @@
> > >  /* xENVCFG flags */
> > >  #define ENVCFG_STCE			(_AC(1, ULL) << 63)
> > >  #define ENVCFG_PBMTE			(_AC(1, ULL) << 62)
> > > +#define ENVCFG_ADUE			(_AC(1, ULL) << 61)
> > >  #define ENVCFG_CBZE			(_AC(1, UL) << 7)
> > >  #define ENVCFG_CBCFE			(_AC(1, UL) << 6)
> > >  #define ENVCFG_CBIE_SHIFT		4
> > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > > index e17d0078a651..35d7aa49785d 100644
> > > --- a/arch/riscv/include/asm/hwcap.h
> > > +++ b/arch/riscv/include/asm/hwcap.h
> > > @@ -81,6 +81,8 @@
> > >  #define RISCV_ISA_EXT_ZTSO		72
> > >  #define RISCV_ISA_EXT_ZACAS		73
> > >  #define RISCV_ISA_EXT_XANDESPMU		74
> > > +#define RISCV_ISA_EXT_SVADE             75
> > > +#define RISCV_ISA_EXT_SVADU		76
> > >  
> > >  #define RISCV_ISA_EXT_XLINUXENVCFG	127
> > >  
> > > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > > index aad8b8ca51f1..7287ea4a6160 100644
> > > --- a/arch/riscv/include/asm/pgtable.h
> > > +++ b/arch/riscv/include/asm/pgtable.h
> > > @@ -120,6 +120,7 @@
> > >  #include <asm/tlbflush.h>
> > >  #include <linux/mm_types.h>
> > >  #include <asm/compat.h>
> > > +#include <asm/cpufeature.h>
> > >  
> > >  #define __page_val_to_pfn(_val)  (((_val) & _PAGE_PFN_MASK) >> _PAGE_PFN_SHIFT)
> > >  
> > > @@ -288,7 +289,6 @@ static inline pte_t pud_pte(pud_t pud)
> > >  }
> > >  
> > >  #ifdef CONFIG_RISCV_ISA_SVNAPOT
> > > -#include <asm/cpufeature.h>
> > >  
> > >  static __always_inline bool has_svnapot(void)
> > >  {
> > > @@ -624,6 +624,18 @@ static inline pgprot_t pgprot_writecombine(pgprot_t _prot)
> > >  	return __pgprot(prot);
> > >  }
> > >  
> > > +/*
> > > + * Both Svade and Svadu control the hardware behavior when the PTE A/D bits need to be set. By
> > > + * default the M-mode firmware enables the hardware updating scheme when only Svadu is present in
> > > + * DT.
> > > + */
> > > +#define arch_has_hw_pte_young arch_has_hw_pte_young
> > > +static inline bool arch_has_hw_pte_young(void)
> > > +{
> > > +	return riscv_has_extension_unlikely(RISCV_ISA_EXT_SVADU) &&
> > > +	       !riscv_has_extension_likely(RISCV_ISA_EXT_SVADE);
> > 
> > It's hard to guess what is, or will be, more likely to be the correct
> > choice of call between the _unlikely and _likely variants. But, while we
> > assume svade is most prevalent right now, it's actually quite unlikely
> > that 'svade' will be in the DT, since DTs haven't been putting it there
> > yet. Anyway, it doesn't really matter much and maybe the _unlikely vs.
> > _likely variants are better for documenting expectations than for
> > performance.
> 
> binding hat off, and kernel hat on, what do we actually do if there's
> neither Svadu or Svade in the firmware's description of the hardware?
> Do we just arbitrarily turn on Svade, like we already do for some
> extensions:
> 	/*
> 	 * These ones were as they were part of the base ISA when the
> 	 * port & dt-bindings were upstreamed, and so can be set
> 	 * unconditionally where `i` is in riscv,isa on DT systems.
> 	 */
> 	if (acpi_disabled) {
> 		set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa);
> 		set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa);
> 		set_bit(RISCV_ISA_EXT_ZICNTR, isainfo->isa);
> 		set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
> 	}
>

Yes, I think that's reasonable, assuming we do it in the final "pass",
where we're sure svadu isn't present. Doing this is a good idea since
we'll be able to simplify conditions, as we can just use 'if (svade)'
since !svade would imply svadu. With FWFT and both, we'll want to remove
svade from the isa bitmap when enabling svadu.

Thanks,
drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v5 1/4] RISC-V: Add Svade and Svadu Extensions Support

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 7678 bytes --]

On Fri, Jun 21, 2024 at 12:42:32PM +0200, Andrew Jones wrote:
> On Fri, Jun 21, 2024 at 11:24:19AM GMT, Conor Dooley wrote:
> > On Fri, Jun 21, 2024 at 10:43:58AM +0200, Andrew Jones wrote:
> > > On Wed, Jun 05, 2024 at 08:15:07PM GMT, Yong-Xuan Wang wrote:
> > > > Svade and Svadu extensions represent two schemes for managing the PTE A/D
> > > > bits. When the PTE A/D bits need to be set, Svade extension intdicates
> > > > that a related page fault will be raised. In contrast, the Svadu extension
> > > > supports hardware updating of PTE A/D bits. Since the Svade extension is
> > > > mandatory and the Svadu extension is optional in RVA23 profile, by default
> > > > the M-mode firmware will enable the Svadu extension in the menvcfg CSR
> > > > when only Svadu is present in DT.
> > > > 
> > > > This patch detects Svade and Svadu extensions from DT and adds
> > > > arch_has_hw_pte_young() to enable optimization in MGLRU and
> > > > __wp_page_copy_user() when we have the PTE A/D bits hardware updating
> > > > support.
> > > > 
> > > > Co-developed-by: Jinyu Tang <tjytimi@163.com>
> > > > Signed-off-by: Jinyu Tang <tjytimi@163.com>
> > > > Signed-off-by: Yong-Xuan Wang <yongxuan.wang@sifive.com>
> > > > ---
> > > >  arch/riscv/Kconfig               |  1 +
> > > >  arch/riscv/include/asm/csr.h     |  1 +
> > > >  arch/riscv/include/asm/hwcap.h   |  2 ++
> > > >  arch/riscv/include/asm/pgtable.h | 14 +++++++++++++-
> > > >  arch/riscv/kernel/cpufeature.c   |  2 ++
> > > >  5 files changed, 19 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > > index b94176e25be1..dbfe2be99bf9 100644
> > > > --- a/arch/riscv/Kconfig
> > > > +++ b/arch/riscv/Kconfig
> > > > @@ -36,6 +36,7 @@ config RISCV
> > > >  	select ARCH_HAS_PMEM_API
> > > >  	select ARCH_HAS_PREPARE_SYNC_CORE_CMD
> > > >  	select ARCH_HAS_PTE_SPECIAL
> > > > +	select ARCH_HAS_HW_PTE_YOUNG
> > > >  	select ARCH_HAS_SET_DIRECT_MAP if MMU
> > > >  	select ARCH_HAS_SET_MEMORY if MMU
> > > >  	select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
> > > > diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
> > > > index 25966995da04..524cd4131c71 100644
> > > > --- a/arch/riscv/include/asm/csr.h
> > > > +++ b/arch/riscv/include/asm/csr.h
> > > > @@ -195,6 +195,7 @@
> > > >  /* xENVCFG flags */
> > > >  #define ENVCFG_STCE			(_AC(1, ULL) << 63)
> > > >  #define ENVCFG_PBMTE			(_AC(1, ULL) << 62)
> > > > +#define ENVCFG_ADUE			(_AC(1, ULL) << 61)
> > > >  #define ENVCFG_CBZE			(_AC(1, UL) << 7)
> > > >  #define ENVCFG_CBCFE			(_AC(1, UL) << 6)
> > > >  #define ENVCFG_CBIE_SHIFT		4
> > > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > > > index e17d0078a651..35d7aa49785d 100644
> > > > --- a/arch/riscv/include/asm/hwcap.h
> > > > +++ b/arch/riscv/include/asm/hwcap.h
> > > > @@ -81,6 +81,8 @@
> > > >  #define RISCV_ISA_EXT_ZTSO		72
> > > >  #define RISCV_ISA_EXT_ZACAS		73
> > > >  #define RISCV_ISA_EXT_XANDESPMU		74
> > > > +#define RISCV_ISA_EXT_SVADE             75
> > > > +#define RISCV_ISA_EXT_SVADU		76
> > > >  
> > > >  #define RISCV_ISA_EXT_XLINUXENVCFG	127
> > > >  
> > > > diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> > > > index aad8b8ca51f1..7287ea4a6160 100644
> > > > --- a/arch/riscv/include/asm/pgtable.h
> > > > +++ b/arch/riscv/include/asm/pgtable.h
> > > > @@ -120,6 +120,7 @@
> > > >  #include <asm/tlbflush.h>
> > > >  #include <linux/mm_types.h>
> > > >  #include <asm/compat.h>
> > > > +#include <asm/cpufeature.h>
> > > >  
> > > >  #define __page_val_to_pfn(_val)  (((_val) & _PAGE_PFN_MASK) >> _PAGE_PFN_SHIFT)
> > > >  
> > > > @@ -288,7 +289,6 @@ static inline pte_t pud_pte(pud_t pud)
> > > >  }
> > > >  
> > > >  #ifdef CONFIG_RISCV_ISA_SVNAPOT
> > > > -#include <asm/cpufeature.h>
> > > >  
> > > >  static __always_inline bool has_svnapot(void)
> > > >  {
> > > > @@ -624,6 +624,18 @@ static inline pgprot_t pgprot_writecombine(pgprot_t _prot)
> > > >  	return __pgprot(prot);
> > > >  }
> > > >  
> > > > +/*
> > > > + * Both Svade and Svadu control the hardware behavior when the PTE A/D bits need to be set. By
> > > > + * default the M-mode firmware enables the hardware updating scheme when only Svadu is present in
> > > > + * DT.
> > > > + */
> > > > +#define arch_has_hw_pte_young arch_has_hw_pte_young
> > > > +static inline bool arch_has_hw_pte_young(void)
> > > > +{
> > > > +	return riscv_has_extension_unlikely(RISCV_ISA_EXT_SVADU) &&
> > > > +	       !riscv_has_extension_likely(RISCV_ISA_EXT_SVADE);
> > > 
> > > It's hard to guess what is, or will be, more likely to be the correct
> > > choice of call between the _unlikely and _likely variants. But, while we
> > > assume svade is most prevalent right now, it's actually quite unlikely
> > > that 'svade' will be in the DT, since DTs haven't been putting it there
> > > yet. Anyway, it doesn't really matter much and maybe the _unlikely vs.
> > > _likely variants are better for documenting expectations than for
> > > performance.
> > 
> > binding hat off, and kernel hat on, what do we actually do if there's
> > neither Svadu or Svade in the firmware's description of the hardware?
> > Do we just arbitrarily turn on Svade, like we already do for some
> > extensions:
> > 	/*
> > 	 * These ones were as they were part of the base ISA when the
> > 	 * port & dt-bindings were upstreamed, and so can be set
> > 	 * unconditionally where `i` is in riscv,isa on DT systems.
> > 	 */
> > 	if (acpi_disabled) {
> > 		set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa);
> > 		set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa);
> > 		set_bit(RISCV_ISA_EXT_ZICNTR, isainfo->isa);
> > 		set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
> > 	}
> >
> 
> Yes, I think that's reasonable, assuming we do it in the final "pass",
> where we're sure svadu isn't present.

I haven't thought about specifically when to do it, but does it need to
be in the final pass? If we were to, on each CPU, enable it if Svadu
isn't there, we'd either end up with a system that I suspect we're not
going to be supporting or the correct result. Or am I misunderstanding,
and it will be valid to have a subset of CPUs that have Svadu enabled
from the bootloader?

Note that it would not be problematic to have 3 CPUs with Svade + Svadu
and a 4th with only Svade in the DT because we would just not use the
FWFT mechanism to enable Svadu. It's just the Svadu in isolation case
that I'm asking about.

> Doing this is a good idea since
> we'll be able to simplify conditions, as we can just use 'if (svade)'
> since !svade would imply svadu. With FWFT and both, we'll want to remove
> svade from the isa bitmap when enabling svadu.

Right I would like to move the various extension stuff in this
direction, where they have a bit more intelligence to them, and don't
just reflect the state in DT/ACPI directly.
I've got some patches in mind once Clement's Zca etc patchset
is merged, think I posted one or two as replies to conversations on
the list already. An example would be disabling the vector crypto
extensions if we've had to disable vector, or as you suggest here,
dropping Svade if we have turned on Svadu using FWFT. I think that makes
the APIs more understandable to developers and more useful than they are
at the moment, where to use vector crypto you also need to check vector
itself for the code to be correct. If I call
riscv_isa_extension_available(), and it returns true, the extension
should be usable IMO.

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v5 1/4] RISC-V: Add Svade and Svadu Extensions Support

[Andrew Jones]

On Fri, Jun 21, 2024 at 12:00:32PM GMT, Conor Dooley wrote:
> On Fri, Jun 21, 2024 at 12:42:32PM +0200, Andrew Jones wrote:
> > On Fri, Jun 21, 2024 at 11:24:19AM GMT, Conor Dooley wrote:
> > > On Fri, Jun 21, 2024 at 10:43:58AM +0200, Andrew Jones wrote:
...
> > > > It's hard to guess what is, or will be, more likely to be the correct
> > > > choice of call between the _unlikely and _likely variants. But, while we
> > > > assume svade is most prevalent right now, it's actually quite unlikely
> > > > that 'svade' will be in the DT, since DTs haven't been putting it there
> > > > yet. Anyway, it doesn't really matter much and maybe the _unlikely vs.
> > > > _likely variants are better for documenting expectations than for
> > > > performance.
> > > 
> > > binding hat off, and kernel hat on, what do we actually do if there's
> > > neither Svadu or Svade in the firmware's description of the hardware?
> > > Do we just arbitrarily turn on Svade, like we already do for some
> > > extensions:
> > > 	/*
> > > 	 * These ones were as they were part of the base ISA when the
> > > 	 * port & dt-bindings were upstreamed, and so can be set
> > > 	 * unconditionally where `i` is in riscv,isa on DT systems.
> > > 	 */
> > > 	if (acpi_disabled) {
> > > 		set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa);
> > > 		set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa);
> > > 		set_bit(RISCV_ISA_EXT_ZICNTR, isainfo->isa);
> > > 		set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
> > > 	}
> > >
> > 
> > Yes, I think that's reasonable, assuming we do it in the final "pass",
> > where we're sure svadu isn't present.
> 
> I haven't thought about specifically when to do it, but does it need to
> be in the final pass? If we were to, on each CPU, enable it if Svadu
> isn't there, we'd either end up with a system that I suspect we're not
> going to be supporting or the correct result. Or am I misunderstanding,
> and it will be valid to have a subset of CPUs that have Svadu enabled
> from the bootloader?
> 
> Note that it would not be problematic to have 3 CPUs with Svade + Svadu
> and a 4th with only Svade in the DT because we would just not use the
> FWFT mechanism to enable Svadu. It's just the Svadu in isolation case
> that I'm asking about.

I wasn't thinking about the potential of mixmatched A/D udpating. I'm
pretty sure this will be one of those things that is all or none. I
was more concerned with getting the result right and I had just been
too lazy to double check that the block of code you pointed out is
in the right place to be sure there's no svadu. Now that I look, I
believe it is.

> 
> > Doing this is a good idea since
> > we'll be able to simplify conditions, as we can just use 'if (svade)'
> > since !svade would imply svadu. With FWFT and both, we'll want to remove
> > svade from the isa bitmap when enabling svadu.
> 
> Right I would like to move the various extension stuff in this
> direction, where they have a bit more intelligence to them, and don't
> just reflect the state in DT/ACPI directly.
> I've got some patches in mind once Clement's Zca etc patchset
> is merged, think I posted one or two as replies to conversations on
> the list already. An example would be disabling the vector crypto
> extensions if we've had to disable vector, or as you suggest here,
> dropping Svade if we have turned on Svadu using FWFT. I think that makes
> the APIs more understandable to developers and more useful than they are
> at the moment, where to use vector crypto you also need to check vector
> itself for the code to be correct. If I call
> riscv_isa_extension_available(), and it returns true, the extension
> should be usable IMO.

Sounds good to me.

Thanks,
drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Alexandre Ghiti]

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.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Andrew Jones]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 7847 bytes --]

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?




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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Andrew Jones]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 2431 bytes --]

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.

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Andrew Jones]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 3197 bytes --]

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.

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

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Yong-Xuan Wang]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Yong-Xuan Wang]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Andrew Jones]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v5 1/4] RISC-V: Add Svade and Svadu Extensions Support

[Yong-Xuan Wang]

Hi Conor, Andrew and Alexandre,

On Fri, Jun 21, 2024 at 8:06 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Fri, Jun 21, 2024 at 12:00:32PM GMT, Conor Dooley wrote:
> > On Fri, Jun 21, 2024 at 12:42:32PM +0200, Andrew Jones wrote:
> > > On Fri, Jun 21, 2024 at 11:24:19AM GMT, Conor Dooley wrote:
> > > > On Fri, Jun 21, 2024 at 10:43:58AM +0200, Andrew Jones wrote:
> ...
> > > > > It's hard to guess what is, or will be, more likely to be the correct
> > > > > choice of call between the _unlikely and _likely variants. But, while we
> > > > > assume svade is most prevalent right now, it's actually quite unlikely
> > > > > that 'svade' will be in the DT, since DTs haven't been putting it there
> > > > > yet. Anyway, it doesn't really matter much and maybe the _unlikely vs.
> > > > > _likely variants are better for documenting expectations than for
> > > > > performance.
> > > >
> > > > binding hat off, and kernel hat on, what do we actually do if there's
> > > > neither Svadu or Svade in the firmware's description of the hardware?
> > > > Do we just arbitrarily turn on Svade, like we already do for some
> > > > extensions:
> > > >   /*
> > > >    * These ones were as they were part of the base ISA when the
> > > >    * port & dt-bindings were upstreamed, and so can be set
> > > >    * unconditionally where `i` is in riscv,isa on DT systems.
> > > >    */
> > > >   if (acpi_disabled) {
> > > >           set_bit(RISCV_ISA_EXT_ZICSR, isainfo->isa);
> > > >           set_bit(RISCV_ISA_EXT_ZIFENCEI, isainfo->isa);
> > > >           set_bit(RISCV_ISA_EXT_ZICNTR, isainfo->isa);
> > > >           set_bit(RISCV_ISA_EXT_ZIHPM, isainfo->isa);
> > > >   }
> > > >
> > >
> > > Yes, I think that's reasonable, assuming we do it in the final "pass",
> > > where we're sure svadu isn't present.
> >
> > I haven't thought about specifically when to do it, but does it need to
> > be in the final pass? If we were to, on each CPU, enable it if Svadu
> > isn't there, we'd either end up with a system that I suspect we're not
> > going to be supporting or the correct result. Or am I misunderstanding,
> > and it will be valid to have a subset of CPUs that have Svadu enabled
> > from the bootloader?
> >
> > Note that it would not be problematic to have 3 CPUs with Svade + Svadu
> > and a 4th with only Svade in the DT because we would just not use the
> > FWFT mechanism to enable Svadu. It's just the Svadu in isolation case
> > that I'm asking about.
>
> I wasn't thinking about the potential of mixmatched A/D udpating. I'm
> pretty sure this will be one of those things that is all or none. I
> was more concerned with getting the result right and I had just been
> too lazy to double check that the block of code you pointed out is
> in the right place to be sure there's no svadu. Now that I look, I
> believe it is.
>
> >
> > > Doing this is a good idea since
> > > we'll be able to simplify conditions, as we can just use 'if (svade)'
> > > since !svade would imply svadu. With FWFT and both, we'll want to remove
> > > svade from the isa bitmap when enabling svadu.
> >
> > Right I would like to move the various extension stuff in this
> > direction, where they have a bit more intelligence to them, and don't
> > just reflect the state in DT/ACPI directly.
> > I've got some patches in mind once Clement's Zca etc patchset
> > is merged, think I posted one or two as replies to conversations on
> > the list already. An example would be disabling the vector crypto
> > extensions if we've had to disable vector, or as you suggest here,
> > dropping Svade if we have turned on Svadu using FWFT. I think that makes
> > the APIs more understandable to developers and more useful than they are
> > at the moment, where to use vector crypto you also need to check vector
> > itself for the code to be correct. If I call
> > riscv_isa_extension_available(), and it returns true, the extension
> > should be usable IMO.

Thank you all very much! I will update the code so that
riscv_isa_extension_available()
can reflect the platform's behavior.

Regards,
Yong-Xuan

>
> Sounds good to me.
>
> Thanks,
> drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv