Re: [RFC PATCH 16/28] arm64: RME: Allow populating initial contents

[Steven Price]

On 14/03/2023 15:31, Zhi Wang wrote:
> On Fri, 10 Mar 2023 15:47:16 +0000
> Steven Price <steven.price@xxxxxxx> wrote:
> 
>> On 06/03/2023 17:34, Zhi Wang wrote:
>>> On Fri, 27 Jan 2023 11:29:20 +0000
>>> Steven Price <steven.price@xxxxxxx> 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;  
>>>   
>>
>