| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6d66823e-a6b0-f52a-efe3-0fbf1538597a@redhat.com>
Date: Sat, 23 Apr 2022 20:48:06 +0800
From: Gavin Shan <gshan@...hat.com>
To: Oliver Upton <oupton@...gle.com>
Cc: kvmarm@...ts.cs.columbia.edu, linux-kernel@...r.kernel.org,
eauger@...hat.com, Jonathan.Cameron@...wei.com,
vkuznets@...hat.com, will@...nel.org, shannon.zhaosl@...il.com,
james.morse@....com, mark.rutland@....com, maz@...nel.org,
pbonzini@...hat.com, shan.gavin@...il.com
Subject: Re: [PATCH v6 02/18] KVM: arm64: Route hypercalls based on their
owner
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@...gle.com>
>>>> Signed-off-by: Gavin Shan <gshan@...hat.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
Powered by blists - more mailing lists