Hi Daniel,
On Sat, Jun 7, 2025 at 7:54 PM Daniel Henrique Barboza
<dbarb...@ventanamicro.com> wrote:
>
>
>
> On 6/5/25 11:21 AM, Alexandre Ghiti wrote:
> > The Svrsw60t59b extension allows to free the PTE reserved bits 60 and 59
> > for software to use.
> >
> > Signed-off-by: Alexandre Ghiti <alexgh...@rivosinc.com>
> > ---
> >
> > Changes in v2:
> > - Add support for IOMMU
> > - Make svrsw60t59b depend on sv39 (deepak)
> >
> > Open question: svrsw60t59b in IOMMU should also depend on 64bit, but I
> > did not find an easy to way in riscv_iommu_realize() to detect that, how
> > should I do?
>
>
> What controls the IOMMU behavior is the set of IOMMU capabilities that the
> driver
> chooses to use. Other than that the device should be oblivious to the CPU word
> size.
>
> From what I see in this patch you did the right thing: you added a new
> capability
> to be advertised to software and that's it. It's up to software to decide
> whether
> it's going to use it or not. You can advertise a 64 bit only IOMMU capability
> running
> in a 32 bit CPU and it's up to the OS to not use/ignore it. In fact we
> already do
> that: satp_mode related caps (e.g. RISCV_IOMMU_CAP_SV32X4) are 32/64 bits
> exclusive
> but are always advertised.
>
>
>
> Now, Svrsw60t59b being a 32 bit only extension requires special handling in
> riscv_init_max_cpu_extensions() because the 'max' CPU has a 32 bit variant and
> enabled everything by default. You can use this diff:
>
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index f93cd53f37..96201e15c6 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -1612,6 +1612,8 @@ static void riscv_init_max_cpu_extensions(Object *obj)
>
> if (env->misa_mxl != MXL_RV32) {
> isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zcf), false);
> + } else {
> + isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_svrsw60t59b), false);
> }
>
> /*
>
>
> To fix this test break in 'make check-functional':
>
> Command: /home/danielhb/work/qemu/build/qemu-system-riscv32 -display
> none -vga none -chardev socket,id=mon,fd=5 -mon chardev=mon,mode=control
> -machine virt -chardev socket,id=console,fd=10 -serial chardev:console -cpu
> max -kernel
> /home/danielhb/.cache/qemu/download/872bc8f8e0d4661825d5f47f7bec64988e9d0a8bd5db8917d57e16f66d83b329
> -append printk.time=0 root=/dev/vda console=ttyS0 -blockdev
> driver=raw,file.driver=file,file.filename=/home/danielhb/work/qemu/build/tests/functional/riscv32/test_riscv32_tuxrun.TuxRunRiscV32Test.test_riscv32_maxcpu/scratch/511ad34e63222db08d6c1da16fad224970de36517a784110956ba6a24a0ee5f6,node-name=hd0
> -device virtio-blk-device,drive=hd0
> Output: qemu-system-riscv32: svrsw60t59b is not supported on RV32 and
> MMU-less platforms
Sorry for the late answer and thanks for the fix ^! I have just sent the v2.
Thanks,
Alex
>
>
> Thanks,
>
> Daniel
>
>
> >
> > hw/riscv/riscv-iommu-bits.h | 1 +
> > hw/riscv/riscv-iommu.c | 3 ++-
> > target/riscv/cpu.c | 2 ++
> > target/riscv/cpu_bits.h | 3 ++-
> > target/riscv/cpu_cfg_fields.h.inc | 1 +
> > target/riscv/cpu_helper.c | 3 ++-
> > target/riscv/tcg/tcg-cpu.c | 6 ++++++
> > 7 files changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/riscv/riscv-iommu-bits.h b/hw/riscv/riscv-iommu-bits.h
> > index 1017d73fc6..47fe01bee5 100644
> > --- a/hw/riscv/riscv-iommu-bits.h
> > +++ b/hw/riscv/riscv-iommu-bits.h
> > @@ -79,6 +79,7 @@ struct riscv_iommu_pq_record {
> > #define RISCV_IOMMU_CAP_SV39 BIT_ULL(9)
> > #define RISCV_IOMMU_CAP_SV48 BIT_ULL(10)
> > #define RISCV_IOMMU_CAP_SV57 BIT_ULL(11)
> > +#define RISCV_IOMMU_CAP_SVRSW60T59B BIT_ULL(14)
> > #define RISCV_IOMMU_CAP_SV32X4 BIT_ULL(16)
> > #define RISCV_IOMMU_CAP_SV39X4 BIT_ULL(17)
> > #define RISCV_IOMMU_CAP_SV48X4 BIT_ULL(18)
> > diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
> > index a877e5da84..36eda95a1c 100644
> > --- a/hw/riscv/riscv-iommu.c
> > +++ b/hw/riscv/riscv-iommu.c
> > @@ -2355,7 +2355,8 @@ static void riscv_iommu_realize(DeviceState *dev,
> > Error **errp)
> > }
> > if (s->enable_g_stage) {
> > s->cap |= RISCV_IOMMU_CAP_SV32X4 | RISCV_IOMMU_CAP_SV39X4 |
> > - RISCV_IOMMU_CAP_SV48X4 | RISCV_IOMMU_CAP_SV57X4;
> > + RISCV_IOMMU_CAP_SV48X4 | RISCV_IOMMU_CAP_SV57X4 |
> > + RISCV_IOMMU_CAP_SVRSW60T59B;
> > }
> >
> > if (s->hpm_cntrs > 0) {
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 629ac37501..13f1f56d95 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -228,6 +228,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
> > ISA_EXT_DATA_ENTRY(svinval, PRIV_VERSION_1_12_0, ext_svinval),
> > ISA_EXT_DATA_ENTRY(svnapot, PRIV_VERSION_1_12_0, ext_svnapot),
> > ISA_EXT_DATA_ENTRY(svpbmt, PRIV_VERSION_1_12_0, ext_svpbmt),
> > + ISA_EXT_DATA_ENTRY(svrsw60t59b, PRIV_VERSION_1_13_0, ext_svrsw60t59b),
> > ISA_EXT_DATA_ENTRY(svukte, PRIV_VERSION_1_13_0, ext_svukte),
> > ISA_EXT_DATA_ENTRY(svvptc, PRIV_VERSION_1_13_0, ext_svvptc),
> > ISA_EXT_DATA_ENTRY(xtheadba, PRIV_VERSION_1_11_0, ext_xtheadba),
> > @@ -1282,6 +1283,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] =
> > {
> > MULTI_EXT_CFG_BOOL("svinval", ext_svinval, false),
> > MULTI_EXT_CFG_BOOL("svnapot", ext_svnapot, false),
> > MULTI_EXT_CFG_BOOL("svpbmt", ext_svpbmt, false),
> > + MULTI_EXT_CFG_BOOL("svrsw60t59b", ext_svrsw60t59b, false),
> > MULTI_EXT_CFG_BOOL("svvptc", ext_svvptc, true),
> >
> > MULTI_EXT_CFG_BOOL("zicntr", ext_zicntr, true),
> > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > index a30317c617..51eb7114da 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -675,7 +675,8 @@ typedef enum {
> > #define PTE_SOFT 0x300 /* Reserved for Software */
> > #define PTE_PBMT 0x6000000000000000ULL /* Page-based memory
> > types */
> > #define PTE_N 0x8000000000000000ULL /* NAPOT translation */
> > -#define PTE_RESERVED 0x1FC0000000000000ULL /* Reserved bits */
> > +#define PTE_RESERVED(svrsw60t59b) \
> > + (svrsw60t59b ? 0x07C0000000000000ULL : 0x1FC0000000000000ULL)
> > /* Reserved bits */
> > #define PTE_ATTR (PTE_N | PTE_PBMT) /* All attributes bits */
> >
> > /* Page table PPN shift amount */
> > diff --git a/target/riscv/cpu_cfg_fields.h.inc
> > b/target/riscv/cpu_cfg_fields.h.inc
> > index 59f134a419..ab61c1ccf2 100644
> > --- a/target/riscv/cpu_cfg_fields.h.inc
> > +++ b/target/riscv/cpu_cfg_fields.h.inc
> > @@ -57,6 +57,7 @@ BOOL_FIELD(ext_svadu)
> > BOOL_FIELD(ext_svinval)
> > BOOL_FIELD(ext_svnapot)
> > BOOL_FIELD(ext_svpbmt)
> > +BOOL_FIELD(ext_svrsw60t59b)
> > BOOL_FIELD(ext_svvptc)
> > BOOL_FIELD(ext_svukte)
> > BOOL_FIELD(ext_zdinx)
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 2ed69d7c2d..3479a62cc7 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -1309,6 +1309,7 @@ static int get_physical_address(CPURISCVState *env,
> > hwaddr *physical,
> > bool svade = riscv_cpu_cfg(env)->ext_svade;
> > bool svadu = riscv_cpu_cfg(env)->ext_svadu;
> > bool adue = svadu ? env->menvcfg & MENVCFG_ADUE : !svade;
> > + bool svrsw60t59b = riscv_cpu_cfg(env)->ext_svrsw60t59b;
> >
> > if (first_stage && two_stage && env->virt_enabled) {
> > pbmte = pbmte && (env->henvcfg & HENVCFG_PBMTE);
> > @@ -1376,7 +1377,7 @@ static int get_physical_address(CPURISCVState *env,
> > hwaddr *physical,
> > if (riscv_cpu_sxl(env) == MXL_RV32) {
> > ppn = pte >> PTE_PPN_SHIFT;
> > } else {
> > - if (pte & PTE_RESERVED) {
> > + if (pte & PTE_RESERVED(svrsw60t59b)) {
> > qemu_log_mask(LOG_GUEST_ERROR, "%s: reserved bits set in
> > PTE: "
> > "addr: 0x%" HWADDR_PRIx " pte: 0x"
> > TARGET_FMT_lx "\n",
> > __func__, pte_addr, pte);
> > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> > index 305912b8dd..886006abc3 100644
> > --- a/target/riscv/tcg/tcg-cpu.c
> > +++ b/target/riscv/tcg/tcg-cpu.c
> > @@ -804,6 +804,12 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu,
> > Error **errp)
> > cpu->cfg.ext_ssctr = false;
> > }
> >
> > + if (cpu->cfg.ext_svrsw60t59b &&
> > + (!cpu->cfg.mmu || mcc->def->misa_mxl_max == MXL_RV32)) {
> > + error_setg(errp, "svrsw60t59b is not supported on RV32 and
> > MMU-less platforms");
> > + return;
> > + }
> > +
> > /*
> > * Disable isa extensions based on priv spec after we
> > * validated and set everything we need.
>
