[3/4] kvm: Add kvm_has_pit_state2 helper

Submitted by Jan Kiszka on Feb. 5, 2012, 10:46 a.m.

Details

Message ID 6776cc5c00f4d06d23c11b68d18fd96e27f71718.1328438750.git.jan.kiszka@web.de
State New
Headers show

Commit Message

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

Comments

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 hide | download patch | download mbox

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