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;
>>>
>>
>
