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

[Christoffer Dall]

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

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.

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.

-Christoffer