On 30/04/2025 12:55, Gavin Shan wrote:
> On 4/16/25 11:41 PM, 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 v7:
>> * A return of 0 from kvm_handle_sys_reg() doesn't mean the register has
>> been read (although that can never happen in the current code). Tidy
>> up the condition to handle any future refactoring.
>> 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 | 170 +++++++++++++++++++++++++++++++
>> arch/arm64/kvm/rme.c | 19 ++++
>> 5 files changed, 207 insertions(+), 6 deletions(-)
>> create mode 100644 arch/arm64/kvm/rme-exit.c
>>
>
> One nitpick below.
>
> Reviewed-by: Gavin Shan <gshan@xxxxxxxxxx>
Thanks
[...]
>> diff --git a/arch/arm64/kvm/rme-exit.c b/arch/arm64/kvm/rme-exit.c
>> new file mode 100644
>> index 000000000000..a1adf5610455
>> --- /dev/null
>> +++ b/arch/arm64/kvm/rme-exit.c
>> @@ -0,0 +1,170 @@
>> +// 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);
>> +
>> +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);
>
> kvm_handle_sys_reg() always returns positive value, so it can be:
>
> if (!is_write)
> rc->run->enter.gprs[0] = vcpu_get_reg(vcpu, rt);
I'm not sure the "always returns positive values" is a good
justification - that might change in the future (although I guess the
only reason it returns '1' is just to have the correct function
signature - so it's unlike to change).
However there isn't really any harm here in setting the 'enter' value
even if it fails. So I'll drop the "ret > 0" check.
Thanks,
Steve
