diff mbox

[RFC,V4,5/6] target-arm: Common kvm_arm_vcpu_init() for KVM ARM and KVM ARM64

Message ID 1399280445-25345-1-git-send-email-pranavkumar@linaro.org
State New
Headers show

Commit Message

PranavkumarSawargaonkar May 5, 2014, 9 a.m. UTC 
Introduce a common kvm_arm_vcpu_init() for doing KVM_ARM_VCPU_INIT
ioctl in KVM ARM and KVM ARM64. This also helps us factor-out few
common code lines from kvm_arch_init_vcpu() for KVM ARM/ARM64.

Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
Signed-off-by: Anup Patel <anup.patel@linaro.org>
---
 target-arm/kvm.c     |   23 +++++++++++++++++++++++
 target-arm/kvm32.c   |   18 +++---------------
 target-arm/kvm64.c   |   22 ++++++++--------------
 target-arm/kvm_arm.h |   14 ++++++++++++++
 4 files changed, 48 insertions(+), 29 deletions(-)

Comments

Andreas Färber May 5, 2014, 10:09 a.m. UTC | #1 
Am 05.05.2014 11:00, schrieb Pranavkumar Sawargaonkar:
> Introduce a common kvm_arm_vcpu_init() for doing KVM_ARM_VCPU_INIT
> ioctl in KVM ARM and KVM ARM64. This also helps us factor-out few
> common code lines from kvm_arch_init_vcpu() for KVM ARM/ARM64.
> 
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> ---
>  target-arm/kvm.c     |   23 +++++++++++++++++++++++
>  target-arm/kvm32.c   |   18 +++---------------
>  target-arm/kvm64.c   |   22 ++++++++--------------
>  target-arm/kvm_arm.h |   14 ++++++++++++++
>  4 files changed, 48 insertions(+), 29 deletions(-)
> 
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 39202d7..55bc3a3 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -27,6 +27,29 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>      KVM_CAP_LAST_INFO
>  };
>  
> +int kvm_arm_vcpu_init(CPUState *cs, uint32_t feature0_extra)

Since this is ARM-specific code and both kvm_arch_* implementations cast
it, you could use an ARMCPU *cpu argument - but as you need cs for the
ioctl, it's no net win, so your/PMM's choice.

Apart from that, refactoring looks fine to me.

Cleaning up the fprintf() moved will hopefully be done independently.

Regards,
Andreas
Peter Maydell May 5, 2014, 10:44 a.m. UTC | #2 
On 5 May 2014 10:00, Pranavkumar Sawargaonkar <pranavkumar@linaro.org> wrote:
> Introduce a common kvm_arm_vcpu_init() for doing KVM_ARM_VCPU_INIT
> ioctl in KVM ARM and KVM ARM64. This also helps us factor-out few
> common code lines from kvm_arch_init_vcpu() for KVM ARM/ARM64.
>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> ---
>  target-arm/kvm.c     |   23 +++++++++++++++++++++++
>  target-arm/kvm32.c   |   18 +++---------------
>  target-arm/kvm64.c   |   22 ++++++++--------------
>  target-arm/kvm_arm.h |   14 ++++++++++++++
>  4 files changed, 48 insertions(+), 29 deletions(-)
>
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 39202d7..55bc3a3 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -27,6 +27,29 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>      KVM_CAP_LAST_INFO
>  };
>
> +int kvm_arm_vcpu_init(CPUState *cs, uint32_t feature0_extra)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    struct kvm_vcpu_init init;
> +
> +    if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE) {
> +        fprintf(stderr, "KVM is not supported for this guest CPU type\n");
> +        return -EINVAL;
> +    }
> +
> +    init.target = cpu->kvm_target;
> +    memset(init.features, 0, sizeof(init.features));
> +    if (cpu->start_powered_off) {
> +        init.features[0] |= 1 << KVM_ARM_VCPU_POWER_OFF;
> +    }
> +    if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PSCI_0_2)) {
> +        init.features[0] |= 1 << KVM_ARM_VCPU_PSCI_0_2;
> +    }
> +    init.features[0] |= feature0_extra;
> +
> +    return kvm_vcpu_ioctl(cs, KVM_ARM_VCPU_INIT, &init);
> +}

I said back in the review comments for v2 of this series that we
didn't need to do all this just for reset. Put the features word in
cpu along with kvm_target:
http://patchwork.ozlabs.org/patch/335900/

thanks
-- PMM
PranavkumarSawargaonkar May 5, 2014, 11:42 a.m. UTC | #3 
Hi Peter,

On 5 May 2014 16:14, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 May 2014 10:00, Pranavkumar Sawargaonkar <pranavkumar@linaro.org> wrote:
>> Introduce a common kvm_arm_vcpu_init() for doing KVM_ARM_VCPU_INIT
>> ioctl in KVM ARM and KVM ARM64. This also helps us factor-out few
>> common code lines from kvm_arch_init_vcpu() for KVM ARM/ARM64.
>>
>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
>> Signed-off-by: Anup Patel <anup.patel@linaro.org>
>> ---
>>  target-arm/kvm.c     |   23 +++++++++++++++++++++++
>>  target-arm/kvm32.c   |   18 +++---------------
>>  target-arm/kvm64.c   |   22 ++++++++--------------
>>  target-arm/kvm_arm.h |   14 ++++++++++++++
>>  4 files changed, 48 insertions(+), 29 deletions(-)
>>
>> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
>> index 39202d7..55bc3a3 100644
>> --- a/target-arm/kvm.c
>> +++ b/target-arm/kvm.c
>> @@ -27,6 +27,29 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>>      KVM_CAP_LAST_INFO
>>  };
>>
>> +int kvm_arm_vcpu_init(CPUState *cs, uint32_t feature0_extra)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(cs);
>> +    struct kvm_vcpu_init init;
>> +
>> +    if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE) {
>> +        fprintf(stderr, "KVM is not supported for this guest CPU type\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    init.target = cpu->kvm_target;
>> +    memset(init.features, 0, sizeof(init.features));
>> +    if (cpu->start_powered_off) {
>> +        init.features[0] |= 1 << KVM_ARM_VCPU_POWER_OFF;
>> +    }
>> +    if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PSCI_0_2)) {
>> +        init.features[0] |= 1 << KVM_ARM_VCPU_PSCI_0_2;
>> +    }
>> +    init.features[0] |= feature0_extra;
>> +
>> +    return kvm_vcpu_ioctl(cs, KVM_ARM_VCPU_INIT, &init);
>> +}
>
> I said back in the review comments for v2 of this series that we
> didn't need to do all this just for reset. Put the features word in
> cpu along with kvm_target:
> http://patchwork.ozlabs.org/patch/335900/

Sure, I will do it.

>
> thanks
> -- PMM
Thanks,
Pranav
Rob Herring May 5, 2014, 1:42 p.m. UTC | #4 
On Mon, May 5, 2014 at 6:42 AM, Pranavkumar Sawargaonkar
<pranavkumar@linaro.org> wrote:
> Hi Peter,
>
> On 5 May 2014 16:14, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 5 May 2014 10:00, Pranavkumar Sawargaonkar <pranavkumar@linaro.org> wrote:
>>> Introduce a common kvm_arm_vcpu_init() for doing KVM_ARM_VCPU_INIT
>>> ioctl in KVM ARM and KVM ARM64. This also helps us factor-out few
>>> common code lines from kvm_arch_init_vcpu() for KVM ARM/ARM64.
>>>
>>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
>>> Signed-off-by: Anup Patel <anup.patel@linaro.org>
>>> ---
>>>  target-arm/kvm.c     |   23 +++++++++++++++++++++++
>>>  target-arm/kvm32.c   |   18 +++---------------
>>>  target-arm/kvm64.c   |   22 ++++++++--------------
>>>  target-arm/kvm_arm.h |   14 ++++++++++++++
>>>  4 files changed, 48 insertions(+), 29 deletions(-)

Rather than add this in 2 places, shouldn't this come after patch 5?

Rob

>>>
>>> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
>>> index 39202d7..55bc3a3 100644
>>> --- a/target-arm/kvm.c
>>> +++ b/target-arm/kvm.c
>>> @@ -27,6 +27,29 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>>>      KVM_CAP_LAST_INFO
>>>  };
>>>
>>> +int kvm_arm_vcpu_init(CPUState *cs, uint32_t feature0_extra)
>>> +{
>>> +    ARMCPU *cpu = ARM_CPU(cs);
>>> +    struct kvm_vcpu_init init;
>>> +
>>> +    if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE) {
>>> +        fprintf(stderr, "KVM is not supported for this guest CPU type\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    init.target = cpu->kvm_target;
>>> +    memset(init.features, 0, sizeof(init.features));
>>> +    if (cpu->start_powered_off) {
>>> +        init.features[0] |= 1 << KVM_ARM_VCPU_POWER_OFF;
>>> +    }
>>> +    if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PSCI_0_2)) {
>>> +        init.features[0] |= 1 << KVM_ARM_VCPU_PSCI_0_2;
>>> +    }
>>> +    init.features[0] |= feature0_extra;
>>> +
>>> +    return kvm_vcpu_ioctl(cs, KVM_ARM_VCPU_INIT, &init);
>>> +}
>>
>> I said back in the review comments for v2 of this series that we
>> didn't need to do all this just for reset. Put the features word in
>> cpu along with kvm_target:
>> http://patchwork.ozlabs.org/patch/335900/
>
> Sure, I will do it.
>
>>
>> thanks
>> -- PMM
> Thanks,
> Pranav
diff mbox

Patch

diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 39202d7..55bc3a3 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -27,6 +27,29 @@  const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
     KVM_CAP_LAST_INFO
 };
 
+int kvm_arm_vcpu_init(CPUState *cs, uint32_t feature0_extra)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    struct kvm_vcpu_init init;
+
+    if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE) {
+        fprintf(stderr, "KVM is not supported for this guest CPU type\n");
+        return -EINVAL;
+    }
+
+    init.target = cpu->kvm_target;
+    memset(init.features, 0, sizeof(init.features));
+    if (cpu->start_powered_off) {
+        init.features[0] |= 1 << KVM_ARM_VCPU_POWER_OFF;
+    }
+    if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PSCI_0_2)) {
+        init.features[0] |= 1 << KVM_ARM_VCPU_PSCI_0_2;
+    }
+    init.features[0] |= feature0_extra;
+
+    return kvm_vcpu_ioctl(cs, KVM_ARM_VCPU_INIT, &init);
+}
+
 bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try,
                                       int *fdarray,
                                       struct kvm_vcpu_init *init)
diff --git a/target-arm/kvm32.c b/target-arm/kvm32.c
index cd9ac03..0034248 100644
--- a/target-arm/kvm32.c
+++ b/target-arm/kvm32.c
@@ -166,7 +166,6 @@  static int compare_u64(const void *a, const void *b)
 
 int kvm_arch_init_vcpu(CPUState *cs)
 {
-    struct kvm_vcpu_init init;
     int i, ret, arraylen;
     uint64_t v;
     struct kvm_one_reg r;
@@ -174,23 +173,12 @@  int kvm_arch_init_vcpu(CPUState *cs)
     struct kvm_reg_list *rlp;
     ARMCPU *cpu = ARM_CPU(cs);
 
-    if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE) {
-        fprintf(stderr, "KVM is not supported for this guest CPU type\n");
-        return -EINVAL;
-    }
-
-    init.target = cpu->kvm_target;
-    memset(init.features, 0, sizeof(init.features));
-    if (cpu->start_powered_off) {
-        init.features[0] |= 1 << KVM_ARM_VCPU_POWER_OFF;
-    }
-    if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PSCI_0_2)) {
-        init.features[0] |= 1 << KVM_ARM_VCPU_PSCI_0_2;
-    }
-    ret = kvm_vcpu_ioctl(cs, KVM_ARM_VCPU_INIT, &init);
+    /* Do KVM_ARM_VCPU_INIT ioctl */
+    ret = kvm_arm_vcpu_init(cs, 0x0);
     if (ret) {
         return ret;
     }
+
     /* Query the kernel to make sure it supports 32 VFP
      * registers: QEMU's "cortex-a15" CPU is always a
      * VFP-D32 core. The simplest way to do this is just
diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index bc6cd74..f7cc3ef 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -77,29 +77,23 @@  bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc)
 
 int kvm_arch_init_vcpu(CPUState *cs)
 {
-    ARMCPU *cpu = ARM_CPU(cs);
-    struct kvm_vcpu_init init;
     int ret;
+    ARMCPU *cpu = ARM_CPU(cs);
 
-    if (cpu->kvm_target == QEMU_KVM_ARM_TARGET_NONE ||
-        !arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
-        fprintf(stderr, "KVM is not supported for this guest CPU type\n");
+    if (!arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
+        fprintf(stderr, "KVM only support Aarch64 CPU type\n");
         return -EINVAL;
     }
 
-    init.target = cpu->kvm_target;
-    memset(init.features, 0, sizeof(init.features));
-    if (cpu->start_powered_off) {
-        init.features[0] |= 1 << KVM_ARM_VCPU_POWER_OFF;
-    }
-    if (kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PSCI_0_2)) {
-        init.features[0] |= 1 << KVM_ARM_VCPU_PSCI_0_2;
+    /* Do KVM_ARM_VCPU_INIT ioctl */
+    ret = kvm_arm_vcpu_init(cs, 0x0);
+    if (ret) {
+        return ret;
     }
-    ret = kvm_vcpu_ioctl(cs, KVM_ARM_VCPU_INIT, &init);
 
     /* TODO : support for save/restore/reset of system regs via tuple list */
 
-    return ret;
+    return 0;
 }
 
 #define AARCH64_CORE_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
diff --git a/target-arm/kvm_arm.h b/target-arm/kvm_arm.h
index 137c567..1889ba1 100644
--- a/target-arm/kvm_arm.h
+++ b/target-arm/kvm_arm.h
@@ -15,6 +15,20 @@ 
 #include "exec/memory.h"
 
 /**
+ * kvm_arm_vcpu_init:
+ * @cs: CPUState
+ * @feature0_extra: additional features
+ *
+ * KVM ARM and KVM ARM64 need to use KVM_ARM_VCPU_INIT ioctl for
+ * init/re-init/reset the VCPU with given feature flags.
+ * This is a common function for doing KVM_ARM_VCPU_INIT ioctl
+ * independent of KVM ARM or KVM ARM64.
+ *
+ * Returns: 0 if success else < 0 error code
+ */
+int kvm_arm_vcpu_init(CPUState *cs, uint32_t feature0_extra);
+
+/**
  * kvm_arm_register_device:
  * @mr: memory region for this device
  * @devid: the KVM device ID