All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Target CPU=Host implementation for KVM ARM/ARM64
@ 2013-09-11 12:59 Anup Patel
  2013-09-11 12:59 ` [PATCH 1/2] ARM/ARM64: KVM: Implement KVM_ARM_SUITABLE_TARGET ioctl Anup Patel
  2013-09-11 12:59 ` [PATCH 2/2] KVM: Add documentation for " Anup Patel
  0 siblings, 2 replies; 7+ messages in thread
From: Anup Patel @ 2013-09-11 12:59 UTC (permalink / raw)
  To: linux-arm-kernel

It will be very useful for user space if it has a method of querying
VCPU target type suitable to underlying Host. We can use such querying
mechanism and implement machine models in user space where VCPU target
type is always same-as/suitable-to underlying Host (i.e. Target CPU=Host).

This patch series implements KVM_ARM_SUITABLE_TARGET ioclt for querying
VCPU target type matching underlying host. Using this new ioctl we can
implement VCPU target CPU=Host in user space (i.e. QEMU/KVMTOOL).

Also, it is not mandatory to call KVM_ARM_SUITABLE_TARGET ioctl and the 
old method of using only KVM_ARM_VCPU_INIT ioctl works fine.

Anup Patel (2):
  ARM/ARM64: KVM: Implement KVM_ARM_SUITABLE_TARGET ioctl
  KVM: Add documentation for KVM_ARM_SUITABLE_TARGET ioctl

 Documentation/virtual/kvm/api.txt |   25 ++++++++++++++++++++-----
 arch/arm/kvm/arm.c                |    2 ++
 include/uapi/linux/kvm.h          |    1 +
 3 files changed, 23 insertions(+), 5 deletions(-)

-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] ARM/ARM64: KVM: Implement KVM_ARM_SUITABLE_TARGET ioctl
  2013-09-11 12:59 [PATCH 0/2] Target CPU=Host implementation for KVM ARM/ARM64 Anup Patel
@ 2013-09-11 12:59 ` Anup Patel
  2013-09-11 18:09   ` Christoffer Dall
  2013-09-11 12:59 ` [PATCH 2/2] KVM: Add documentation for " Anup Patel
  1 sibling, 1 reply; 7+ messages in thread
From: Anup Patel @ 2013-09-11 12:59 UTC (permalink / raw)
  To: linux-arm-kernel

For implementing CPU=host, we need a mechanism for querying
VCPU target type suitable to underlying Host.

This patch implements KVM_ARM_SUITABLE_TARGET ioctl which
return output of kvm_arm_target() to user space for KVM ARM
and KVM ARM64.

Signed-off-by: Anup Patel <anup.patel@linaro.org>
Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
---
 arch/arm/kvm/arm.c       |    2 ++
 include/uapi/linux/kvm.h |    1 +
 2 files changed, 3 insertions(+)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 9c697db..4bdc2e1 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -715,6 +715,8 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		return kvm_vcpu_set_target(vcpu, &init);
 
 	}
+	case KVM_ARM_SUITABLE_TARGET:
+		return kvm_target_cpu();
 	case KVM_SET_ONE_REG:
 	case KVM_GET_ONE_REG: {
 		struct kvm_one_reg reg;
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 99c2533..8285daa 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1012,6 +1012,7 @@ struct kvm_s390_ucas_mapping {
 /* VM is being stopped by host */
 #define KVM_KVMCLOCK_CTRL	  _IO(KVMIO,   0xad)
 #define KVM_ARM_VCPU_INIT	  _IOW(KVMIO,  0xae, struct kvm_vcpu_init)
+#define KVM_ARM_SUITABLE_TARGET	  _IO(KVMIO,   0xaf)
 #define KVM_GET_REG_LIST	  _IOWR(KVMIO, 0xb0, struct kvm_reg_list)
 
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] KVM: Add documentation for KVM_ARM_SUITABLE_TARGET ioctl
  2013-09-11 12:59 [PATCH 0/2] Target CPU=Host implementation for KVM ARM/ARM64 Anup Patel
  2013-09-11 12:59 ` [PATCH 1/2] ARM/ARM64: KVM: Implement KVM_ARM_SUITABLE_TARGET ioctl Anup Patel
@ 2013-09-11 12:59 ` Anup Patel
  2013-09-11 18:09   ` Christoffer Dall
  1 sibling, 1 reply; 7+ messages in thread
From: Anup Patel @ 2013-09-11 12:59 UTC (permalink / raw)
  To: linux-arm-kernel

To implement CPU=Host we have added KVM_ARM_SUITABLE_TARGET ioctl
which provides a CPU target type to user space for creating VCPU
matching underlying Host.

This patch adds info related to this new KVM_ARM_SUITABLE_TARGET
ioctl in the KVM API documentation.

Signed-off-by: Anup Patel <anup.patel@linaro.org>
Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>+
---
 Documentation/virtual/kvm/api.txt |   25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index ef925ea..1ae9721 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2283,7 +2283,7 @@ current state.  "addr" is ignored.
 Capability: basic
 Architectures: arm, arm64
 Type: vcpu ioctl
-Parameters: struct struct kvm_vcpu_init (in)
+Parameters: struct kvm_vcpu_init (in)
 Returns: 0 on success; -1 on error
 Errors:
  ?EINVAL: ???the target is unknown, or the combination of features is invalid.
@@ -2303,8 +2303,24 @@ Possible features:
 	- KVM_ARM_VCPU_EL1_32BIT: Starts the CPU in a 32bit mode.
 	  Depends on KVM_CAP_ARM_EL1_32BIT (arm64 only).
 
+4.83 KVM_ARM_SUITABLE_TARGET
 
-4.83 KVM_GET_REG_LIST
+Capability: basic
+Architectures: arm, arm64
+Type: vcpu ioctl
+Parameters: None
+Returns: 0 on success; -1 on error
+Errors:
+ ?EINVAL: ???no suitable target available for the host
+
+This queries KVM for suitable CPU target type which can be emulated by 
+KVM on underlying host. This is not a mandatory API and could be used
+to create VCPUs matching underlying host.
+
+The ioctl returns a target type which can be directly passed-back to 
+the KVM_ARM_VCPU_INIT ioctl.
+
+4.84 KVM_GET_REG_LIST
 
 Capability: basic
 Architectures: arm, arm64
@@ -2323,8 +2339,7 @@ struct kvm_reg_list {
 This ioctl returns the guest registers that are supported for the
 KVM_GET_ONE_REG/KVM_SET_ONE_REG calls.
 
-
-4.84 KVM_ARM_SET_DEVICE_ADDR
+4.85 KVM_ARM_SET_DEVICE_ADDR
 
 Capability: KVM_CAP_ARM_SET_DEVICE_ADDR
 Architectures: arm, arm64
@@ -2362,7 +2377,7 @@ must be called after calling KVM_CREATE_IRQCHIP, but before calling
 KVM_RUN on any of the VCPUs.  Calling this ioctl twice for any of the
 base addresses will return -EEXIST.
 
-4.85 KVM_PPC_RTAS_DEFINE_TOKEN
+4.86 KVM_PPC_RTAS_DEFINE_TOKEN
 
 Capability: KVM_CAP_PPC_RTAS
 Architectures: ppc
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] KVM: Add documentation for KVM_ARM_SUITABLE_TARGET ioctl
  2013-09-11 12:59 ` [PATCH 2/2] KVM: Add documentation for " Anup Patel
@ 2013-09-11 18:09   ` Christoffer Dall
  2013-09-11 18:42     ` Anup Patel
  0 siblings, 1 reply; 7+ messages in thread
From: Christoffer Dall @ 2013-09-11 18:09 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Sep 11, 2013 at 06:29:53PM +0530, Anup Patel wrote:
> To implement CPU=Host we have added KVM_ARM_SUITABLE_TARGET ioctl
> which provides a CPU target type to user space for creating VCPU
> matching underlying Host.
> 
> This patch adds info related to this new KVM_ARM_SUITABLE_TARGET
> ioctl in the KVM API documentation.
> 
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>+
> ---
>  Documentation/virtual/kvm/api.txt |   25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index ef925ea..1ae9721 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2283,7 +2283,7 @@ current state.  "addr" is ignored.
>  Capability: basic
>  Architectures: arm, arm64
>  Type: vcpu ioctl
> -Parameters: struct struct kvm_vcpu_init (in)
> +Parameters: struct kvm_vcpu_init (in)
>  Returns: 0 on success; -1 on error
>  Errors:
>   ?EINVAL: ???the target is unknown, or the combination of features is invalid.
> @@ -2303,8 +2303,24 @@ Possible features:
>  	- KVM_ARM_VCPU_EL1_32BIT: Starts the CPU in a 32bit mode.
>  	  Depends on KVM_CAP_ARM_EL1_32BIT (arm64 only).

unrelated change, can you put in separate patch?

>  
> +4.83 KVM_ARM_SUITABLE_TARGET

hmm, I'm a little uneasy about the use of the word 'suitable'.
KVM_ARM_PREFERRED_TARGET ?

>  
> -4.83 KVM_GET_REG_LIST
> +Capability: basic
> +Architectures: arm, arm64
> +Type: vcpu ioctl

this should be a system ioctl (or at least a VM ioctl) so we can query
the preferred vcpu without creating one.

> +Parameters: None
> +Returns: 0 on success; -1 on error

no parameters and return 0 on success?

I would suggest returning struct kvm_vcpu_init (out) instead.  It makes
for a more flexible API in case we want to say something about the
preferred/supported vcpu features as well in the future.

> +Errors:
> + ?EINVAL: ???no suitable target available for the host

EINVAL is not really a proper error code for this IMHO, because it
indicates the user supplied something incorrect.  How about ENODEV?

> +
> +This queries KVM for suitable CPU target type which can be emulated by 
> +KVM on underlying host. This is not a mandatory API and could be used
> +to create VCPUs matching underlying host.

this last sentece caused me to trip.  Isn't the second part of the
sentence (after the 'and') what you are trying to say in the first
sentece, and what you want to say here is that it is not required to
call this ioctl before calling KVM_ARM_VCPU_INIT?  I'm not sure this
information is required at all, but it doesn't hurt I guess.

> +
> +The ioctl returns a target type which can be directly passed-back to 

s/passed-back/passed back/
or consider revising it to "... which can be used for the target type in
the KVM_ARM_VCPU_INIT ioctl."

> +the KVM_ARM_VCPU_INIT ioctl.
> +
> +4.84 KVM_GET_REG_LIST
>  
>  Capability: basic
>  Architectures: arm, arm64
> @@ -2323,8 +2339,7 @@ struct kvm_reg_list {
>  This ioctl returns the guest registers that are supported for the
>  KVM_GET_ONE_REG/KVM_SET_ONE_REG calls.
>  
> -
> -4.84 KVM_ARM_SET_DEVICE_ADDR
> +4.85 KVM_ARM_SET_DEVICE_ADDR
>  
>  Capability: KVM_CAP_ARM_SET_DEVICE_ADDR
>  Architectures: arm, arm64
> @@ -2362,7 +2377,7 @@ must be called after calling KVM_CREATE_IRQCHIP, but before calling
>  KVM_RUN on any of the VCPUs.  Calling this ioctl twice for any of the
>  base addresses will return -EEXIST.
>  
> -4.85 KVM_PPC_RTAS_DEFINE_TOKEN
> +4.86 KVM_PPC_RTAS_DEFINE_TOKEN
>  
>  Capability: KVM_CAP_PPC_RTAS
>  Architectures: ppc
> -- 
> 1.7.9.5
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] ARM/ARM64: KVM: Implement KVM_ARM_SUITABLE_TARGET ioctl
  2013-09-11 12:59 ` [PATCH 1/2] ARM/ARM64: KVM: Implement KVM_ARM_SUITABLE_TARGET ioctl Anup Patel
@ 2013-09-11 18:09   ` Christoffer Dall
  0 siblings, 0 replies; 7+ messages in thread
From: Christoffer Dall @ 2013-09-11 18:09 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Sep 11, 2013 at 06:29:52PM +0530, Anup Patel wrote:
> For implementing CPU=host, we need a mechanism for querying
> VCPU target type suitable to underlying Host.
> 
> This patch implements KVM_ARM_SUITABLE_TARGET ioctl which
> return output of kvm_arm_target() to user space for KVM ARM
> and KVM ARM64.
> 
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> ---
>  arch/arm/kvm/arm.c       |    2 ++
>  include/uapi/linux/kvm.h |    1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 9c697db..4bdc2e1 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -715,6 +715,8 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  		return kvm_vcpu_set_target(vcpu, &init);
>  
>  	}
> +	case KVM_ARM_SUITABLE_TARGET:
> +		return kvm_target_cpu();

as I commented on the other patch I think we should try to think of a
better name and this shouldn't be a VCPU ioctl.

>  	case KVM_SET_ONE_REG:
>  	case KVM_GET_ONE_REG: {
>  		struct kvm_one_reg reg;
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 99c2533..8285daa 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1012,6 +1012,7 @@ struct kvm_s390_ucas_mapping {
>  /* VM is being stopped by host */
>  #define KVM_KVMCLOCK_CTRL	  _IO(KVMIO,   0xad)
>  #define KVM_ARM_VCPU_INIT	  _IOW(KVMIO,  0xae, struct kvm_vcpu_init)
> +#define KVM_ARM_SUITABLE_TARGET	  _IO(KVMIO,   0xaf)
>  #define KVM_GET_REG_LIST	  _IOWR(KVMIO, 0xb0, struct kvm_reg_list)
>  
>  #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
> -- 
> 1.7.9.5
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/2] KVM: Add documentation for KVM_ARM_SUITABLE_TARGET ioctl
  2013-09-11 18:09   ` Christoffer Dall
@ 2013-09-11 18:42     ` Anup Patel
  2013-09-11 19:15       ` Christoffer Dall
  0 siblings, 1 reply; 7+ messages in thread
From: Anup Patel @ 2013-09-11 18:42 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Sep 11, 2013 at 11:39 PM, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> On Wed, Sep 11, 2013 at 06:29:53PM +0530, Anup Patel wrote:
>> To implement CPU=Host we have added KVM_ARM_SUITABLE_TARGET ioctl
>> which provides a CPU target type to user space for creating VCPU
>> matching underlying Host.
>>
>> This patch adds info related to this new KVM_ARM_SUITABLE_TARGET
>> ioctl in the KVM API documentation.
>>
>> Signed-off-by: Anup Patel <anup.patel@linaro.org>
>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>+
>> ---
>>  Documentation/virtual/kvm/api.txt |   25 ++++++++++++++++++++-----
>>  1 file changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>> index ef925ea..1ae9721 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -2283,7 +2283,7 @@ current state.  "addr" is ignored.
>>  Capability: basic
>>  Architectures: arm, arm64
>>  Type: vcpu ioctl
>> -Parameters: struct struct kvm_vcpu_init (in)
>> +Parameters: struct kvm_vcpu_init (in)
>>  Returns: 0 on success; -1 on error
>>  Errors:
>>    EINVAL:    the target is unknown, or the combination of features is invalid.
>> @@ -2303,8 +2303,24 @@ Possible features:
>>       - KVM_ARM_VCPU_EL1_32BIT: Starts the CPU in a 32bit mode.
>>         Depends on KVM_CAP_ARM_EL1_32BIT (arm64 only).
>
> unrelated change, can you put in separate patch?

Ok

>
>>
>> +4.83 KVM_ARM_SUITABLE_TARGET
>
> hmm, I'm a little uneasy about the use of the word 'suitable'.
> KVM_ARM_PREFERRED_TARGET ?

Sure, I will rename it to KVM_ARM_PREFERRED_TARGET.

>
>>
>> -4.83 KVM_GET_REG_LIST
>> +Capability: basic
>> +Architectures: arm, arm64
>> +Type: vcpu ioctl
>
> this should be a system ioctl (or at least a VM ioctl) so we can query
> the preferred vcpu without creating one.

I was really confused on this one because the ioctl is all about VCPU
target type but I am fine calling it system (or VM) ioctl.

>
>> +Parameters: None
>> +Returns: 0 on success; -1 on error
>
> no parameters and return 0 on success?

Typo from my-side.
It returns positive value upon success.

>
> I would suggest returning struct kvm_vcpu_init (out) instead.  It makes
> for a more flexible API in case we want to say something about the
> preferred/supported vcpu features as well in the future.

I started-off with an implementation which returned struct kvm_vcpu_init
but there will be features such POWER_OFF which be only decided by
user space.

For e.g. SMP Guest might start VCPU0 in POWER_ON whereas other
VCPUs in POWER_OFF. We cannot suggest anything about POWER
state of VCPU to user space from KVM because we really dont know
what machine model is emulated by user space.

IMHO, the only thing that seems worth returning is the VCPU target
type hence this implementation.

>
>> +Errors:
>> +  EINVAL:    no suitable target available for the host
>
> EINVAL is not really a proper error code for this IMHO, because it
> indicates the user supplied something incorrect.  How about ENODEV?

Ok, I can change this.

I kept EINVAL because I did not want to make changes in
kvm_target_cpu().

>
>> +
>> +This queries KVM for suitable CPU target type which can be emulated by
>> +KVM on underlying host. This is not a mandatory API and could be used
>> +to create VCPUs matching underlying host.
>
> this last sentece caused me to trip.  Isn't the second part of the
> sentence (after the 'and') what you are trying to say in the first
> sentece, and what you want to say here is that it is not required to
> call this ioctl before calling KVM_ARM_VCPU_INIT?  I'm not sure this
> information is required at all, but it doesn't hurt I guess.

Ok, let me try to re-phrase this.

>
>> +
>> +The ioctl returns a target type which can be directly passed-back to
>
> s/passed-back/passed back/
> or consider revising it to "... which can be used for the target type in
> the KVM_ARM_VCPU_INIT ioctl."

Ok

>
>> +the KVM_ARM_VCPU_INIT ioctl.
>> +
>> +4.84 KVM_GET_REG_LIST
>>
>>  Capability: basic
>>  Architectures: arm, arm64
>> @@ -2323,8 +2339,7 @@ struct kvm_reg_list {
>>  This ioctl returns the guest registers that are supported for the
>>  KVM_GET_ONE_REG/KVM_SET_ONE_REG calls.
>>
>> -
>> -4.84 KVM_ARM_SET_DEVICE_ADDR
>> +4.85 KVM_ARM_SET_DEVICE_ADDR
>>
>>  Capability: KVM_CAP_ARM_SET_DEVICE_ADDR
>>  Architectures: arm, arm64
>> @@ -2362,7 +2377,7 @@ must be called after calling KVM_CREATE_IRQCHIP, but before calling
>>  KVM_RUN on any of the VCPUs.  Calling this ioctl twice for any of the
>>  base addresses will return -EEXIST.
>>
>> -4.85 KVM_PPC_RTAS_DEFINE_TOKEN
>> +4.86 KVM_PPC_RTAS_DEFINE_TOKEN
>>
>>  Capability: KVM_CAP_PPC_RTAS
>>  Architectures: ppc
>> --
>> 1.7.9.5
>>
> _______________________________________________
> kvmarm mailing list
> kvmarm at lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/2] KVM: Add documentation for KVM_ARM_SUITABLE_TARGET ioctl
  2013-09-11 18:42     ` Anup Patel
@ 2013-09-11 19:15       ` Christoffer Dall
  0 siblings, 0 replies; 7+ messages in thread
From: Christoffer Dall @ 2013-09-11 19:15 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Sep 12, 2013 at 12:12:55AM +0530, Anup Patel wrote:
> On Wed, Sep 11, 2013 at 11:39 PM, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> > On Wed, Sep 11, 2013 at 06:29:53PM +0530, Anup Patel wrote:
> >> To implement CPU=Host we have added KVM_ARM_SUITABLE_TARGET ioctl
> >> which provides a CPU target type to user space for creating VCPU
> >> matching underlying Host.
> >>
> >> This patch adds info related to this new KVM_ARM_SUITABLE_TARGET
> >> ioctl in the KVM API documentation.
> >>
> >> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> >> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>+
> >> ---
> >>  Documentation/virtual/kvm/api.txt |   25 ++++++++++++++++++++-----
> >>  1 file changed, 20 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> >> index ef925ea..1ae9721 100644
> >> --- a/Documentation/virtual/kvm/api.txt
> >> +++ b/Documentation/virtual/kvm/api.txt
> >> @@ -2283,7 +2283,7 @@ current state.  "addr" is ignored.
> >>  Capability: basic
> >>  Architectures: arm, arm64
> >>  Type: vcpu ioctl
> >> -Parameters: struct struct kvm_vcpu_init (in)
> >> +Parameters: struct kvm_vcpu_init (in)
> >>  Returns: 0 on success; -1 on error
> >>  Errors:
> >>    EINVAL:    the target is unknown, or the combination of features is invalid.
> >> @@ -2303,8 +2303,24 @@ Possible features:
> >>       - KVM_ARM_VCPU_EL1_32BIT: Starts the CPU in a 32bit mode.
> >>         Depends on KVM_CAP_ARM_EL1_32BIT (arm64 only).
> >
> > unrelated change, can you put in separate patch?
> 
> Ok
> 
> >
> >>
> >> +4.83 KVM_ARM_SUITABLE_TARGET
> >
> > hmm, I'm a little uneasy about the use of the word 'suitable'.
> > KVM_ARM_PREFERRED_TARGET ?
> 
> Sure, I will rename it to KVM_ARM_PREFERRED_TARGET.
> 
> >
> >>
> >> -4.83 KVM_GET_REG_LIST
> >> +Capability: basic
> >> +Architectures: arm, arm64
> >> +Type: vcpu ioctl
> >
> > this should be a system ioctl (or at least a VM ioctl) so we can query
> > the preferred vcpu without creating one.
> 
> I was really confused on this one because the ioctl is all about VCPU
> target type but I am fine calling it system (or VM) ioctl.
> 

the 'Type' goes to which type of file descriptor the ioctl is called on.
After I wrote the comment I realized you don't technically need the type
of the CPU before creating a vcpu fd, but I think it's nicer this way,
so user space can just tell at the earliest state what this particular
KVM instance prefers.

> >
> >> +Parameters: None
> >> +Returns: 0 on success; -1 on error
> >
> > no parameters and return 0 on success?
> 
> Typo from my-side.
> It returns positive value upon success.
> 
> >
> > I would suggest returning struct kvm_vcpu_init (out) instead.  It makes
> > for a more flexible API in case we want to say something about the
> > preferred/supported vcpu features as well in the future.
> 
> I started-off with an implementation which returned struct kvm_vcpu_init
> but there will be features such POWER_OFF which be only decided by
> user space.
> 
> For e.g. SMP Guest might start VCPU0 in POWER_ON whereas other
> VCPUs in POWER_OFF. We cannot suggest anything about POWER
> state of VCPU to user space from KVM because we really dont know
> what machine model is emulated by user space.
> 
> IMHO, the only thing that seems worth returning is the VCPU target
> type hence this implementation.
> 

I agree as things are looking right now, but I wonder if we would later
add cpu features like an emulated SMC environment and we want to be able
to return that information without having to come up with yet another
ioctl.  Are there any downsides in returning the kvm_vcpu_init struct
and just specify here in the docs that the features field does not
return anything useful (yet)?

> >
> >> +Errors:
> >> +  EINVAL:    no suitable target available for the host
> >
> > EINVAL is not really a proper error code for this IMHO, because it
> > indicates the user supplied something incorrect.  How about ENODEV?
> 
> Ok, I can change this.
> 
> I kept EINVAL because I did not want to make changes in
> kvm_target_cpu().
> 

ah, ok, yeah, the EINVAL tends to be too overloaded inside the kernel
and I guess I fell into that trap as well.  If it doesn't make the code
too ugly, I think another error would be bette for a user space API.

Should be fixable with something like

return (ret == -EINVAL) ? -ENODEV : ret;

> >
> >> +
> >> +This queries KVM for suitable CPU target type which can be emulated by
> >> +KVM on underlying host. This is not a mandatory API and could be used
> >> +to create VCPUs matching underlying host.
> >
> > this last sentece caused me to trip.  Isn't the second part of the
> > sentence (after the 'and') what you are trying to say in the first
> > sentece, and what you want to say here is that it is not required to
> > call this ioctl before calling KVM_ARM_VCPU_INIT?  I'm not sure this
> > information is required at all, but it doesn't hurt I guess.
> 
> Ok, let me try to re-phrase this.
> 
> >
> >> +
> >> +The ioctl returns a target type which can be directly passed-back to
> >
> > s/passed-back/passed back/
> > or consider revising it to "... which can be used for the target type in
> > the KVM_ARM_VCPU_INIT ioctl."
> 
> Ok
> 
> >
> >> +the KVM_ARM_VCPU_INIT ioctl.
> >> +
> >> +4.84 KVM_GET_REG_LIST
> >>
> >>  Capability: basic
> >>  Architectures: arm, arm64
> >> @@ -2323,8 +2339,7 @@ struct kvm_reg_list {
> >>  This ioctl returns the guest registers that are supported for the
> >>  KVM_GET_ONE_REG/KVM_SET_ONE_REG calls.
> >>
> >> -
> >> -4.84 KVM_ARM_SET_DEVICE_ADDR
> >> +4.85 KVM_ARM_SET_DEVICE_ADDR
> >>
> >>  Capability: KVM_CAP_ARM_SET_DEVICE_ADDR
> >>  Architectures: arm, arm64
> >> @@ -2362,7 +2377,7 @@ must be called after calling KVM_CREATE_IRQCHIP, but before calling
> >>  KVM_RUN on any of the VCPUs.  Calling this ioctl twice for any of the
> >>  base addresses will return -EEXIST.
> >>
> >> -4.85 KVM_PPC_RTAS_DEFINE_TOKEN
> >> +4.86 KVM_PPC_RTAS_DEFINE_TOKEN
> >>
> >>  Capability: KVM_CAP_PPC_RTAS
> >>  Architectures: ppc
> >> --
> >> 1.7.9.5
> >>
> > _______________________________________________
> > kvmarm mailing list
> > kvmarm at lists.cs.columbia.edu
> > https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2013-09-11 19:15 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-09-11 12:59 [PATCH 0/2] Target CPU=Host implementation for KVM ARM/ARM64 Anup Patel
2013-09-11 12:59 ` [PATCH 1/2] ARM/ARM64: KVM: Implement KVM_ARM_SUITABLE_TARGET ioctl Anup Patel
2013-09-11 18:09   ` Christoffer Dall
2013-09-11 12:59 ` [PATCH 2/2] KVM: Add documentation for " Anup Patel
2013-09-11 18:09   ` Christoffer Dall
2013-09-11 18:42     ` Anup Patel
2013-09-11 19:15       ` Christoffer Dall

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.