Re: [PATCH v3 17/17] riscv: Remove unused instruction headers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The mmio_test_destroy() function calls kvm_io_bus_unregister_dev() without holding the required slots_lock.
- [Medium] Missing sign-extension modifier for floating-point memory instruction offsets in insn.tbl.
--

--- Patch [17]: [PATCH v3 17/17] riscv: Remove unused instruction headers ---
commit 4c51ea6677f4f9f14ba5d191936c510b5b961ed2
Author: Charlie Jenkins <thecharlesjenkins@gmail.com>

riscv: Remove unused instruction headers

All usages of hard-coded riscv instruction have been migrated over to
the generated instruction headers so the old macros can be deleted.

> diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h
> index bf6f5d530491b..ea1f139d6d904 100644
> --- a/arch/riscv/include/asm/insn.h
> +++ b/arch/riscv/include/asm/insn.h
[ ... ]
> -#define RV_EXTRACT_UTYPE_IMM(x) \
> -	({typeof(x) x_ = (x); \
> -	(RV_X_MASK(x_, RV_U_IMM_31_12_OPOFF, RV_U_IMM_31_12_MASK)); })
> -
> -#define RV_EXTRACT_JTYPE_IMM(x) \
> -	({typeof(x) x_ = (x); \
> -	(RV_X_MASK(x_, RV_J_IMM_10_1_OPOFF, RV_J_IMM_10_1_MASK) << RV_J_IMM_10_1_OFF) | \
> -	(RV_X_MASK(x_, RV_J_IMM_11_OPOFF, RV_J_IMM_11_MASK) << RV_J_IMM_11_OFF) | \
> -	(RV_X_MASK(x_, RV_J_IMM_19_12_OPOFF, RV_J_IMM_19_12_MASK) << RV_J_IMM_19_12_OFF) | \
> -	(RV_IMM_SIGN(x_) << RV_J_IMM_SIGN_OFF); })
> -
> -#define RV_EXTRACT_ITYPE_IMM(x) \
> -	({typeof(x) x_ = (x); \
> -	(RV_X_MASK(x_, RV_I_IMM_11_0_OPOFF, RV_I_IMM_11_0_MASK)) | \
> -	(RV_IMM_SIGN(x_) << RV_I_IMM_SIGN_OFF); })
> -

[Severity: Medium]
Since we are removing these macros and relying on the generated instruction
headers, do the generated macros correctly handle sign extension?

In arch/riscv/tools/insn.tbl, the floating-point loads and stores (which use
standard I-type and S-type formats with 12-bit signed immediates) are declared
with imm= instead of imm~=:

arch/riscv/tools/insn.tbl:
    ...
    fld common 011<12|0000111<0 imm=31-20 xs1=19-15 fd=11-7
    ...
    fsw common 010<12|0100111<0 imm=31-25|11-7 fs2=24-20 xs1=19-15
    ...

Because the ~ sign-extension modifier is missing, will the generated C
extraction macros (e.g., riscv_insn_fld_extract_imm) incorrectly zero-extend
the immediate offset instead of sign-extending it?

This could mathematically corrupt negative memory offsets (e.g., -8(sp)) into
large unsigned positive numbers. Although no kernel code currently extracts
the immediate for these specific floating-point instructions, this acts as a
latent logical defect.

[ ... ]

[Severity: High]
Does mmio_test_destroy() need to hold the slots_lock here?

When a KVM VM is destroyed, kvm_destroy_devices() invokes mmio_test_destroy(),
which calls kvm_io_bus_unregister_dev().

virt/kvm/mmio_test.c:mmio_test_destroy() {
    ...
    static void mmio_test_destroy(struct kvm_device *dev)
    {
        kvm_io_bus_unregister_dev(dev->kvm, KVM_MMIO_BUS,
                                  &((struct mmio_test *)dev->private)->dev);
    ...
    }

Because kvm_io_bus_unregister_dev() contains a mandatory
lockdep_assert_held(&kvm->slots_lock), and slots_lock is not acquired by
kvm_destroy_devices() or the device destructor, this can trigger a lockdep
splat during VM teardown.

Can this be triggered locally by unprivileged userspace executing KVM
selftests or deliberately creating and closing a VM with the test device,
leading to kernel log spam and lockdep warnings?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-riscv_insn_table-v3-0-3f8e5f75ba97@gmail.com?part=17