On Tue, Feb 14, 2023 at 6:52 PM LIU Zhiwei <zhiwei_...@linux.alibaba.com> wrote: > > > On 2023/2/9 14:23, Deepak Gupta wrote: > > Introducing riscv `zisslpcfi` extension to riscv target. `zisslpcfi` > > extension provides hardware assistance to riscv hart to enable control > > flow integrity (CFI) for software. > > > > `zisslpcfi` extension expects hart to implement `zimops`. `zimops` stands > > for "unprivileged integer maybe operations". `zimops` carve out certain > > reserved opcodes encodings from integer spec to "may be operations" > > encodings. `zimops` opcode encodings simply move 0 to rd. > > `zisslpcfi` claims some of the `zimops` encodings and use them for shadow > > stack management or indirect branch tracking. Any future extension can > > also claim `zimops` encodings. > > Does the zimops has a independent specification? If so, you should give > a link to this > specification.
Actual formal documentation is still a work in progress. I am hoping to provide a reference to it in my next iteration. > > > > > This patch also adds a dependency check for `zimops` to be enabled if > > `zisslpcfi` is enabled on the hart. > > You should don't add two extensions in one patch. I think you should add > them one by one. > And add the zimop first. In my opinion, you should implement the whole > zimop extension before > adding any patch for zisslpcfi, including the implementation of mop.rr > and mop.r. Noted will make sure of that and will send two different patch series then. > > > > > Signed-off-by: Deepak Gupta <de...@rivosinc.com> > > Signed-off-by: Kip Walker <k...@rivosinc.com> > > --- > > target/riscv/cpu.c | 13 +++++++++++++ > > target/riscv/cpu.h | 2 ++ > > 2 files changed, 15 insertions(+) > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > > index cc75ca7667..6b4e90eb91 100644 > > --- a/target/riscv/cpu.c > > +++ b/target/riscv/cpu.c > > @@ -110,6 +110,8 @@ static const struct isa_ext_data isa_edata_arr[] = { > > ISA_EXT_DATA_ENTRY(svnapot, true, PRIV_VERSION_1_12_0, ext_svnapot), > > ISA_EXT_DATA_ENTRY(svpbmt, true, PRIV_VERSION_1_12_0, ext_svpbmt), > > ISA_EXT_DATA_ENTRY(xventanacondops, true, PRIV_VERSION_1_12_0, > > ext_XVentanaCondOps), > > + ISA_EXT_DATA_ENTRY(zimops, true, PRIV_VERSION_1_12_0, ext_zimops), > > + ISA_EXT_DATA_ENTRY(zisslpcfi, true, PRIV_VERSION_1_12_0, ext_cfi), > Add them one by one. > > }; > > > > static bool isa_ext_is_enabled(RISCVCPU *cpu, > > @@ -792,6 +794,11 @@ static void riscv_cpu_realize(DeviceState *dev, Error > > **errp) > > return; > > } > > > > + if (cpu->cfg.ext_cfi && !cpu->cfg.ext_zimops) { > > + error_setg(errp, "Zisslpcfi extension requires Zimops > > extension"); > > + return; > > + } > > + > Seems reasonable for me. > > /* Set the ISA extensions, checks should have happened above */ > > if (cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinx || > > cpu->cfg.ext_zhinxmin) { > > @@ -1102,6 +1109,12 @@ static Property riscv_cpu_properties[] = { > > #ifndef CONFIG_USER_ONLY > > DEFINE_PROP_UINT64("resetvec", RISCVCPU, env.resetvec, > > DEFAULT_RSTVEC), > > #endif > > + /* > > + * Zisslpcfi CFI extension, Zisslpcfi implicitly means Zimops is > > + * implemented > > + */ > > + DEFINE_PROP_BOOL("zisslpcfi", RISCVCPU, cfg.ext_cfi, true), > > + DEFINE_PROP_BOOL("zimops", RISCVCPU, cfg.ext_zimops, true), > > Default value should be false. Yes, I have to fix this. > > Zhiwei > > > > > DEFINE_PROP_BOOL("short-isa-string", RISCVCPU, cfg.short_isa_string, > > false), > > > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > > index f5609b62a2..9a923760b2 100644 > > --- a/target/riscv/cpu.h > > +++ b/target/riscv/cpu.h > > @@ -471,6 +471,8 @@ struct RISCVCPUConfig { > > uint32_t mvendorid; > > uint64_t marchid; > > uint64_t mimpid; > > + bool ext_zimops; > > + bool ext_cfi; > > > > /* Vendor-specific custom extensions */ > > bool ext_XVentanaCondOps;