[PATCH] KVM: x86: Extend KVM_SET_VCPU_EVENTS with selective updates

[PATCH] KVM: x86: Extend KVM_SET_VCPU_EVENTS with selective updates

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2901 bytes --]

User space may not want to overwrite asynchronously changing VCPU event
states on write-back. So allow to skip nmi.pending and sipi_vector by
setting corresponding bits in the flags field of kvm_vcpu_events.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 Documentation/kvm/api.txt  |   10 +++++++++-
 arch/x86/include/asm/kvm.h |    4 ++++
 arch/x86/kvm/x86.c         |    9 ++++++---
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/Documentation/kvm/api.txt b/Documentation/kvm/api.txt
index e1a1141..ee07e3a 100644
--- a/Documentation/kvm/api.txt
+++ b/Documentation/kvm/api.txt
@@ -685,7 +685,7 @@ struct kvm_vcpu_events {
 		__u8 pad;
 	} nmi;
 	__u32 sipi_vector;
-	__u32 flags;   /* must be zero */
+	__u32 flags;
 };
 
 4.30 KVM_SET_VCPU_EVENTS
@@ -701,6 +701,14 @@ vcpu.
 
 See KVM_GET_VCPU_EVENTS for the data structure.
 
+Fields that may be modified asynchronously by running VCPUs can be excluded
+from the update. These fields are nmi.pending and sipi_vector. Set the
+corresponding mask bits in the flags field to suppress overwriting their
+current state:
+
+KVM_VCPUEVENT_MASK_NMI_PENDING - do not update nmi.pending
+KVM_VCPUEVENT_MASK_SIPI_VECTOR - do not update sipi_vector
+
 
 5. The kvm_run structure
 
diff --git a/arch/x86/include/asm/kvm.h b/arch/x86/include/asm/kvm.h
index 950df43..acf8585 100644
--- a/arch/x86/include/asm/kvm.h
+++ b/arch/x86/include/asm/kvm.h
@@ -254,6 +254,10 @@ struct kvm_reinject_control {
 	__u8 reserved[31];
 };
 
+/* When set in flags, skip corresponding fields on KVM_SET_VCPU_EVENTS */
+#define KVM_VCPUEVENT_MASK_NMI_PENDING	0x00000001
+#define KVM_VCPUEVENT_MASK_SIPI_VECTOR	0x00000002
+
 /* for KVM_GET/SET_VCPU_EVENTS */
 struct kvm_vcpu_events {
 	struct {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index dd15d7a..368843c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1953,7 +1953,8 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
 static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
 					      struct kvm_vcpu_events *events)
 {
-	if (events->flags)
+	if (events->flags &
+	    ~(KVM_VCPUEVENT_MASK_NMI_PENDING | KVM_VCPUEVENT_MASK_SIPI_VECTOR))
 		return -EINVAL;
 
 	vcpu_load(vcpu);
@@ -1970,10 +1971,12 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
 		kvm_pic_clear_isr_ack(vcpu->kvm);
 
 	vcpu->arch.nmi_injected = events->nmi.injected;
-	vcpu->arch.nmi_pending = events->nmi.pending;
+	if (!(events->flags & KVM_VCPUEVENT_MASK_NMI_PENDING))
+		vcpu->arch.nmi_pending = events->nmi.pending;
 	kvm_x86_ops->set_nmi_mask(vcpu, events->nmi.masked);
 
-	vcpu->arch.sipi_vector = events->sipi_vector;
+	if (!(events->flags & KVM_VCPUEVENT_MASK_SIPI_VECTOR))
+		vcpu->arch.sipi_vector = events->sipi_vector;
 
 	vcpu_put(vcpu);
 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH] KVM: x86: Extend KVM_SET_VCPU_EVENTS with selective updates

[Avi Kivity]

On 12/06/2009 06:55 PM, Jan Kiszka wrote:
> User space may not want to overwrite asynchronously changing VCPU event
> states on write-back. So allow to skip nmi.pending and sipi_vector by
> setting corresponding bits in the flags field of kvm_vcpu_events.
>    

I think a positive flag (do update nmi and sipi_vector) will cause less 
confusion, no?  If we do that, we'll need to set them on 
KVM_GET_VCPU_EVENTS.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: x86: Extend KVM_SET_VCPU_EVENTS with selective updates

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 527 bytes --]

Avi Kivity wrote:
> On 12/06/2009 06:55 PM, Jan Kiszka wrote:
>> User space may not want to overwrite asynchronously changing VCPU event
>> states on write-back. So allow to skip nmi.pending and sipi_vector by
>> setting corresponding bits in the flags field of kvm_vcpu_events.
>>    
> 
> I think a positive flag (do update nmi and sipi_vector) will cause less
> confusion, no?  If we do that, we'll need to set them on
> KVM_GET_VCPU_EVENTS.

I'm fine with the former but I don't understand the latter.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH] KVM: x86: Extend KVM_SET_VCPU_EVENTS with selective updates

[Avi Kivity]

On 12/06/2009 07:12 PM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> On 12/06/2009 06:55 PM, Jan Kiszka wrote:
>>      
>>> User space may not want to overwrite asynchronously changing VCPU event
>>> states on write-back. So allow to skip nmi.pending and sipi_vector by
>>> setting corresponding bits in the flags field of kvm_vcpu_events.
>>>
>>>        
>> I think a positive flag (do update nmi and sipi_vector) will cause less
>> confusion, no?  If we do that, we'll need to set them on
>> KVM_GET_VCPU_EVENTS.
>>      
> I'm fine with the former but I don't understand the latter.
>
>    

What I meant was the on KVM_GET_VCPU_EVENTS, the two flags should be 
set.  This way, the meaning of the flags is identical on both: the 
associated field contains valid data.

I made this change to your v2 and applied and queued for .33.  Please 
review (in next) to make sure it makes sense.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: x86: Extend KVM_SET_VCPU_EVENTS with selective updates

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

Avi Kivity wrote:
> On 12/06/2009 07:12 PM, Jan Kiszka wrote:
>> Avi Kivity wrote:
>>   
>>> On 12/06/2009 06:55 PM, Jan Kiszka wrote:
>>>     
>>>> User space may not want to overwrite asynchronously changing VCPU event
>>>> states on write-back. So allow to skip nmi.pending and sipi_vector by
>>>> setting corresponding bits in the flags field of kvm_vcpu_events.
>>>>
>>>>        
>>> I think a positive flag (do update nmi and sipi_vector) will cause less
>>> confusion, no?  If we do that, we'll need to set them on
>>> KVM_GET_VCPU_EVENTS.
>>>      
>> I'm fine with the former but I don't understand the latter.
>>
>>    
> 
> What I meant was the on KVM_GET_VCPU_EVENTS, the two flags should be
> set.  This way, the meaning of the flags is identical on both: the
> associated field contains valid data.
> 
> I made this change to your v2 and applied and queued for .33.  Please
> review (in next) to make sure it makes sense.
> 

OK, but calling these bits "SET" makes no sense anymore. What about
KVM_VCPUEVENT_VALID_*?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH] KVM: x86: Extend KVM_SET_VCPU_EVENTS with selective updates

[Avi Kivity]

On 12/15/2009 06:41 PM, Jan Kiszka wrote:
>
> OK, but calling these bits "SET" makes no sense anymore. What about
> KVM_VCPUEVENT_VALID_*?
>    

Sure.  Want to patch?

-- 
error compiling committee.c: too many arguments to function

[PATCH] KVM: x86: Adjust KVM_VCPUEVENT flag names

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3408 bytes --]

Avi Kivity wrote:
> On 12/15/2009 06:41 PM, Jan Kiszka wrote:
>>
>> OK, but calling these bits "SET" makes no sense anymore. What about
>> KVM_VCPUEVENT_VALID_*?
>>    
> 
> Sure.  Want to patch?
> 

Here we go:

--------->

KVM: x86: Adjust KVM_VCPUEVENT flag names

They are also used on KVM_GET_VCPU_EVENTS, so 'VALID' is a better name
element than 'SET'.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 Documentation/kvm/api.txt  |    4 ++--
 arch/x86/include/asm/kvm.h |    4 ++--
 arch/x86/kvm/x86.c         |   12 ++++++------
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/Documentation/kvm/api.txt b/Documentation/kvm/api.txt
index b22592d..2811e45 100644
--- a/Documentation/kvm/api.txt
+++ b/Documentation/kvm/api.txt
@@ -706,8 +706,8 @@ from the update. These fields are nmi.pending and sipi_vector. Keep the
 corresponding bits in the flags field cleared to suppress overwriting the
 current in-kernel state. The bits are:
 
-KVM_VCPUEVENT_SET_NMI_PENDING - transfer nmi.pending to the kernel
-KVM_VCPUEVENT_SET_SIPI_VECTOR - transfer sipi_vector
+KVM_VCPUEVENT_VALID_NMI_PENDING - transfer nmi.pending to the kernel
+KVM_VCPUEVENT_VALID_SIPI_VECTOR - transfer sipi_vector
 
 
 5. The kvm_run structure
diff --git a/arch/x86/include/asm/kvm.h b/arch/x86/include/asm/kvm.h
index d1a67ae..f46b79f 100644
--- a/arch/x86/include/asm/kvm.h
+++ b/arch/x86/include/asm/kvm.h
@@ -255,8 +255,8 @@ struct kvm_reinject_control {
 };
 
 /* When set in flags, include corresponding fields on KVM_SET_VCPU_EVENTS */
-#define KVM_VCPUEVENT_SET_NMI_PENDING	0x00000001
-#define KVM_VCPUEVENT_SET_SIPI_VECTOR	0x00000002
+#define KVM_VCPUEVENT_VALID_NMI_PENDING	0x00000001
+#define KVM_VCPUEVENT_VALID_SIPI_VECTOR	0x00000002
 
 /* for KVM_GET/SET_VCPU_EVENTS */
 struct kvm_vcpu_events {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e181c6f..0113752 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1945,8 +1945,8 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
 
 	events->sipi_vector = vcpu->arch.sipi_vector;
 
-	events->flags = (KVM_VCPUEVENT_SET_NMI_PENDING
-			 | KVM_VCPUEVENT_SET_SIPI_VECTOR);
+	events->flags = (KVM_VCPUEVENT_VALID_NMI_PENDING
+			 | KVM_VCPUEVENT_VALID_SIPI_VECTOR);
 
 	vcpu_put(vcpu);
 }
@@ -1954,8 +1954,8 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
 static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
 					      struct kvm_vcpu_events *events)
 {
-	if (events->flags &
-	    ~(KVM_VCPUEVENT_SET_NMI_PENDING | KVM_VCPUEVENT_SET_SIPI_VECTOR))
+	if (events->flags & ~(KVM_VCPUEVENT_VALID_NMI_PENDING
+			      | KVM_VCPUEVENT_VALID_SIPI_VECTOR))
 		return -EINVAL;
 
 	vcpu_load(vcpu);
@@ -1972,11 +1972,11 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
 		kvm_pic_clear_isr_ack(vcpu->kvm);
 
 	vcpu->arch.nmi_injected = events->nmi.injected;
-	if (events->flags & KVM_VCPUEVENT_SET_NMI_PENDING)
+	if (events->flags & KVM_VCPUEVENT_VALID_NMI_PENDING)
 		vcpu->arch.nmi_pending = events->nmi.pending;
 	kvm_x86_ops->set_nmi_mask(vcpu, events->nmi.masked);
 
-	if (events->flags & KVM_VCPUEVENT_SET_SIPI_VECTOR)
+	if (events->flags & KVM_VCPUEVENT_VALID_SIPI_VECTOR)
 		vcpu->arch.sipi_vector = events->sipi_vector;
 
 	vcpu_put(vcpu);


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH] KVM: x86: Adjust KVM_VCPUEVENT flag names

[Avi Kivity]

On 12/16/2009 12:57 AM, Jan Kiszka wrote:
> They are also used on KVM_GET_VCPU_EVENTS, so 'VALID' is a better name
> element than 'SET'.
>
>    

Thanks - applied and queued.

-- 
error compiling committee.c: too many arguments to function

[PATCH v2] KVM: x86: Extend KVM_SET_VCPU_EVENTS with selective updates

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2965 bytes --]

User space may not want to overwrite asynchronously changing VCPU event
states on write-back. So allow to skip nmi.pending and sipi_vector by
setting corresponding bits in the flags field of kvm_vcpu_events.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v2:
 - invert selection logic

 Documentation/kvm/api.txt  |   10 +++++++++-
 arch/x86/include/asm/kvm.h |    4 ++++
 arch/x86/kvm/x86.c         |    9 ++++++---
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/Documentation/kvm/api.txt b/Documentation/kvm/api.txt
index e1a1141..b22592d 100644
--- a/Documentation/kvm/api.txt
+++ b/Documentation/kvm/api.txt
@@ -685,7 +685,7 @@ struct kvm_vcpu_events {
 		__u8 pad;
 	} nmi;
 	__u32 sipi_vector;
-	__u32 flags;   /* must be zero */
+	__u32 flags;
 };
 
 4.30 KVM_SET_VCPU_EVENTS
@@ -701,6 +701,14 @@ vcpu.
 
 See KVM_GET_VCPU_EVENTS for the data structure.
 
+Fields that may be modified asynchronously by running VCPUs can be excluded
+from the update. These fields are nmi.pending and sipi_vector. Keep the
+corresponding bits in the flags field cleared to suppress overwriting the
+current in-kernel state. The bits are:
+
+KVM_VCPUEVENT_SET_NMI_PENDING - transfer nmi.pending to the kernel
+KVM_VCPUEVENT_SET_SIPI_VECTOR - transfer sipi_vector
+
 
 5. The kvm_run structure
 
diff --git a/arch/x86/include/asm/kvm.h b/arch/x86/include/asm/kvm.h
index 950df43..d1a67ae 100644
--- a/arch/x86/include/asm/kvm.h
+++ b/arch/x86/include/asm/kvm.h
@@ -254,6 +254,10 @@ struct kvm_reinject_control {
 	__u8 reserved[31];
 };
 
+/* When set in flags, include corresponding fields on KVM_SET_VCPU_EVENTS */
+#define KVM_VCPUEVENT_SET_NMI_PENDING	0x00000001
+#define KVM_VCPUEVENT_SET_SIPI_VECTOR	0x00000002
+
 /* for KVM_GET/SET_VCPU_EVENTS */
 struct kvm_vcpu_events {
 	struct {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index dd15d7a..e3c35ff 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1953,7 +1953,8 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
 static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
 					      struct kvm_vcpu_events *events)
 {
-	if (events->flags)
+	if (events->flags &
+	    ~(KVM_VCPUEVENT_SET_NMI_PENDING | KVM_VCPUEVENT_SET_SIPI_VECTOR))
 		return -EINVAL;
 
 	vcpu_load(vcpu);
@@ -1970,10 +1971,12 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
 		kvm_pic_clear_isr_ack(vcpu->kvm);
 
 	vcpu->arch.nmi_injected = events->nmi.injected;
-	vcpu->arch.nmi_pending = events->nmi.pending;
+	if (events->flags & KVM_VCPUEVENT_SET_NMI_PENDING)
+		vcpu->arch.nmi_pending = events->nmi.pending;
 	kvm_x86_ops->set_nmi_mask(vcpu, events->nmi.masked);
 
-	vcpu->arch.sipi_vector = events->sipi_vector;
+	if (events->flags & KVM_VCPUEVENT_SET_SIPI_VECTOR)
+		vcpu->arch.sipi_vector = events->sipi_vector;
 
 	vcpu_put(vcpu);
 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH v2] KVM: x86: Extend KVM_SET_VCPU_EVENTS with selective updates

[Marcelo Tosatti]

On Sun, Dec 06, 2009 at 06:24:15PM +0100, Jan Kiszka wrote:
> User space may not want to overwrite asynchronously changing VCPU event
> states on write-back. So allow to skip nmi.pending and sipi_vector by
> setting corresponding bits in the flags field of kvm_vcpu_events.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Can't you handle this in userspace entirely, only updating vcpu_events
state when appropriate? 

Shouldnt the vcpu be stopped in the first place, when its state is
updated?

> ---
> 
> Changes in v2:
>  - invert selection logic
> 
>  Documentation/kvm/api.txt  |   10 +++++++++-
>  arch/x86/include/asm/kvm.h |    4 ++++
>  arch/x86/kvm/x86.c         |    9 ++++++---
>  3 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/kvm/api.txt b/Documentation/kvm/api.txt
> index e1a1141..b22592d 100644
> --- a/Documentation/kvm/api.txt
> +++ b/Documentation/kvm/api.txt
> @@ -685,7 +685,7 @@ struct kvm_vcpu_events {
>  		__u8 pad;
>  	} nmi;
>  	__u32 sipi_vector;
> -	__u32 flags;   /* must be zero */
> +	__u32 flags;
>  };
>  
>  4.30 KVM_SET_VCPU_EVENTS
> @@ -701,6 +701,14 @@ vcpu.
>  
>  See KVM_GET_VCPU_EVENTS for the data structure.
>  
> +Fields that may be modified asynchronously by running VCPUs can be excluded
> +from the update. These fields are nmi.pending and sipi_vector. Keep the
> +corresponding bits in the flags field cleared to suppress overwriting the
> +current in-kernel state. The bits are:
> +
> +KVM_VCPUEVENT_SET_NMI_PENDING - transfer nmi.pending to the kernel
> +KVM_VCPUEVENT_SET_SIPI_VECTOR - transfer sipi_vector
> +
>  
>  5. The kvm_run structure
>  
> diff --git a/arch/x86/include/asm/kvm.h b/arch/x86/include/asm/kvm.h
> index 950df43..d1a67ae 100644
> --- a/arch/x86/include/asm/kvm.h
> +++ b/arch/x86/include/asm/kvm.h
> @@ -254,6 +254,10 @@ struct kvm_reinject_control {
>  	__u8 reserved[31];
>  };
>  
> +/* When set in flags, include corresponding fields on KVM_SET_VCPU_EVENTS */
> +#define KVM_VCPUEVENT_SET_NMI_PENDING	0x00000001
> +#define KVM_VCPUEVENT_SET_SIPI_VECTOR	0x00000002
> +
>  /* for KVM_GET/SET_VCPU_EVENTS */
>  struct kvm_vcpu_events {
>  	struct {
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index dd15d7a..e3c35ff 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1953,7 +1953,8 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
>  static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
>  					      struct kvm_vcpu_events *events)
>  {
> -	if (events->flags)
> +	if (events->flags &
> +	    ~(KVM_VCPUEVENT_SET_NMI_PENDING | KVM_VCPUEVENT_SET_SIPI_VECTOR))
>  		return -EINVAL;
>  
>  	vcpu_load(vcpu);
> @@ -1970,10 +1971,12 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
>  		kvm_pic_clear_isr_ack(vcpu->kvm);
>  
>  	vcpu->arch.nmi_injected = events->nmi.injected;
> -	vcpu->arch.nmi_pending = events->nmi.pending;
> +	if (events->flags & KVM_VCPUEVENT_SET_NMI_PENDING)
> +		vcpu->arch.nmi_pending = events->nmi.pending;
>  	kvm_x86_ops->set_nmi_mask(vcpu, events->nmi.masked);
>  
> -	vcpu->arch.sipi_vector = events->sipi_vector;
> +	if (events->flags & KVM_VCPUEVENT_SET_SIPI_VECTOR)
> +		vcpu->arch.sipi_vector = events->sipi_vector;
>  
>  	vcpu_put(vcpu);
>  
> 

Re: [PATCH v2] KVM: x86: Extend KVM_SET_VCPU_EVENTS with selective updates

[Avi Kivity]

On 12/08/2009 04:02 PM, Marcelo Tosatti wrote:
> On Sun, Dec 06, 2009 at 06:24:15PM +0100, Jan Kiszka wrote:
>    
>> User space may not want to overwrite asynchronously changing VCPU event
>> states on write-back. So allow to skip nmi.pending and sipi_vector by
>> setting corresponding bits in the flags field of kvm_vcpu_events.
>>
>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>      
> Can't you handle this in userspace entirely, only updating vcpu_events
> state when appropriate?
>    

For what we do now I think you're right, it can be handled in userspace.

But in general, there's currently no way to update vcpu_events without 
overwriting nmi and sipi_vector, which can also be written concurrently 
by other vcpus.  So there's a hole in the interface.

> Shouldnt the vcpu be stopped in the first place, when its state is
> updated?
>    

It is stopped, but other vcpus are not.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v2] KVM: x86: Extend KVM_SET_VCPU_EVENTS with selective updates

[Marcelo Tosatti]

On Tue, Dec 08, 2009 at 04:07:32PM +0200, Avi Kivity wrote:
> On 12/08/2009 04:02 PM, Marcelo Tosatti wrote:
>> On Sun, Dec 06, 2009 at 06:24:15PM +0100, Jan Kiszka wrote:
>>    
>>> User space may not want to overwrite asynchronously changing VCPU event
>>> states on write-back. So allow to skip nmi.pending and sipi_vector by
>>> setting corresponding bits in the flags field of kvm_vcpu_events.
>>>
>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>>      
>> Can't you handle this in userspace entirely, only updating vcpu_events
>> state when appropriate?
>>    
>
> For what we do now I think you're right, it can be handled in userspace.
>
> But in general, there's currently no way to update vcpu_events without  
> overwriting nmi and sipi_vector, which can also be written concurrently  
> by other vcpus.  So there's a hole in the interface.
>
>> Shouldnt the vcpu be stopped in the first place, when its state is
>> updated?
>>    
>
> It is stopped, but other vcpus are not.

I don't see the need for setting any state in kvm_vcpu_events
automatically, on kernel entry (apparently there was consensus that
saving similar state explicitly in qemu was the way to go).

kvm_arch_put_registers in qemu saves mpstate now that way,
and the same problem is present.

The sites to load vcpu_events would be machine reset and cpu_load
only, right?

Re: [PATCH v2] KVM: x86: Extend KVM_SET_VCPU_EVENTS with selective updates

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1985 bytes --]

Marcelo Tosatti wrote:
> On Tue, Dec 08, 2009 at 04:07:32PM +0200, Avi Kivity wrote:
>> On 12/08/2009 04:02 PM, Marcelo Tosatti wrote:
>>> On Sun, Dec 06, 2009 at 06:24:15PM +0100, Jan Kiszka wrote:
>>>    
>>>> User space may not want to overwrite asynchronously changing VCPU event
>>>> states on write-back. So allow to skip nmi.pending and sipi_vector by
>>>> setting corresponding bits in the flags field of kvm_vcpu_events.
>>>>
>>>> Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>>>      
>>> Can't you handle this in userspace entirely, only updating vcpu_events
>>> state when appropriate?
>>>    
>> For what we do now I think you're right, it can be handled in userspace.
>>
>> But in general, there's currently no way to update vcpu_events without  
>> overwriting nmi and sipi_vector, which can also be written concurrently  
>> by other vcpus.  So there's a hole in the interface.
>>
>>> Shouldnt the vcpu be stopped in the first place, when its state is
>>> updated?
>>>    
>> It is stopped, but other vcpus are not.
> 
> I don't see the need for setting any state in kvm_vcpu_events
> automatically, on kernel entry (apparently there was consensus that
> saving similar state explicitly in qemu was the way to go).

(I don't think so. IMHO the cleaner way is to avoid loading critical
states unless we are resetting or vmloading.)

> 
> kvm_arch_put_registers in qemu saves mpstate now that way,
> and the same problem is present.
> 
> The sites to load vcpu_events would be machine reset and cpu_load
> only, right?

That is how qemu use it, currently. But this interface should be
designed with more flexibility. For the (yet theoretical) case you want
to update RIP of a single VCPU, you also have to reset all the
context-related states but maybe not the asynchronously changing ones
like nmi.pending. We have no such use case yet, but KVM should not
prevent them by design (if the change is so trivial).

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH v2] KVM: x86: Extend KVM_SET_VCPU_EVENTS with selective updates

[Avi Kivity]

On 12/08/2009 11:17 PM, Jan Kiszka wrote:
>
>> I don't see the need for setting any state in kvm_vcpu_events
>> automatically, on kernel entry (apparently there was consensus that
>> saving similar state explicitly in qemu was the way to go).
>>      
> (I don't think so. IMHO the cleaner way is to avoid loading critical
> states unless we are resetting or vmloading.)
>
>    

I now agree.  But instead of SCOPE_RESET and SCOPE_RUNTIME (or whatever 
that was), how about SCOPE_GPR, SCOPE_FPU, SCOPE_SREGS etc.  That means 
the backing code in kvm.c doesn't have to know what qemu is interested 
in wrt SCOPE_RESET, and it's easier for readers to infer what is meant.

>> kvm_arch_put_registers in qemu saves mpstate now that way,
>> and the same problem is present.
>>
>> The sites to load vcpu_events would be machine reset and cpu_load
>> only, right?
>>      
> That is how qemu use it, currently. But this interface should be
> designed with more flexibility. For the (yet theoretical) case you want
> to update RIP of a single VCPU, you also have to reset all the
> context-related states but maybe not the asynchronously changing ones
> like nmi.pending. We have no such use case yet, but KVM should not
> prevent them by design (if the change is so trivial).
>
>    

Yes.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v2] KVM: x86: Extend KVM_SET_VCPU_EVENTS with selective updates

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

Avi Kivity wrote:
> On 12/08/2009 11:17 PM, Jan Kiszka wrote:
>>
>>> I don't see the need for setting any state in kvm_vcpu_events
>>> automatically, on kernel entry (apparently there was consensus that
>>> saving similar state explicitly in qemu was the way to go).
>>>      
>> (I don't think so. IMHO the cleaner way is to avoid loading critical
>> states unless we are resetting or vmloading.)
>>
>>    
> 
> I now agree.  But instead of SCOPE_RESET and SCOPE_RUNTIME (or whatever
> that was), how about SCOPE_GPR, SCOPE_FPU, SCOPE_SREGS etc.  That means
> the backing code in kvm.c doesn't have to know what qemu is interested
> in wrt SCOPE_RESET, and it's easier for readers to infer what is meant.

That's not my idea. I want to be able to state the scope in generic,
arch-independent, KVM-unaware code. What the scope actually means /wrt
writeback should only be defined in the arch-specific kvm service
implementing it. Your suggestion would go in the wrong direction IMO.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH v2] KVM: x86: Extend KVM_SET_VCPU_EVENTS with selective updates

[Avi Kivity]

On 12/15/2009 06:43 PM, Jan Kiszka wrote:
>
>> I now agree.  But instead of SCOPE_RESET and SCOPE_RUNTIME (or whatever
>> that was), how about SCOPE_GPR, SCOPE_FPU, SCOPE_SREGS etc.  That means
>> the backing code in kvm.c doesn't have to know what qemu is interested
>> in wrt SCOPE_RESET, and it's easier for readers to infer what is meant.
>>      
> That's not my idea. I want to be able to state the scope in generic,
> arch-independent, KVM-unaware code. What the scope actually means /wrt
> writeback should only be defined in the arch-specific kvm service
> implementing it. Your suggestion would go in the wrong direction IMO.
>    

What I'm worried is how to tell which registers go in which scope?  And 
contrariwise, when doing a cpu_synchronize_state(), how to select the 
scope?  It's easy when there's just normal and reset, but what happens 
when we gain another one?  The code may not know who calls it.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v2] KVM: x86: Extend KVM_SET_VCPU_EVENTS with selective updates

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1736 bytes --]

Avi Kivity wrote:
> On 12/15/2009 06:43 PM, Jan Kiszka wrote:
>>
>>> I now agree.  But instead of SCOPE_RESET and SCOPE_RUNTIME (or whatever
>>> that was), how about SCOPE_GPR, SCOPE_FPU, SCOPE_SREGS etc.  That means
>>> the backing code in kvm.c doesn't have to know what qemu is interested
>>> in wrt SCOPE_RESET, and it's easier for readers to infer what is meant.
>>>      
>> That's not my idea. I want to be able to state the scope in generic,
>> arch-independent, KVM-unaware code. What the scope actually means /wrt
>> writeback should only be defined in the arch-specific kvm service
>> implementing it. Your suggestion would go in the wrong direction IMO.
>>    
> 
> What I'm worried is how to tell which registers go in which scope?  And
> contrariwise, when doing a cpu_synchronize_state(), how to select the
> scope?  It's easy when there's just normal and reset, but what happens
> when we gain another one?  The code may not know who calls it.
> 

In my original patch, scopes could only be widened: If we first sync'ed
for potential register modifications and then added a sync for reset,
the latter ruled on write-back. In my current idea, there would be three
sync scopes (in increasing order):

CPU_SYNC_RUNTIME  - only write states that cannot not change
                    asynchronously
CPU_SYNC_RESET    - write everything that would change during a CPU
                    reset (excludes TSC MSR on x86)
CPU_SYNC_COMPLETE - write everything

I think these scopes are generic enough to match problems of other archs
beyond x86 as well (though I don't if any exist).

Hope I'll find some time soon to code this down, but I'm currently
stuffed with unrelated issues.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH] KVM: x86: Extend KVM_SET_VCPU_EVENTS with selective updates

[Jan Kiszka]

Jan Kiszka wrote:
> User space may not want to overwrite asynchronously changing VCPU event
> states on write-back. So allow to skip nmi.pending and sipi_vector by
> setting corresponding bits in the flags field of kvm_vcpu_events.
> 

What will happen to this patch now? Merge during 2.6.33 window or drop?

Jan

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
>  Documentation/kvm/api.txt  |   10 +++++++++-
>  arch/x86/include/asm/kvm.h |    4 ++++
>  arch/x86/kvm/x86.c         |    9 ++++++---
>  3 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/kvm/api.txt b/Documentation/kvm/api.txt
> index e1a1141..ee07e3a 100644
> --- a/Documentation/kvm/api.txt
> +++ b/Documentation/kvm/api.txt
> @@ -685,7 +685,7 @@ struct kvm_vcpu_events {
>  		__u8 pad;
>  	} nmi;
>  	__u32 sipi_vector;
> -	__u32 flags;   /* must be zero */
> +	__u32 flags;
>  };
>  
>  4.30 KVM_SET_VCPU_EVENTS
> @@ -701,6 +701,14 @@ vcpu.
>  
>  See KVM_GET_VCPU_EVENTS for the data structure.
>  
> +Fields that may be modified asynchronously by running VCPUs can be excluded
> +from the update. These fields are nmi.pending and sipi_vector. Set the
> +corresponding mask bits in the flags field to suppress overwriting their
> +current state:
> +
> +KVM_VCPUEVENT_MASK_NMI_PENDING - do not update nmi.pending
> +KVM_VCPUEVENT_MASK_SIPI_VECTOR - do not update sipi_vector
> +
>  
>  5. The kvm_run structure
>  
> diff --git a/arch/x86/include/asm/kvm.h b/arch/x86/include/asm/kvm.h
> index 950df43..acf8585 100644
> --- a/arch/x86/include/asm/kvm.h
> +++ b/arch/x86/include/asm/kvm.h
> @@ -254,6 +254,10 @@ struct kvm_reinject_control {
>  	__u8 reserved[31];
>  };
>  
> +/* When set in flags, skip corresponding fields on KVM_SET_VCPU_EVENTS */
> +#define KVM_VCPUEVENT_MASK_NMI_PENDING	0x00000001
> +#define KVM_VCPUEVENT_MASK_SIPI_VECTOR	0x00000002
> +
>  /* for KVM_GET/SET_VCPU_EVENTS */
>  struct kvm_vcpu_events {
>  	struct {
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index dd15d7a..368843c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1953,7 +1953,8 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
>  static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
>  					      struct kvm_vcpu_events *events)
>  {
> -	if (events->flags)
> +	if (events->flags &
> +	    ~(KVM_VCPUEVENT_MASK_NMI_PENDING | KVM_VCPUEVENT_MASK_SIPI_VECTOR))
>  		return -EINVAL;
>  
>  	vcpu_load(vcpu);
> @@ -1970,10 +1971,12 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
>  		kvm_pic_clear_isr_ack(vcpu->kvm);
>  
>  	vcpu->arch.nmi_injected = events->nmi.injected;
> -	vcpu->arch.nmi_pending = events->nmi.pending;
> +	if (!(events->flags & KVM_VCPUEVENT_MASK_NMI_PENDING))
> +		vcpu->arch.nmi_pending = events->nmi.pending;
>  	kvm_x86_ops->set_nmi_mask(vcpu, events->nmi.masked);
>  
> -	vcpu->arch.sipi_vector = events->sipi_vector;
> +	if (!(events->flags & KVM_VCPUEVENT_MASK_SIPI_VECTOR))
> +		vcpu->arch.sipi_vector = events->sipi_vector;
>  
>  	vcpu_put(vcpu);
>  
> 

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH] KVM: x86: Extend KVM_SET_VCPU_EVENTS with selective updates

[Avi Kivity]

On 12/14/2009 12:32 PM, Jan Kiszka wrote:
> Jan Kiszka wrote:
>    
>> User space may not want to overwrite asynchronously changing VCPU event
>> states on write-back. So allow to skip nmi.pending and sipi_vector by
>> setting corresponding bits in the flags field of kvm_vcpu_events.
>>
>>      
> What will happen to this patch now? Merge during 2.6.33 window or drop?
>    

I'll merge it into 2.6.33.

-- 
error compiling committee.c: too many arguments to function