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

[Anup Patel]

On Mon, Sep 23, 2013 at 9:14 PM, Marc Zyngier <marc.zyngier at arm.com> wrote:
> 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.

Agreed, I will send revised patch where we return zeroed-out features
in struct kvm_vcpu_init (for now).

>
> Cheers,
>
>         M.
> --
> Jazz is not dead. It just smells funny...
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Regards,
Anup