[kvmarm] [PATCH v6 14/15] KVM: ARM: Power State Coordination Interface implementation

[Christoffer Dall]

On Mon, Jan 21, 2013 at 1:08 PM, Marc Zyngier <marc.zyngier at arm.com> wrote:
> On Mon, 21 Jan 2013 12:54:22 -0500, Christoffer Dall
> <c.dall at virtualopensystems.com> wrote:
>> On Mon, Jan 21, 2013 at 12:43 PM, Marc Zyngier <marc.zyngier at arm.com>
>> wrote:
>>> On Mon, 21 Jan 2013 09:50:18 -0500, Christoffer Dall
>>> <c.dall at virtualopensystems.com> wrote:
>>>> On Mon, Jan 21, 2013 at 5:04 AM, Marc Zyngier <marc.zyngier at arm.com>
>>> wrote:
>>>>> On Sun, 20 Jan 2013 18:35:51 -0500, Christoffer Dall
>>>>> <c.dall at virtualopensystems.com> wrote:
>>>>>> On Thu, Jan 17, 2013 at 10:55 AM, Marc Zyngier
> <marc.zyngier at arm.com>
>>>>>> wrote:
>>>>>>> On 16/01/13 17:59, Christoffer Dall wrote:
>>>>>>>> From: Marc Zyngier <marc.zyngier at arm.com>
>>>>>>>>
>>>>>>>> Implement the PSCI specification (ARM DEN 0022A) to control
>>>>>>>> virtual CPUs being "powered" on or off.
>>>>>>>>
>>>>>>>> PSCI/KVM is detected using the KVM_CAP_ARM_PSCI capability.
>>>>>>>>
>>>>>>>> A virtual CPU can now be initialized in a "powered off" state,
>>>>>>>> using the KVM_ARM_VCPU_POWER_OFF feature flag.
>>>>>>>>
>>>>>>>> The guest can use either SMC or HVC to execute a PSCI function.
>>>>>>>>
>>>>>>>> Reviewed-by: Will Deacon <will.deacon at arm.com>
>>>>>>>> Signed-off-by: Marc Zyngier <marc.zyngier at arm.com>
>>>>>>>> Signed-off-by: Christoffer Dall <c.dall at virtualopensystems.com>
>>>>>>>
>>>>>>> A few bits went wrong when you reworked this patch. See below.
>>>>>
>>>>> [...]
>>>>>
>>>>>>>> @@ -443,13 +445,17 @@ static int handle_hvc(struct kvm_vcpu *vcpu,
>>>>>>>> struct kvm_run *run)
>>>>>>>>       trace_kvm_hvc(*vcpu_pc(vcpu), *vcpu_reg(vcpu, 0),
>>>>>>>>                     vcpu->arch.hsr & HSR_HVC_IMM_MASK);
>>>>>>>>
>>>>>>>> +     if (kvm_psci_call(vcpu))
>>>>>>>> +             return 1;
>>>>>>>> +
>>>>>>>>       return 1;
>>>>>>>
>>>>>>> No undef injection if there is no PSCI match?
>>>>>
>>>>> You haven't addressed this issue in you patch.
>>>>>
>>>> right, well, it's actually quite nice not having it give you an
>>>> undefined exception when it logs the trace event. The psci protocol
>>>> relies on a confirmation in form of a return value anyhow, so it was
>>>> actually on purpose to remove it, so you can do things like easily
>>>> measure exit times or probe places in the guest.
>>>
>>> If that's for tracing purpose, why don't you allocate another
> hypercall?
>>> Returning to the guest without signalling that nothing was executed
> seems
>>> very wrong to me.
>>>
>> hmm, yeah, maybe you're right.
>>
>> I was just debating with myself whether an undefined isn't too rough.
>> It made sense when we didn't have any kind of handling of hvc, but now
>> an hvc isn't really an undefined instruction, and if we assume that we
>> have a series of hypercalls, multiplexed by a number in r0, but you
>> don't really know what's available on your VM host, it also seems very
>> wrong to have an ABI that says, try it, and if it's not implemented
>> handle the undefined exception.... Know what I mean? perhaps we should
>> return -1 in r0 instead or something.
>
> Aside from the API discovery discussion, my thoughts on the HVC semantics:
> The way I see it, HVC effectively becomes an undefined instruction if the
> upper layer doesn't know about the requested service. This is very
> different from "I know what you mean, but I can't do it now". We should
> really tell the guest "I have no clue what you're talking about", because
> something is utterly wrong. This is a case of not being able to give the VM
> the semantics it expects, and trying to paper over it.
>
> Now, returning something in r0 could have been a possibility if it was
> specified in the ARM ARM, and it is not. So we can already forget about it.
>
well if you use an HVC instruction, you're no longer relying on the
ARM ARM concerning the effects of such an instruction, you're relying
on the assumption of a hyper call ABI that we're discussing right now.
And I think that's an important discussion.

If they way forward is, tell the guest which HVCs are available and if
you do anything else than this we're going to shoot you with an undef,
ok, but for an ABI to inform the guest of an unavailable hypercall,
even though the call was valid on some other host, I think that's
equally awful and something we wish to avoid.

Anyway, in no way high priority and at this point it's probably safer
that we yell than ignore it in silence, so I'm going to put the undef
back right away, I just figured the discussion was important, as I
said.

-Christoffer