Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3f4dcc1e-7679-0f7c-44ef-6b0824400259@arm.com>
Date:   Wed, 22 Mar 2023 11:51:20 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [RFC PATCH 16/28] arm64: RME: Allow populating initial contents
Content-Language: en-GB
To:     Zhi Wang <zhi.wang.linux@gmail.com>
Cc:     kvm@vger.kernel.org, kvmarm@lists.linux.dev,
        Catalin Marinas <catalin.marinas@arm.com>,
        Marc Zyngier <maz@kernel.org>, Will Deacon <will@kernel.org>,
        James Morse <james.morse@arm.com>,
        Oliver Upton <oliver.upton@linux.dev>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Zenghui Yu <yuzenghui@huawei.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Joey Gouly <joey.gouly@arm.com>,
        Alexandru Elisei <alexandru.elisei@arm.com>,
        Christoffer Dall <christoffer.dall@arm.com>,
        Fuad Tabba <tabba@google.com>, linux-coco@lists.linux.dev
References: <20230127112248.136810-1-suzuki.poulose@arm.com>
 <20230127112932.38045-1-steven.price@arm.com>
 <20230127112932.38045-17-steven.price@arm.com>
 <20230306193439.000048f2@gmail.com>
 <6c6ff608-2314-a49a-84ee-fac883c4e227@arm.com>
 <20230314173121.00006cfb@gmail.com>
From:   Steven Price <steven.price@arm.com>
In-Reply-To: <20230314173121.00006cfb@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 14/03/2023 15:31, Zhi Wang wrote:
> On Fri, 10 Mar 2023 15:47:16 +0000
> Steven Price <steven.price@arm.com> wrote:
> 
>> On 06/03/2023 17:34, Zhi Wang wrote:
>>> On Fri, 27 Jan 2023 11:29:20 +0000
>>> Steven Price <steven.price@arm.com> wrote:

<snip>

>>>> +	if (kvm_realm_state(kvm) != REALM_STATE_NEW)
>>>> +		return -EBUSY;  
>>>
>>> Maybe -EINVAL? The realm hasn't been created (RMI_REALM_CREATE is not called
>>> yet). The userspace shouldn't reach this path.  
>>
>> Well user space can attempt to populate in the ACTIVE state - which is
>> where the idea of 'busy' comes from. Admittedly it's a little confusing
>> when RMI_REALM_CREATE hasn't been called.
>>
>> I'm not particularly bothered about the return code, but it's useful to
>> have a different code to -EINVAL as it's not an invalid argument, but
>> calling at the wrong time. I can't immediately see a better error code
>> though.
>>
> The reason why I feel -EBUSY is little bit off is EBUSY usually indicates
> something is already initialized and currently running, then another
> calling path wanna to operate it. 
> 
> I took a look on the ioctls in arch/arm64/kvm/arm.c. It seems people have
> different opinions for calling execution path at a wrong time:
> 
> For example:
> 
> long kvm_arch_vcpu_ioctl()
> ...
>         case KVM_GET_REG_LIST: {
>                 struct kvm_reg_list __user *user_list = argp;
>                 struct kvm_reg_list reg_list;
>                 unsigned n;
> 
>                 r = -ENOEXEC;
>                 if (unlikely(!kvm_vcpu_initialized(vcpu)))
>                         break;
> 
>                 r = -EPERM;
>                 if (!kvm_arm_vcpu_is_finalized(vcpu))
>                         break;
> 
> If we have to choose one, I prefer -ENOEXEC as -EPERM is stranger. But
> personally my vote goes to -EINVAL.

Ok, I think you've convinced me - I'll change to -EINVAL. It is invalid
use of the API and none of the other error codes seem a great fit.

Although I do wish Linux had more descriptive error codes - I often end
up peppering the kernel with a few printks when using a new API to find
out what I'm doing wrong.

Steve

>> Steve
>>
>>>> +
>>>> +	if (!IS_ALIGNED(args->populate_ipa_base, PAGE_SIZE) ||
>>>> +	    !IS_ALIGNED(args->populate_ipa_size, PAGE_SIZE))
>>>> +		return -EINVAL;
>>>> +
>>>> +	ipa_base = args->populate_ipa_base;
>>>> +	ipa_end = ipa_base + args->populate_ipa_size;
>>>> +
>>>> +	if (ipa_end < ipa_base)
>>>> +		return -EINVAL;
>>>> +
>>>> +	return populate_par_region(kvm, ipa_base, ipa_end);
>>>> +}
>>>> +
>>>>  static int set_ipa_state(struct kvm_vcpu *vcpu,
>>>>  			 unsigned long ipa,
>>>>  			 unsigned long end,
>>>> @@ -748,6 +1102,18 @@ int kvm_realm_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap)
>>>>  		r = kvm_init_ipa_range_realm(kvm, &args);
>>>>  		break;
>>>>  	}
>>>> +	case KVM_CAP_ARM_RME_POPULATE_REALM: {
>>>> +		struct kvm_cap_arm_rme_populate_realm_args args;
>>>> +		void __user *argp = u64_to_user_ptr(cap->args[1]);
>>>> +
>>>> +		if (copy_from_user(&args, argp, sizeof(args))) {
>>>> +			r = -EFAULT;
>>>> +			break;
>>>> +		}
>>>> +
>>>> +		r = kvm_populate_realm(kvm, &args);
>>>> +		break;
>>>> +	}
>>>>  	default:
>>>>  		r = -EINVAL;
>>>>  		break;  
>>>   
>>
>