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

[Marc Zyngier]

On 11/01/13 17:48, Christoffer Dall wrote:
> 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.

Sorry, but I do not buy this.

In this particular case, the meaning of the value returned has nothing
to do with the handle_exit convention. We never return to user space,
let alone signaling an error.

Trying to force all the code in this convention makes it actually harder
to review, because this is the exception in the kernel. This is why I
suggest keeping the handle_exit return convention at this exact point,
and not impose it on anything else.

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


-- 
Jazz is not dead. It just smells funny...