KVM: x86: Extend KVM_SET_VCPU_EVENTS with selective updates

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.

Quoted 4 lines · Jan Kiszka

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.

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 <[email protected]>

---

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(-)

Documentation/kvm/api.txt +9 -1

index e1a1141..b22592d 100644
@@ -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
 

arch/x86/include/asm/kvm.h +4 -0

index 950df43..d1a67ae 100644
@@ -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 {

arch/x86/kvm/x86.c +6 -3

index dd15d7a..e3c35ff 100644
@@ -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);
 

Quoted 6 lines · Jan Kiszka

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<[email protected]>

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?

Quoted 11 lines · Marcelo Tosatti

Quoted 6 lines · Jan Kiszka

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<[email protected]>

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.

Quoted 3 lines

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

It is stopped, but other vcpus are not.

Quoted 18 lines · Avi Kivity

Quoted 11 lines · Marcelo Tosatti

Quoted 6 lines · Jan Kiszka

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<[email protected]>

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.

Quoted 4 lines

Quoted 3 lines

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?

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 <[email protected]>

---

 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(-)

Documentation/kvm/api.txt +9 -1

index e1a1141..ee07e3a 100644
@@ -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
 

arch/x86/include/asm/kvm.h +4 -0

index 950df43..acf8585 100644
@@ -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 {

arch/x86/kvm/x86.c +6 -3

index dd15d7a..368843c 100644
@@ -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);
 

Quoted 5 lines · Jan Kiszka

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?

Quoted 5 lines

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.)

Quoted 6 lines

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).

Quoted 7 lines · Avi Kivity

Quoted 5 lines · Jan Kiszka

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.

Quoted 4 lines

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.

Quoted 9 lines · Jan Kiszka

Quoted 7 lines · Avi Kivity

Quoted 5 lines · Jan Kiszka

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 9 lines · Jan Kiszka

Quoted 5 lines

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.

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

Quoted 6 lines

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.

Quoted 11 lines · Jan Kiszka

Quoted 6 lines

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.

Quoted 4 lines · Jan Kiszka

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

Sure. Want to patch?

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