Hi James,
From: James Morse [mailto:james.morse@arm.com] Sent: Monday, November 7, 2022 3:31 PM To: Salil Mehta salil.mehta@huawei.com; Jean-Philippe Brucker jean-philippe.brucker@arm.com; Jean-Philippe Brucker jean-philippe@linaro.org; linaro-open-discussions@op-lists.linaro.org Cc: Lorenzo Pieralisi lorenzo.pieralisi@linaro.org; Lorenzo Pieralisi lorenzo.pieralisi@gmail.com; Jonathan Cameron jonathan.cameron@huawei.com; Russell King linux@armlinux.org.uk; mehta.salil.lnk@gmail.com; salil.mehta@opnsrc.net Subject: Re: [Question] Regarding immediate value assigned to 'hypercall.nr' and its corresponding userspace check
Hi Salil,
On 03/11/2022 16:54, Salil Mehta wrote:
Question: We are using HVC conduit for PSCI,
You might be using HVC - its whatever you put in the flags field of the FADT. Don't assume its hardcoded, as for nested-virts virtual-EL2 it needs to be SMC.
Good point. I guess you are talking about using PSCI_USE_HVC flag?
ACPI Specification FADT Subsection: 5.2.9 Fixed ACPI Description Table (FADT) 5.2.9.4 ARM Architecture boot flags
Shouldn’t host check for this flag and then set appropriate 'hypercall.nr'? if so, we would need a change in the KVM host as well?
do we need a check if hypercall being trapped to userspace is SMC or HVC?
Ideally, yes. If the guest is using something other than the conduit you specified for PSCI, it shouldn't get PSCI back.
Sure. I have done the changes but it does not includes HVC/SMC check. I will introduce this check as a separate patch. I have broadly tested the PSCI changes, trapping and exit handling is working correctly with your latest branch. You might want to have a look at[1]:
Repository: [1] https://github.com/salil-mehta/qemu.git virt-cpuhp-armv8/rfc-v1-port29092022.psci
BTW, there is a one liner change in the KVM host(in your latest branch[2]) which is required as well.
[2] https://git.gitlab.arm.com/linux-arm/linux-jm.git virtual_cpu_hotplug/rfc/v0.2
#Hunk
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c index fcab358de228..3c2136cd7a3f 100644 --- a/arch/arm64/kvm/hypercalls.c +++ b/arch/arm64/kvm/hypercalls.c @@ -126,7 +126,7 @@ int kvm_hvc_user(struct kvm_vcpu *vcpu) int i; struct kvm_run *run = vcpu->run;
- if (test_bit(KVM_ARCH_FLAG_HVC_TO_USER, &vcpu->kvm->arch.flags)) { + if (!test_bit(KVM_ARCH_FLAG_HVC_TO_USER, &vcpu->kvm->arch.flags)) { smccc_set_retval(vcpu, SMCCC_RET_NOT_SUPPORTED, 0, 0, 0); return 1; }
File: linux/arch/arm64/kvm/hypercalls.c (your code change) int kvm_hvc_user(struct kvm_vcpu *vcpu) { [...]
run->exit_reason = KVM_EXIT_HYPERCALL; run->hypercall.nr = kvm_vcpu_hvc_get_imm(vcpu); -> this would be referred by userspace code? /* Add the first parameters for fast access. */ [...]
return 0; }
# Userspace Code Excerpt (JPB's June 2021 code reference)
In the userspace, a check similar to below might be required I guess:
/* Under KVM, the PSCI conduit is HVC */
if (imm & KVM_ARM_EXIT_HYPERCALL_SMC) {
XXXX_log_mask("%s: unexpected SMC exit\n", __func__);return 0;}
+/*
- In kvm_run::hypercall::nr, this bit indicates that the call is a SMC. When
- the bit is not set, the call is an HVC.
- */
+#define KVM_ARM_EXIT_HYPERCALL_SMC (1ULL << 63)
I am being lazy but Could you please provide more clarification on the use of 'hypercall.nr' for this check?
Its the immediate as part of the HVC/SMC instruction. PSCI says it should be 0 for PSCI, so anything implementing PSCI should check that. I think HyperV's use of this stuff uses a different immediate - so the register values alone are not enough to prevent collisions.
And perhaps that was the reason to define KVM_ARM_EXIT_HYPERCALL_SMC in the KVM host and use last unused 64th Bit which is being later checked in the userspace?
We might need this change again then at both the paces?
Thanks Salil