Re: [PATCH v2 08/16] riscv: kvm: Use generated instruction headers for csr code

From: Charlie Jenkins <thecharlesjenkins@gmail.com>
To: Jesse T <mr.bossman075@gmail.com>
Cc: "Paul Walmsley" <pjw@kernel.org>,
	"Palmer Dabbelt" <palmer@dabbelt.com>,
	"Nam Cao" <namcao@linutronix.de>,
	"Alexandre Ghiti" <alex@ghiti.fr>,
	"Anup Patel" <anup@brainfault.org>,
	"Atish Patra" <atish.patra@linux.dev>,
	"Conor Dooley" <conor@kernel.org>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"Shuah Khan" <shuah@kernel.org>,
	"Radim Krčmář" <radim.krcmar@oss.qualcomm.com>,
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
	kvm@vger.kernel.org, kvm-riscv@lists.infradead.org,
	linux-kselftest@vger.kernel.org
Subject: Re: [PATCH v2 08/16] riscv: kvm: Use generated instruction headers for csr code
Date: Mon, 29 Jun 2026 23:17:50 -0700	[thread overview]
Message-ID: <akNfjsbVVcPGNg5a@blinky> (raw)
In-Reply-To: <CAJFTR8T33Yw-wqPFVLe8it2cWjXCz_xPpRCRmoVMQyn7_=UBqw@mail.gmail.com>

On Mon, Jun 29, 2026 at 04:25:53PM -0400, Jesse T wrote:
> On Mon, Jun 22, 2026 at 1:08 AM Charlie Jenkins via B4 Relay
> <devnull+thecharlesjenkins.gmail.com@kernel.org> wrote:
> >
> > From: Charlie Jenkins <thecharlesjenkins@gmail.com>
> >
> > Migrate the csr parsing code to use the generated instruction headers
> > instead of the hand-written instruction composition functions.
> >
> > Signed-off-by: Charlie Jenkins <thecharlesjenkins@gmail.com>
> >
> > ---
> >
> > This is a simple transformation that I have again validated through
> > brute force.
> > ---
> >  arch/riscv/kvm/vcpu_insn.c | 47 +++++++++++++++++++++++-----------------------
> >  1 file changed, 24 insertions(+), 23 deletions(-)
> >
> > diff --git a/arch/riscv/kvm/vcpu_insn.c b/arch/riscv/kvm/vcpu_insn.c
> > index f09f9251d1f0..8ccf6ec722f0 100644
> > --- a/arch/riscv/kvm/vcpu_insn.c
> > +++ b/arch/riscv/kvm/vcpu_insn.c
> > @@ -146,43 +146,44 @@ int kvm_riscv_vcpu_csr_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >
> >  static int csr_insn(struct kvm_vcpu *vcpu, struct kvm_run *run, ulong insn)
> >  {
> > +       #define GET_REG(_rd) (*((unsigned long *)(&vcpu->arch.guest_context) + _rd))
> > +
> 
> IMO it would be better to make this a static inline function, otherwise.

Yeah that's fair, I'll make that change!

> 
> Reviewed-by: Jesse Taube <jtaubepe@redhat.com>
> 
> >         int i, rc = KVM_INSN_ILLEGAL_TRAP;
> > -       unsigned int csr_num = insn >> SH_RS2;
> > -       unsigned int rs1_num = (insn >> SH_RS1) & MASK_RX;
> > -       ulong rs1_val = GET_RS1(insn, &vcpu->arch.guest_context);
> > +       unsigned int csr_num;
> >         const struct csr_func *tcfn, *cfn = NULL;
> >         ulong val = 0, wr_mask = 0, new_val = 0;
> >
> >         /* Decode the CSR instruction */
> > -       switch (GET_FUNCT3(insn)) {
> > -       case GET_FUNCT3(INSN_MATCH_CSRRW):
> > +       if (riscv_insn_is_csrrw(insn)) {
> >                 wr_mask = -1UL;
> > -               new_val = rs1_val;
> > -               break;
> > -       case GET_FUNCT3(INSN_MATCH_CSRRS):
> > -               wr_mask = rs1_val;
> > +               new_val = GET_REG(riscv_insn_csrrw_extract_xs1(insn));
> > +               csr_num = riscv_insn_csrrw_extract_csr(insn);
> > +       } else if (riscv_insn_is_csrrs(insn)) {
> > +               wr_mask = GET_REG(riscv_insn_csrrs_extract_xs1(insn));
> >                 new_val = -1UL;
> > -               break;
> > -       case GET_FUNCT3(INSN_MATCH_CSRRC):
> > -               wr_mask = rs1_val;
> > +               csr_num = riscv_insn_csrrs_extract_csr(insn);
> > +       } else if (riscv_insn_is_csrrc(insn)) {
> > +               wr_mask = GET_REG(riscv_insn_csrrs_extract_xs1(insn));
> >                 new_val = 0;
> > -               break;
> > -       case GET_FUNCT3(INSN_MATCH_CSRRWI):
> > +               csr_num = riscv_insn_csrrc_extract_csr(insn);
> > +       } else if (riscv_insn_is_csrrwi(insn)) {
> >                 wr_mask = -1UL;
> > -               new_val = rs1_num;
> > -               break;
> > -       case GET_FUNCT3(INSN_MATCH_CSRRSI):
> > -               wr_mask = rs1_num;
> > +               new_val = riscv_insn_csrrwi_extract_imm(insn);
> > +               csr_num = riscv_insn_csrrwi_extract_csr(insn);
> > +       } else if (riscv_insn_is_csrrsi(insn)) {
> > +               wr_mask = riscv_insn_csrrwi_extract_imm(insn);
> >                 new_val = -1UL;
> > -               break;
> > -       case GET_FUNCT3(INSN_MATCH_CSRRCI):
> > -               wr_mask = rs1_num;
> > +               csr_num = riscv_insn_csrrsi_extract_csr(insn);
> > +       } else if (riscv_insn_is_csrrci(insn)) {
> > +               wr_mask = GET_REG(riscv_insn_csrrwi_extract_imm(insn));
> >                 new_val = 0;
> > -               break;
> > -       default:
> > +               csr_num = riscv_insn_csrrwi_extract_csr(insn);
> > +       } else {
> >                 return rc;
> >         }
> >
> > +       #undef GET_REG
> > +
> >         /* Save instruction decode info */
> >         vcpu->arch.csr_decode.insn = insn;
> >         vcpu->arch.csr_decode.return_handled = 0;
> >
> > --
> > 2.54.0
> >
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv