Messages in this thread | | | Subject | Re: [PATCH v6 02/18] KVM: arm64: Route hypercalls based on their owner | From | Gavin Shan <> | Date | Sat, 23 Apr 2022 20:48:06 +0800 |
| |
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@google.com> >>>> Signed-off-by: Gavin Shan <gshan@redhat.com> >>>> --- >>>> 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
| |