Hi Oliver,
On 4/23/22 1:59 AM, Oliver Upton wrote:
On Fri, Apr 22, 2022 at 08:20:50PM +0800, Gavin Shan wrote:
On 4/21/22 4:19 PM, Oliver Upton wrote:
On Sun, Apr 03, 2022 at 11:38:55PM +0800, Gavin Shan wrote:
kvm_hvc_call_handler() directly handles the incoming hypercall, or
and routes it based on its (function) ID. kvm_psci_call() becomes
the gate keeper to handle the hypercall that can't be handled by
any one else. It makes kvm_hvc_call_handler() a bit messy.
This reorgnizes the code to route the hypercall to the corresponding
handler based on its owner.
nit: write changelogs in the imperative:
Reorganize the code to ...
Thanks again for your review. It will be corrected in next respin.
By the way, could you help to review the rest when you have free
cycles? :)
Yup, I've been thinking on the rest of the series just to make sure the
feedback I give is sane.
Sure.
The hypercall may be handled directly
in the handler or routed further to the corresponding functionality.
The (function) ID is always verified before it's routed to the
corresponding functionality. By the way, @func_id is repalced by
@func, to be consistent with by smccc_get_function().
PSCI is the only exception, those hypercalls defined by 0.2 or
beyond are routed to the handler for Standard Secure Service, but
those defined in 0.1 are routed to the handler for Standard
Hypervisor Service.
Suggested-by: Oliver Upton <oupton@xxxxxxxxxx>
Signed-off-by: Gavin Shan <gshan@xxxxxxxxxx>
---
arch/arm64/kvm/hypercalls.c | 199 +++++++++++++++++++++++-------------
1 file changed, 127 insertions(+), 72 deletions(-)
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index 8438fd79e3f0..b659387d8919 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
[...]
+static int kvm_hvc_standard(struct kvm_vcpu *vcpu, u32 func)
+{
+ u64 val = SMCCC_RET_NOT_SUPPORTED;
+
+ switch (func) {
+ case ARM_SMCCC_TRNG_VERSION ... ARM_SMCCC_TRNG_RND32:
+ case ARM_SMCCC_TRNG_RND64:
+ return kvm_trng_call(vcpu);
+ case PSCI_0_2_FN_PSCI_VERSION ... PSCI_0_2_FN_SYSTEM_RESET:
+ case PSCI_0_2_FN64_CPU_SUSPEND ... PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
+ case PSCI_1_0_FN_PSCI_FEATURES ... PSCI_1_0_FN_SET_SUSPEND_MODE:
+ case PSCI_1_0_FN64_SYSTEM_SUSPEND:
+ case PSCI_1_1_FN_SYSTEM_RESET2:
+ case PSCI_1_1_FN64_SYSTEM_RESET2:
Isn't it known from the SMCCC what range of hypercall numbers PSCI and
TRNG fall under, respectively?
https://developer.arm.com/documentation/den0028/e/
See sections 6.3 and 6.4.
Bit#30 of the function ID is the call convention indication, which is
either 32 or 64-bits. For TRNG's function IDs, its 32-bits and 64-bits
variants are discrete. Besides, the spec reserves more functions IDs
than what range we're using. It means we don't have symbols to match
the reserved ranges. So it looks good to me for TRNG cases.
For PSCI, it can be simplified as below, according to the defination
in include/uapi/linux/psci.h:
case PSCI_0_2_FN_PSCI_VERSION ...
PSCI_1_1_FN_SYSTEM_RESET2: /* 32-bits */
case PSCI_0_2_FN64_CPU_SUSPEND ...
PSCI_1_1_FN64_SYSTEM_RESET2: /* 64-bits */
Right, but this still requires that we go back and update this switch
statement every time we add a new PSCI call, which is exactly what I was
hoping we could avoid. Doing this based exactly on the spec reduces the
burden for future changes, and keeps all relevant context in a single
spot.
#define SMCCC_STD_PSCI_RANGE_START 0x0000
#define SMCCC_STD_PSCI_RANGE_END 0x001f
#define SMCCC_STD_TRNG_RANGE_START 0x0050
#define SMCCC_STD_TRNG_RANGE_END 0x005f
switch (ARM_SMCCC_FUNC_NUM(function_id)) {
case SMCCC_STD_PSCI_RANGE_START ... SMCCC_STD_PSCI_RANGE_END:
return kvm_psci_call(vcpu);
case SMCCC_STD_TRNG_RANGE_START ... SMCCC_STD_TRNG_RANGE_END:
return kvm_trng_call(vcpu);
...
}
Yep, we should avoid to visit and modify this function when a new PSCI call
is added. I intended not to introduce new macros, especially in the header
file (include/linux/arm-smccc.h), which is out of kvm/arm64 scope to some
degree. However, these newly added macros will have life much easier. I will
include the changes in next respin.
+ case KVM_PSCI_FN_CPU_SUSPEND ... KVM_PSCI_FN_MIGRATE:
+ return kvm_psci_call(vcpu);
You might want to handle these from the main call handler with a giant
disclaimer that these values predate SMCCC and therefore collide with
the standard hypervisor service range.
[...]
I probably just keep it as it is to follow the rule: to route
based on the owner strictly. Besides, there are 3 levels to
handle SMCCCs after this patch is applied, which corresponds
to 3 handlers as main/owner/function. It sounds more natural
for reader to follow the implementation in this way.
I think this makes it much more confusing for the reader, as you'd be
hard pressed to find these function IDs in the SMCCC spec. Since their
values are outside of the specification, it is confusing to only address
them after these switch statements have decided that they belong to a
particular service owner as they do not.
Ok. Lets filter these SMCCC PSCI numbers in kvm_hvc_call_handler():
/* Filter these calls that aren't documented in the specification */
if (func >= KVM_PSCI_FN_CPU_SUSPEND && func <= KVM_PSCI_FN_MIGRATE)
return kvm_psci_call(vcpu);
switch (ARM_SMCCC_OWNER_NUM(func)) {
:
}
Thanks,
Gavin
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
