Patchwork [3/4] kvm: Add kvm_has_pit_state2 helper

login
register
mail settings
Submitter Jan Kiszka
Date Feb. 5, 2012, 10:46 a.m.
Message ID <6776cc5c00f4d06d23c11b68d18fd96e27f71718.1328438750.git.jan.kiszka@web.de>
Download mbox | patch
Permalink /patch/139622/
State New
Headers show

Comments

Jan Kiszka - Feb. 5, 2012, 10:46 a.m.
From: Jan Kiszka <jan.kiszka@siemens.com>

To be used for in-kernel PIT emulation.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 kvm-all.c  |   10 ++++++++++
 kvm-stub.c |    5 +++++
 kvm.h      |    1 +
 3 files changed, 16 insertions(+), 0 deletions(-)
Juan Quintela - Feb. 5, 2012, 8:03 p.m.
Jan Kiszka <jan.kiszka@web.de> wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> To be used for in-kernel PIT emulation.

....

> +    int pit_state2;

This is used as a bool.

>      int xsave, xcrs;
>      int many_ioeventfds;
>      int irqchip_inject_ioctl;
> @@ -954,6 +955,10 @@ int kvm_init(void)
>      s->xcrs = kvm_check_extension(s, KVM_CAP_XCRS);
>  #endif
>  
> +#ifdef KVM_CAP_PIT_STATE2
> +    s->pit_state2 = kvm_check_extension(s, KVM_CAP_PIT_STATE2);
> +#endif
> +

[ this happened to me when I was reviewing this patch, but culprit is
not this patch]

really kvm_check_extension() should also return a bool, but that is a
bigger change that this patch series tend to introduce.

So, I went to "man ioctl"

> RETURN VALUE
>        Usually, on success zero is returned.  A few ioctl() requests  use  the
>        return  value  as an output parameter and return a nonnegative value on
>       success.  On error, -1 is returned, and errno is set appropriately.

Usually is the important word there.

And then went to kvm-all.c

int kvm_check_extension(KVMState *s, unsigned int extension)
{
    int ret;

    ret = kvm_ioctl(s, KVM_CHECK_EXTENSION, extension);
    if (ret < 0) {
        ret = 0;
    }

    return ret;
}

What? it allways return zero?  Something should be wrong here.  I will
expect kvm_check_extension() to work by now.

Yes, kvm_ioctl() return 1 went the extension is there, just for the
people confused like me.

Later, Juan.
Jan Kiszka - Feb. 5, 2012, 8:36 p.m.
On 2012-02-05 21:03, Juan Quintela wrote:
> Jan Kiszka <jan.kiszka@web.de> wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> To be used for in-kernel PIT emulation.
> 
> ....
> 
>> +    int pit_state2;
> 
> This is used as a bool.
> 
>>      int xsave, xcrs;
>>      int many_ioeventfds;
>>      int irqchip_inject_ioctl;
>> @@ -954,6 +955,10 @@ int kvm_init(void)
>>      s->xcrs = kvm_check_extension(s, KVM_CAP_XCRS);
>>  #endif
>>  
>> +#ifdef KVM_CAP_PIT_STATE2
>> +    s->pit_state2 = kvm_check_extension(s, KVM_CAP_PIT_STATE2);
>> +#endif
>> +
> 
> [ this happened to me when I was reviewing this patch, but culprit is
> not this patch]
> 
> really kvm_check_extension() should also return a bool, but that is a
> bigger change that this patch series tend to introduce.
> 
> So, I went to "man ioctl"
> 
>> RETURN VALUE
>>        Usually, on success zero is returned.  A few ioctl() requests  use  the
>>        return  value  as an output parameter and return a nonnegative value on
>>       success.  On error, -1 is returned, and errno is set appropriately.
> 
> Usually is the important word there.

Right, because the driver decides in the end what is returned.

In case of KVM_CHECK_EXTENSION, it is > 0 for supported. Sometimes it
returns an integer value that encode "how much" is supported, e.g.
KVM_CAP_IRQ_ROUTING provides the number of supported GSI routes.

That said, all those boolean caps could indeed be encoded as such. Some
smarter way to initialize and retrieve them would also be nice. Too much
boilerplate code.

Thanks for bending your brain around this :)

Jan

Patch

diff --git a/kvm-all.c b/kvm-all.c
index c4babda..bddf922 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -75,6 +75,7 @@  struct KVMState
     struct kvm_sw_breakpoint_head kvm_sw_breakpoints;
 #endif
     int pit_in_kernel;
+    int pit_state2;
     int xsave, xcrs;
     int many_ioeventfds;
     int irqchip_inject_ioctl;
@@ -954,6 +955,10 @@  int kvm_init(void)
     s->xcrs = kvm_check_extension(s, KVM_CAP_XCRS);
 #endif
 
+#ifdef KVM_CAP_PIT_STATE2
+    s->pit_state2 = kvm_check_extension(s, KVM_CAP_PIT_STATE2);
+#endif
+
     ret = kvm_arch_init(s);
     if (ret < 0) {
         goto err;
@@ -1291,6 +1296,11 @@  int kvm_has_xcrs(void)
     return kvm_state->xcrs;
 }
 
+int kvm_has_pit_state2(void)
+{
+    return kvm_state->pit_state2;
+}
+
 int kvm_has_many_ioeventfds(void)
 {
     if (!kvm_enabled()) {
diff --git a/kvm-stub.c b/kvm-stub.c
index f63a0d2..1f1c686 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -78,6 +78,11 @@  int kvm_allows_irq0_override(void)
     return 1;
 }
 
+int kvm_has_pit_state2(void)
+{
+    return 0;
+}
+
 void kvm_setup_guest_memory(void *start, size_t size)
 {
 }
diff --git a/kvm.h b/kvm.h
index f9f1dc8..8ef4476 100644
--- a/kvm.h
+++ b/kvm.h
@@ -54,6 +54,7 @@  int kvm_has_robust_singlestep(void);
 int kvm_has_debugregs(void);
 int kvm_has_xsave(void);
 int kvm_has_xcrs(void);
+int kvm_has_pit_state2(void);
 int kvm_has_many_ioeventfds(void);
 int kvm_has_gsi_routing(void);