target/riscv: Support SW update of PTE A/D bits and Ssptwad extension

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

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(+)

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

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.

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

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

On Wed, Jul 20, 2022 at 1:52 PM Anup Patel <anup@brainfault.org> wrote:
>
> 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 ?

I'm just trying to wrap my head around this.

So the priv spec has this line:

"Two schemes to manage the A and D bits are permitted"

The first scheme just raises a page-fault exception, the second scheme
updates the A/D bits as you would expect.

The profiles spec then states that you must do the second scheme, as
that is what all software expects.

This patch is adding optional support for the first scheme.

Why do we want to support that? We can do either and we are
implementing the much more usual scheme. I don't see a reason to
bother implementing the other one. Is anyone ever going to use it?

Alistair

Hi Alistair,

Why do we want to support that? We can do either and we are
> implementing the much more usual scheme. I don't see a reason to
> bother implementing the other one. Is anyone ever going to use it?
>

Thanks for your response.
I got it.

Regards,
Jim Shu