[Question] Regarding immediate value assigned to 'hypercall.nr' and its corresponding userspace check
Hi James,
Question: We are using HVC conduit for PSCI, do we need a check if hypercall being trapped to userspace is SMC or HVC?
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?
[1] https://developer.arm.com/documentation/ddi0596/2021-12/Base-Instructions/HV... [HVC #0]
Thanks Salil
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.
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.
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.
Thanks,
James
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
Hi Salil,
On 08/11/2022 13:50, Salil Mehta wrote:
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
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?
Yes,
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?
The host is irrelevant here. This is down to what the VMM described in the ACPI tables to the guest. KVM has no clue whether you said to use HVC or SMC. KVM can (and does!) trap both.
A linux guest will always use 0 as the immediate for PSCI, because that is what the PSCI spec says to use. Another guest may decide to use "super cool vendor hack API", which uses the '1337' immediate, not realising its running as a guest. When the HVC_TO_USER cap is enabled, this will also go to the VMM. The values in x0 may clash with PSCI. The VMM needs to check the immediate.
hypercall.nr is the immediate value the guest used, as its easy for KVM to extract from the ESR.
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]:
It looks like the difference between HVC/SMC isn't exposed via this API. Presumably it would only be SMC once nested-virt is supported, and then HVC would never be seen like this. In which case, we can get away with not checking. This is something to pay attention to when it gets posted on the mailing list, as these things may change!
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; }
Makes sense - thanks! I couldn't test this without the Qemu changes.
+/*
- 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?
I'm not sure if Jean-Philippe changed it, but I think the logic is SMC won't ever be seen by a VMM oustide nested-virt. This is because EL3 may not be implemented, in which case it undef's before it can be trapped. The nested-virt support changes the rules here to ensure SMC can always be trapped from virtual-EL2, even if EL3 isn't implemented.
I think we can rely on there only being one type of hypercall for now. That may change once this reaches the mailing list if this logic turns out to be faulty.
Thanks,
James
linaro-open-discussions@op-lists.linaro.org
-
James Morse -
Salil Mehta