Re: [PATCH] target: riscv: Add Svrsw60t59b extension support

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

On 6/25/25 4:36 AM, Alexandre Ghiti wrote:
> Hi Daniel,
> 
> On Sat, Jun 7, 2025 at 7:54 PM Daniel Henrique Barboza
> <dbarboza@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 <alexghiti@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.


I'm not seeing it in my box and in qemu-devel and in qemu-riscv. Perhaps a
re-sending might be in order. Thanks,


Daniel

> 
> 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.
>>