Re: [PATCH v7 17/45] arm64: RME: Handle realm enter/exit

[Gavin Shan]

On 4/8/25 2:34 AM, Steven Price wrote:
> On 04/03/2025 01:03, Gavin Shan wrote:
> > On 2/14/25 2:13 AM, Steven Price wrote:
> > > Entering a realm is done using a SMC call to the RMM. On exit the
> > > exit-codes need to be handled slightly differently to the normal KVM
> > > path so define our own functions for realm enter/exit and hook them
> > > in if the guest is a realm guest.
> > >
> > > Signed-off-by: Steven Price <steven.price@xxxxxxx>
> > > ---
> > > Changes since v6:
> > >    * Use vcpu_err() rather than pr_err/kvm_err when there is an associated
> > >      vcpu to the error.
> > >    * Return -EFAULT for KVM_EXIT_MEMORY_FAULT as per the documentation for
> > >      this exit type.
> > >    * Split code handling a RIPAS change triggered by the guest to the
> > >      following patch.
> > > Changes since v5:
> > >    * For a RIPAS_CHANGE request from the guest perform the actual RIPAS
> > >      change on next entry rather than immediately on the exit. This allows
> > >      the VMM to 'reject' a RIPAS change by refusing to continue
> > >      scheduling.
> > > Changes since v4:
> > >    * Rename handle_rme_exit() to handle_rec_exit()
> > >    * Move the loop to copy registers into the REC enter structure from the
> > >      to rec_exit_handlers callbacks to kvm_rec_enter(). This fixes a bug
> > >      where the handler exits to user space and user space wants to modify
> > >      the GPRS.
> > >    * Some code rearrangement in rec_exit_ripas_change().
> > > Changes since v2:
> > >    * realm_set_ipa_state() now provides an output parameter for the
> > >      top_iap that was changed. Use this to signal the VMM with the correct
> > >      range that has been transitioned.
> > >    * Adapt to previous patch changes.
> > > ---
> > >    arch/arm64/include/asm/kvm_rme.h |   3 +
> > >    arch/arm64/kvm/Makefile          |   2 +-
> > >    arch/arm64/kvm/arm.c             |  19 +++-
> > >    arch/arm64/kvm/rme-exit.c        | 171 +++++++++++++++++++++++++++++++
> > >    arch/arm64/kvm/rme.c             |  19 ++++
> > >    5 files changed, 208 insertions(+), 6 deletions(-)
> > >    create mode 100644 arch/arm64/kvm/rme-exit.c
> >
> > With below nitpicks addressed:
> >
> > Reviewed-by: Gavin Shan <gshan@xxxxxxxxxx>
> >
> > [...]
> >
> > > diff --git a/arch/arm64/kvm/rme-exit.c b/arch/arm64/kvm/rme-exit.c
> > > new file mode 100644
> > > index 000000000000..aae1adefe1a3
> > > --- /dev/null
> > > +++ b/arch/arm64/kvm/rme-exit.c
> > > @@ -0,0 +1,171 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (C) 2023 ARM Ltd.
> > > + */
> > > +
> > > +#include <linux/kvm_host.h>
> > > +#include <kvm/arm_hypercalls.h>
> > > +#include <kvm/arm_psci.h>
> > > +
> > > +#include <asm/rmi_smc.h>
> > > +#include <asm/kvm_emulate.h>
> > > +#include <asm/kvm_rme.h>
> > > +#include <asm/kvm_mmu.h>
> > > +
> > > +typedef int (*exit_handler_fn)(struct kvm_vcpu *vcpu);
> > > +
> >
> > Duplicated to exit_handler_fn, defined in handle_exit.c, need move the
> > definition to header file.
>
> While I get this is duplication, I'm a little reluctant to move it to a
> header file because this is completely internal to each C file (the
> xxx_exit_handler[] arrays are both static). If either side wants to e.g.
> add an extra argument there shouldn't be a requirement to reflect that
> change in the other.
>
> Specifically I'm wondering if we're going to ever need to pass an RMI
> return status into the rme-exit callbacks at some point.

Ok, thanks for explaining it in details. In that case, I think it's fine
to keep exit_handler_fn in rme-exit.c.

> > > +static int rec_exit_reason_notimpl(struct kvm_vcpu *vcpu)
> > > +{
> > > +    struct realm_rec *rec = &vcpu->arch.rec;
> > > +
> > > +    vcpu_err(vcpu, "Unhandled exit reason from realm (ESR: %#llx)\n",
> > > +         rec->run->exit.esr);
> > > +    return -ENXIO;
> > > +}
> > > +
> > > +static int rec_exit_sync_dabt(struct kvm_vcpu *vcpu)
> > > +{
> > > +    return kvm_handle_guest_abort(vcpu);
> > > +}
> > > +
> > > +static int rec_exit_sync_iabt(struct kvm_vcpu *vcpu)
> > > +{
> > > +    struct realm_rec *rec = &vcpu->arch.rec;
> > > +
> > > +    vcpu_err(vcpu, "Unhandled instruction abort (ESR: %#llx).\n",
> > > +         rec->run->exit.esr);
> > > +    return -ENXIO;
> > > +}
> > > +
> > > +static int rec_exit_sys_reg(struct kvm_vcpu *vcpu)
> > > +{
> > > +    struct realm_rec *rec = &vcpu->arch.rec;
> > > +    unsigned long esr = kvm_vcpu_get_esr(vcpu);
> > > +    int rt = kvm_vcpu_sys_get_rt(vcpu);
> > > +    bool is_write = !(esr & 1);
> > > +    int ret;
> > > +
> > > +    if (is_write)
> > > +        vcpu_set_reg(vcpu, rt, rec->run->exit.gprs[0]);
> > > +
> > > +    ret = kvm_handle_sys_reg(vcpu);
> > > +
> > > +    if (ret >= 0 && !is_write)
> > > +        rec->run->enter.gprs[0] = vcpu_get_reg(vcpu, rt);
> > > +
> >
> > Unncessary blank line and the conditon isn't completely correct:
> > kvm_handle_sys_reg()
> > should return 0 if the requested emulation fails, even it always returns
> > 1 for now.
>
> It shouldn't matter, but like you say it's not technically the correct
> condition so I'll fix this up.

Ok.

> Thanks,
> Steve
>
> >      ret = kvm_handle_sys_reg(vcpu);
> >      if (ret > 0 && !is_write)
> >          rec->run->enter.gprs[0] = vcpu_get_reg(vcpu, rt);
> >
> > > +    return ret;
> > > +}
> > > +

Thanks,
Gavin