Re: [PATCH] target/riscv: Support SW update of PTE A/D bits and Ssptwad extension

[Anup Patel]

On Wed, Jul 20, 2022 at 5:02 AM Jim Shu <jim.shu@sifive.com> wrote:
>
> Hi Anup,
>
> Do you think it is OK if we only use ssptwad as a CPU option name
> but not a RISC-V extension? just like MMU and PMP options of RISC-V.
> (And we could change it to RISC-V extension in the future
> if Ssptwad becomes the formal RISC-V extension)
>
> Both HW/SW update schemes are already defined in RISC-V priv spec,
> so I think it's reasonable to implement them in QEMU. The only issue here is
> to choose a proper CPU option name to turn on/off HW update of A/D bits.

I am not saying that we should avoid implementing it in QEMU.

My suggestion is to differentiate HW optionalities from ISA extensions
in QEMU. For example, instead of referring to this as Ssptwad, we should
call it "hw_ptwad" or "opt_ptwad" and don't use the "ext_" prefix.

@Alistair Francis might have better suggestions on this ?

Regards,
Anup

>
> Regards,
> Jim Shu
>
> On Mon, Jul 18, 2022 at 12:02 PM Anup Patel <anup@brainfault.org> wrote:
>>
>> +Atish
>>
>> On Mon, Jul 18, 2022 at 9:23 AM Jim Shu <jim.shu@sifive.com> wrote:
>> >
>> > RISC-V priv spec v1.12 permits 2 PTE-update schemes of A/D-bit
>> > (Access/Dirty bit): HW update or SW update. RISC-V profile defines the
>> > extension name 'Ssptwad' for HW update to PTE A/D bits.
>> > https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc
>>
>> The Ssptwad (even though used by profiles) is not a well defined RISC-V
>> ISA extension. Rather, Ssptwad is just a name used in profiles to represent
>> an optional feature.
>>
>> In fact, since it is not a well-defined ISA extension, QEMU cannot include
>> Ssptwad in the ISA string as well.
>>
>> I think for such optionalities which are not well-defined ISA extensions,
>> QEMU should treat it differently.
>>
>> Regards,
>> Anup
>>
>> >
>> > Current QEMU RISC-V implements HW update scheme, so this commit
>> > introduces SW update scheme to QEMU and uses the 'Ssptwad' extension
>> > as the CPU option to select 2 PTE-update schemes. QEMU RISC-V CPU still
>> > uses HW update scheme (ext_ssptwad=true) by default to keep the backward
>> > compatibility.
>> >
>> > SW update scheme implemention is based on priv spec v1.12:
>> > "When a virtual page is accessed and the A bit is clear, or is written
>> > and the D bit is clear, a page-fault exception (corresponding to the
>> > original access type) is raised."
>> >
>> > Signed-off-by: Jim Shu <jim.shu@sifive.com>
>> > Reviewed-by: Frank Chang <frank.chang@sifive.com>
>> > ---
>> >  target/riscv/cpu.c        | 2 ++
>> >  target/riscv/cpu.h        | 1 +
>> >  target/riscv/cpu_helper.c | 9 +++++++++
>> >  3 files changed, 12 insertions(+)
>> >
>> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> > index 1bb3973806..1d38c1c1d2 100644
>> > --- a/target/riscv/cpu.c
>> > +++ b/target/riscv/cpu.c
>> > @@ -857,6 +857,7 @@ static void riscv_cpu_init(Object *obj)
>> >
>> >      cpu->cfg.ext_ifencei = true;
>> >      cpu->cfg.ext_icsr = true;
>> > +    cpu->cfg.ext_ssptwad = true;
>> >      cpu->cfg.mmu = true;
>> >      cpu->cfg.pmp = true;
>> >
>> > @@ -900,6 +901,7 @@ static Property riscv_cpu_extensions[] = {
>> >      DEFINE_PROP_BOOL("svinval", RISCVCPU, cfg.ext_svinval, false),
>> >      DEFINE_PROP_BOOL("svnapot", RISCVCPU, cfg.ext_svnapot, false),
>> >      DEFINE_PROP_BOOL("svpbmt", RISCVCPU, cfg.ext_svpbmt, false),
>> > +    DEFINE_PROP_BOOL("ssptwad", RISCVCPU, cfg.ext_ssptwad, true),
>> >
>> >      DEFINE_PROP_BOOL("zba", RISCVCPU, cfg.ext_zba, true),
>> >      DEFINE_PROP_BOOL("zbb", RISCVCPU, cfg.ext_zbb, true),
>> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> > index 5c7acc055a..2eee59af98 100644
>> > --- a/target/riscv/cpu.h
>> > +++ b/target/riscv/cpu.h
>> > @@ -433,6 +433,7 @@ struct RISCVCPUConfig {
>> >      bool ext_zve32f;
>> >      bool ext_zve64f;
>> >      bool ext_zmmul;
>> > +    bool ext_ssptwad;
>> >      bool rvv_ta_all_1s;
>> >
>> >      uint32_t mvendorid;
>> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> > index 59b3680b1b..a8607c0d7b 100644
>> > --- a/target/riscv/cpu_helper.c
>> > +++ b/target/riscv/cpu_helper.c
>> > @@ -981,6 +981,15 @@ restart:
>> >
>> >              /* Page table updates need to be atomic with MTTCG enabled */
>> >              if (updated_pte != pte) {
>> > +                if (!cpu->cfg.ext_ssptwad) {
>> > +                    /*
>> > +                     * If A/D bits are managed by SW, HW just raises the
>> > +                     * page fault exception corresponding to the original
>> > +                     * access type when A/D bits need to be updated.
>> > +                     */
>> > +                    return TRANSLATE_FAIL;
>> > +                }
>> > +
>> >                  /*
>> >                   * - if accessed or dirty bits need updating, and the PTE 
>> > is
>> >                   *   in RAM, then we do so atomically with a compare and 
>> > swap.
>> > --
>> > 2.17.1
>> >
>> >