Re: [PATCH v6 01/12] target/riscv: add zicbop extension flag

[Daniel Henrique Barboza]

On 10/28/23 06:49, Andrew Jones wrote:
> On Sat, Oct 28, 2023 at 05:54:16AM -0300, Daniel Henrique Barboza wrote:
> > QEMU already implements zicbom (Cache Block Management Operations) and
> > zicboz (Cache Block Zero Operations). Commit 59cb29d6a5 ("target/riscv:
> > add Zicbop cbo.prefetch{i, r, m} placeholder") added placeholders for
> > what would be the instructions for zicbop (Cache Block Prefetch
> > Operations), which are now no-ops.
> >
> > The RVA22U64 profile mandates zicbop, which means that applications that
> > run with this profile might expect zicbop to be present in the riscv,isa
> > DT and might behave badly if it's absent.
> >
> > Adding zicbop as an extension will make our future RVA22U64
> > implementation more in line with what userspace expects and, if/when
> > cache block prefetch operations became relevant to QEMU, we already have
> > the extension flag to turn then on/off as needed.
> >
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> >   hw/riscv/virt.c        | 5 +++++
> >   target/riscv/cpu.c     | 3 +++
> >   target/riscv/cpu_cfg.h | 2 ++
> >   3 files changed, 10 insertions(+)
> >
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index 1732c42915..99c087240f 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -273,6 +273,11 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int 
> > socket,
> >                                     cpu_ptr->cfg.cboz_blocksize);
> >           }
> >   
> > +        if (cpu_ptr->cfg.ext_zicbop) {
> > +            qemu_fdt_setprop_cell(ms->fdt, cpu_name, "riscv,cbop-block-size",
>
> I think we need to get this node approved by devicetree@vger.kernel.org
> and merged into [1] first.
>
> [1] Linux repo: Documentation/devicetree/bindings/riscv/cpus.yaml

We got an ack from the devicetree folks to add the "riscv,cbop-block-size"
DT node:

https://lore.kernel.org/linux-devicetree/20231030-attest-unchain-ab99981a5738@spud/


I believe we don't have blockers to proceed with this change then. Thanks,


Daniel

> > +                                  cpu_ptr->cfg.cbop_blocksize);
> > +        }
> > +
> >           qemu_fdt_setprop_string(ms->fdt, cpu_name, "compatible", "riscv");
> >           qemu_fdt_setprop_string(ms->fdt, cpu_name, "status", "okay");
> >           qemu_fdt_setprop_cell(ms->fdt, cpu_name, "reg",
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index f40da4c661..6c0050988f 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -78,6 +78,7 @@ const uint32_t misa_bits[] = {RVI, RVE, RVM, RVA, RVF, RVD, 
> > RVV,
> >    */
> >   const RISCVIsaExtData isa_edata_arr[] = {
> >       ISA_EXT_DATA_ENTRY(zicbom, PRIV_VERSION_1_12_0, ext_zicbom),
> > +    ISA_EXT_DATA_ENTRY(zicbop, PRIV_VERSION_1_12_0, ext_zicbop),
> >       ISA_EXT_DATA_ENTRY(zicboz, PRIV_VERSION_1_12_0, ext_zicboz),
> >       ISA_EXT_DATA_ENTRY(zicond, PRIV_VERSION_1_12_0, ext_zicond),
> >       ISA_EXT_DATA_ENTRY(zicntr, PRIV_VERSION_1_12_0, ext_zicntr),
> > @@ -1336,6 +1337,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
> >       MULTI_EXT_CFG_BOOL("zhinxmin", ext_zhinxmin, false),
> >   
> >       MULTI_EXT_CFG_BOOL("zicbom", ext_zicbom, true),
> > +    MULTI_EXT_CFG_BOOL("zicbop", ext_zicbop, true),
> >       MULTI_EXT_CFG_BOOL("zicboz", ext_zicboz, true),
> >   
> >       MULTI_EXT_CFG_BOOL("zmmul", ext_zmmul, false),
> > @@ -1424,6 +1426,7 @@ Property riscv_cpu_options[] = {
> >       DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
> >   
> >       DEFINE_PROP_UINT16("cbom_blocksize", RISCVCPU, cfg.cbom_blocksize, 64),
> > +    DEFINE_PROP_UINT16("cbop_blocksize", RISCVCPU, cfg.cbop_blocksize, 64),
> >       DEFINE_PROP_UINT16("cboz_blocksize", RISCVCPU, cfg.cboz_blocksize, 64),
> >   
> >       DEFINE_PROP_END_OF_LIST(),
> > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> > index 6eef4a51ea..2203b4c45b 100644
> > --- a/target/riscv/cpu_cfg.h
> > +++ b/target/riscv/cpu_cfg.h
> > @@ -65,6 +65,7 @@ struct RISCVCPUConfig {
> >       bool ext_zicntr;
> >       bool ext_zicsr;
> >       bool ext_zicbom;
> > +    bool ext_zicbop;
> >       bool ext_zicboz;
> >       bool ext_zicond;
> >       bool ext_zihintntl;
> > @@ -134,6 +135,7 @@ struct RISCVCPUConfig {
> >       uint16_t vlen;
> >       uint16_t elen;
> >       uint16_t cbom_blocksize;
> > +    uint16_t cbop_blocksize;
> >       uint16_t cboz_blocksize;
> >       bool mmu;
> >       bool pmp;
> > --
> > 2.41.0
>
> Otherwise,
>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>
> Thanks,
> drew