[kvmarm] [PATCH v2 2/2] ARM: KVM: Power State Coordination Interface implementation

[Christoffer Dall]

On Fri, Jan 11, 2013 at 12:43 PM, Marc Zyngier <marc.zyngier at arm.com> wrote:
> On 11/01/13 17:33, Christoffer Dall wrote:
>> On Fri, Jan 11, 2013 at 12:24 PM, Marc Zyngier <marc.zyngier at arm.com> wrote:
>>> On 11/01/13 17:12, Russell King - ARM Linux wrote:
>>>> On Thu, Jan 10, 2013 at 04:06:45PM +0000, Marc Zyngier wrote:
>>>>> +int kvm_psci_call(struct kvm_vcpu *vcpu)
>>>>> +{
>>>>> +    unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0);
>>>>> +    unsigned long val;
>>>>> +
>>>>> +    switch (psci_fn) {
>>>>> +    case KVM_PSCI_FN_CPU_OFF:
>>>>> +            kvm_psci_vcpu_off(vcpu);
>>>>> +            val = KVM_PSCI_RET_SUCCESS;
>>>>> +            break;
>>>>> +    case KVM_PSCI_FN_CPU_ON:
>>>>> +            val = kvm_psci_vcpu_on(vcpu);
>>>>> +            break;
>>>>> +    case KVM_PSCI_FN_CPU_SUSPEND:
>>>>> +    case KVM_PSCI_FN_MIGRATE:
>>>>> +            val = KVM_PSCI_RET_NI;
>>>>> +            break;
>>>>> +
>>>>> +    default:
>>>>> +            return -1;
>>>>> +    }
>>>>> +
>>>>> +    *vcpu_reg(vcpu, 0) = val;
>>>>> +    return 0;
>>>>> +}
>>>>
>>>> We were discussing recently on #kernel about kernel APIs and the way that
>>>> our integer-returning functions pretty much use 0 for success, and -errno
>>>> for failures, whereas our pointer-returning functions are a mess.
>>>>
>>>> And above we have something returning -1 to some other chunk of code outside
>>>> this compilation unit.  That doesn't sound particularly clever to me.
>>>
>>> The original code used to return -EINVAL, see:
>>> https://lists.cs.columbia.edu/pipermail/kvmarm/2013-January/004509.html
>>>
>>> Christoffer (Cc-ed) didn't like this, hence the -1. I'm happy to revert
>>> the code to its original state though.
>>>
>> I don't want to return -EINVAL, because for the rest of the KVM code
>> this would mean kill the guest.
>>
>> The convention used in other archs of KVM as well as for ARM is that
>> the handle_exit functions return:
>>
>> -ERRNO: Error, report this error to user space
>> 0: Everything is fine, but return to user space to let it do I/O
>> emulation and whatever it wants to do
>> 1: Everything is fine, return directly to the guest without going to user space
>
> That is assuming we propagate the handle_exit convention down to the
> leaf calls, and I object to that. The 3 possible values only apply to
> handle_exit, and we should keep that convention as local as possible,
> because this is the odd case.

I don't agree - it requires you to carefully follow a potentially deep
call trace to make sense of what it does, and worse, it's directly
misleading in the case of KVM/ARM. So either change it everywhere and
have a consistent calling convention or adhere to what we do elsewhere
and use the bool.

>
>> And then you do:
>> if (handle_something() == 0)
>>     return 1;
>>
>> which I thought was confusing, so I said make the function a bool, to
>> avoid the confusion, like Rusty did for all the coprocessor emulation
>> functions.
>
> I don't see a compelling reason to propagate this convention to areas
> that do not require it. In the PSCI case, we have a basic
> handled/not-handled state, the later indicating the reason. The exit
> handling functions can convert the error codes to whatever the run loop
> requires.
>

again, that's why I suggest returning a bool instead. You just said
it: it's a basic handled/not-handled state. Why do you want to return
-EINVAL if that's not propogated anywhere?

If the return codes are local and do not map reasonably to error codes
and more complicated than a bool, I would vote for a #define
HVC_HANDLE_SUCCESS, HVC_HANDLE_ERROR, HVC_HANDLE_XXXXXX, ...



>> There are obviously other ways to handle the "return 1" case, like
>> having an extra state that you carry around, and we can change all the
>> code to do that, but I just don't think it's worth it, as we are in
>> fact quite close to the existing kernel API.
>