Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Reply-To: Gavin Shan <gshan@redhat.com>
Subject: Re: [PATCH v6 02/18] KVM: arm64: Route hypercalls based on their
 owner
To:     Oliver Upton <oupton@google.com>
Cc:     kvmarm@lists.cs.columbia.edu, linux-kernel@vger.kernel.org,
        eauger@redhat.com, Jonathan.Cameron@huawei.com,
        vkuznets@redhat.com, will@kernel.org, shannon.zhaosl@gmail.com,
        james.morse@arm.com, mark.rutland@arm.com, maz@kernel.org,
        pbonzini@redhat.com, shan.gavin@gmail.com
References: <20220403153911.12332-1-gshan@redhat.com>
 <20220403153911.12332-3-gshan@redhat.com> <YmETmWvPPQvHpQwP@google.com>
From:   Gavin Shan <gshan@redhat.com>
Message-ID: <2519e2fa-4d6a-a5f8-1057-6b1820853036@redhat.com>
Date:   Fri, 22 Apr 2022 20:20:50 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.0
MIME-Version: 1.0
In-Reply-To: <YmETmWvPPQvHpQwP@google.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Oliver,

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? :)

>> 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 */

>> +		return kvm_psci_call(vcpu);
>> +	}
>> +
>> +	smccc_set_retval(vcpu, val, 0, 0, 0);
>> +	return 1;
> 
> I don't think any cases of the switch statement change val, could you
> just use SMCCC_RET_NOT_SUPPORTED here?
> 

Yes, Will do in next respin.

>> +}
>> +
>> +static int kvm_hvc_standard_hyp(struct kvm_vcpu *vcpu, u32 func)
>> +{
>> +	u64 val = SMCCC_RET_NOT_SUPPORTED;
>> +	gpa_t gpa;
>> +
>> +	switch (func) {
>>   	case ARM_SMCCC_HV_PV_TIME_FEATURES:
>> -		val[0] = kvm_hypercall_pv_features(vcpu);
>> +		val = kvm_hypercall_pv_features(vcpu);
>>   		break;
>>   	case ARM_SMCCC_HV_PV_TIME_ST:
>>   		gpa = kvm_init_stolen_time(vcpu);
>>   		if (gpa != GPA_INVALID)
>> -			val[0] = gpa;
>> +			val = gpa;
>>   		break;
>> +	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.

>> +
>> +int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
>> +{
>> +	u32 func = smccc_get_function(vcpu);
>> +	u64 val = SMCCC_RET_NOT_SUPPORTED;
>> +
>> +	switch (ARM_SMCCC_OWNER_NUM(func)) {
>> +	case ARM_SMCCC_OWNER_ARCH:
>> +		return kvm_hvc_arch(vcpu, func);
>> +	case ARM_SMCCC_OWNER_STANDARD:
>> +		return kvm_hvc_standard(vcpu, func);
>> +	case ARM_SMCCC_OWNER_STANDARD_HYP:
>> +		return kvm_hvc_standard_hyp(vcpu, func);
>> +	case ARM_SMCCC_OWNER_VENDOR_HYP:
>> +		return kvm_hvc_vendor_hyp(vcpu, func);
>> +	}
>> +
>> +	smccc_set_retval(vcpu, val, 0, 0, 0);
> 
> Same here, avoid indirecting the return value through a local variable.
> 

Sure, will do in next respin.

Thanks,
Gavin