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

[Jan Kiszka]

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



signature.asc
Description: OpenPGP digital signature

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

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jan Kiszka]

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



signature.asc
Description: OpenPGP digital signature

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

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jan Kiszka]

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



signature.asc
Description: OpenPGP digital signature

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
>>>  
>> 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?


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
 

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

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 

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_PENDING0x0001
> +#define KVM_VCPUEVENT_SET_SIPI_VECTOR0x0002
> +
>  /* 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);
>  
> 


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[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 
---

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  0x0001
+#define KVM_VCPUEVENT_SET_SIPI_VECTOR  0x0002
+
 /* 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);
 



signature.asc
Description: OpenPGP digital signature