[PATCH v3 1/4] ARM: KVM: Implement kvm_vcpu_preferred_target() function

[Marc Zyngier]

On 23/09/13 16:31, Christoffer Dall wrote:
> On Mon, Sep 23, 2013 at 01:35:20PM +0530, Anup Patel wrote:
>> Hi Christoffer/Marc,
>>
>> On Fri, Sep 20, 2013 at 12:50 AM, Christoffer Dall
>> <christoffer.dall at linaro.org> wrote:
>>> On Thu, Sep 19, 2013 at 03:27:54PM +0100, Marc Zyngier wrote:
>>>> On 19/09/13 14:11, Anup Patel wrote:
>>>>> This patch implements kvm_vcpu_preferred_target() function for
>>>>> KVM ARM which will help us implement KVM_ARM_PREFERRED_TARGET ioctl
>>>>> for user space.
>>>>>
>>>>> Signed-off-by: Anup Patel <anup.patel at linaro.org>
>>>>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar at linaro.org>
>>>>> ---
>>>>>  arch/arm/kvm/guest.c              |   20 ++++++++++++++++++++
>>>>>  arch/arm64/include/asm/kvm_host.h |    1 +
>>>>>  2 files changed, 21 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
>>>>> index 152d036..b407e6c 100644
>>>>> --- a/arch/arm/kvm/guest.c
>>>>> +++ b/arch/arm/kvm/guest.c
>>>>> @@ -222,6 +222,26 @@ int kvm_vcpu_set_target(struct kvm_vcpu *vcpu,
>>>>>     return kvm_reset_vcpu(vcpu);
>>>>>  }
>>>>>
>>>>> +int kvm_vcpu_preferred_target(struct kvm_vcpu_init *init)
>>>>> +{
>>>>> +   int target = kvm_target_cpu();
>>>>> +
>>>>> +   if (target < 0)
>>>>> +           return -ENODEV;
>>>>> +
>>>>> +   memset(init, 0, sizeof(*init));
>>>>> +
>>>>> +   /*
>>>>> +    * For now, we return all optional features are available
>>>>> +    * for preferred target. In future, we might have features
>>>>> +    * available based on underlying host.
>>>>> +    */
>>>>> +   init->target = (__u32)target;
>>>>> +   init->features[0] |= (1 << KVM_ARM_VCPU_POWER_OFF);
>>>>
>>>> I'm in two minds about this feature reporting. I see they serve a
>>>> purpose, but they also duplicate capabilities, which is the standard way
>>>> to advertise what KVM can do.
>>>>
>>>> It means we end up having to sync two reporting mechanism, and I feel
>>>> this is in general a bad idea.
>>>>
>>>> Furthermore, KVM_ARM_VCPU_POWER_OFF is hardly a feature of the HW, but
>>>> rather a firmware emulation thing.
>>>>
>>>> Peter, Christoffer: Thoughts?
>>>>
>>> I wanted to return the full kvm_vcpu_init instead of just a target int,
>>> so we did not have to come up with yet another ioctl if we need to
>>> return more information about the capabilities of the host CPU in the
>>> future.
>>>
>>> But perhaps we can formulate the API, to say only the (currently empty)
>>> following list of features can only be enabled if the corresponding bit
>>> is enabled, or something along those lines.
>>>
>>> -Christoffer
>>> _______________________________________________
>>> kvmarm mailing list
>>> kvmarm at lists.cs.columbia.edu
>>> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
>>
>> Do we stick with current implementation of returning struct kvm_vcpu_init ?
>> OR
>> Do we return struct kvm_vcpu_init with all features set to zero ?
>>
> Let's give Marc a day or two to respond to this one ;)

Are you implying I'm getting slow? ;-)

To answer Anup's question, I would tend to be cautious, and not expose
things for which we already have another API in place. So far, we've
stuck with the KVM approach of having a capability bit for each feature
we enable, and I'm quite happy with that.

So I'm in favour of Christoffer's proposal to return an empty feature
set, and start adding stuff if/when the need arises.

Cheers,

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