qemu-kvm broken after ./configure --disable-kvm

qemu-kvm broken after ./configure --disable-kvm

[Beth Kon]

Building latest git with ./configure --disable-kvm breaks with errors in 
pcspk.c

Re: qemu-kvm broken after ./configure --disable-kvm

[Jan Kiszka]

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

Beth Kon wrote:
> Building latest git with ./configure --disable-kvm breaks with errors in
> pcspk.c

With latest git, things break much earlier in case your host does not
provide linux/kvm.h because libkvm-all.h includes it unconditionally.

I would like to push this task to Glauber as he is already shuffling
around most of the involved code: Could you have a look on --disable-kvm
too while you are at it? My basic idea would be to get rid of direct
qemu-kvm.h includes so that you always obtain the required [proto]types
by including kvm.h, independent of CONFIG_KVM and already prepared for
upstream where there is no qemu-kvm.h.

Regarding the bugs I left behind in pcspk.c, I would suggest something
like

diff --git a/hw/pcspk.c b/hw/pcspk.c
index 9e1b59a..5b624d1 100644
--- a/hw/pcspk.c
+++ b/hw/pcspk.c
@@ -51,10 +51,9 @@ static const char *s_spk = "pcspk";
 static PCSpkState pcspk_state;
 
 #ifdef USE_KVM_PIT
-static void kvm_get_pit_ch2(PITState *pit,
-                            struct kvm_pit_state *inkernel_state)
+static void kvm_get_pit_ch2(PITState *pit, KVMPITState *inkernel_state)
 {
-    struct kvm_pit_state pit_state;
+    KVMPITState pit_state;
 
     if (kvm_enabled() && qemu_kvm_pit_in_kernel()) {
         kvm_get_pit(kvm_context, &pit_state);
@@ -68,8 +67,7 @@ static void kvm_get_pit_ch2(PITState *pit,
     }
 }
 
-static void kvm_set_pit_ch2(PITState *pit,
-                            struct kvm_pit_state *inkernel_state)
+static void kvm_set_pit_ch2(PITState *pit, KVMPITState *inkernel_state)
 {
     if (kvm_enabled() && qemu_kvm_pit_in_kernel()) {
         inkernel_state->channels[2].mode = pit->channels[2].mode;
@@ -82,9 +80,9 @@ static void kvm_set_pit_ch2(PITState *pit,
 }
 #else
 static inline void kvm_get_pit_ch2(PITState *pit,
-                                   kvm_pit_state *inkernel_state) { }
+                                   KVMPITState *inkernel_state) { }
 static inline void kvm_set_pit_ch2(PITState *pit,
-                                   kvm_pit_state *inkernel_state) { }
+                                   KVMPITState *inkernel_state) { }
 #endif
 
 static inline void generate_samples(PCSpkState *s)
@@ -168,7 +166,7 @@ static uint32_t pcspk_ioport_read(void *opaque, uint32_t addr)
 
 static void pcspk_ioport_write(void *opaque, uint32_t addr, uint32_t val)
 {
-    struct kvm_pit_state inkernel_state;
+    KVMPITState inkernel_state;
     PCSpkState *s = opaque;
     const int gate = val & 1;
 

where KVMPITState is defined as

#ifdef KVM_CAP_PIT
typedef struct kvm_pit_state KVMPITState;
#else
typedef struct { } KVMPITState;
#endif

Thanks,
Jan


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

Re: qemu-kvm broken after ./configure --disable-kvm

[Avi Kivity]

Jan Kiszka wrote:
> Beth Kon wrote:
>   
>> Building latest git with ./configure --disable-kvm breaks with errors in
>> pcspk.c
>>     
>
> With latest git, things break much earlier in case your host does not
> provide linux/kvm.h because libkvm-all.h includes it unconditionally.
>
>   

But qemu-kvm carries linux/kvm.h, so this never happens?


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

Re: qemu-kvm broken after ./configure --disable-kvm

[Jan Kiszka]

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

Avi Kivity wrote:
> Jan Kiszka wrote:
>> Beth Kon wrote:
>>  
>>> Building latest git with ./configure --disable-kvm breaks with errors in
>>> pcspk.c
>>>     
>>
>> With latest git, things break much earlier in case your host does not
>> provide linux/kvm.h because libkvm-all.h includes it unconditionally.
>>
>>   
> 
> But qemu-kvm carries linux/kvm.h, so this never happens?

1. qemu-kvm does not use its own headers when you specify --disble-kvm.
   That's the technical reason for this bug.

2. Upstream does not, and it's unclear if it ever will (if we push
   recent headers into kvm-kmod, I think there is no urgent need
   anymore). At least for code to-be-pushed upstream, we must not
   rely in this anyway.

Jan


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

Re: qemu-kvm broken after ./configure --disable-kvm

[Avi Kivity]

Jan Kiszka wrote:
>>>> Building latest git with ./configure --disable-kvm breaks with errors in
>>>> pcspk.c
>>>>     
>>>>         
>>> With latest git, things break much earlier in case your host does not
>>> provide linux/kvm.h because libkvm-all.h includes it unconditionally.
>>>
>>>   
>>>       
>> But qemu-kvm carries linux/kvm.h, so this never happens?
>>     
>
> 1. qemu-kvm does not use its own headers when you specify --disble-kvm.
>    That's the technical reason for this bug.
>   

Ah, okay.  This should be fixed (by including the headers) as long as we 
continue to carry kvm.h.

> 2. Upstream does not, and it's unclear if it ever will (if we push
>    recent headers into kvm-kmod, I think there is no urgent need
>    anymore). At least for code to-be-pushed upstream, we must not
>    rely in this anyway.

Yes.

Adding the headers to kvm-kmod.h is the right thing technically, but 
something tells me we'll get a lot of failures by people compiling first 
and installing later, rather than the sequence needed to make things 
work: compile and install kvm-kmod, compile and install qemu[-kvm].  Not 
all of the failures will be visible at compile time.

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

Re: qemu-kvm broken after ./configure --disable-kvm

[Jan Kiszka]

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

Avi Kivity wrote:
> Jan Kiszka wrote:
>>>>> Building latest git with ./configure --disable-kvm breaks with
>>>>> errors in
>>>>> pcspk.c
>>>>>             
>>>> With latest git, things break much earlier in case your host does not
>>>> provide linux/kvm.h because libkvm-all.h includes it unconditionally.
>>>>
>>>>         
>>> But qemu-kvm carries linux/kvm.h, so this never happens?
>>>     
>>
>> 1. qemu-kvm does not use its own headers when you specify --disble-kvm.
>>    That's the technical reason for this bug.
>>   
> 
> Ah, okay.  This should be fixed (by including the headers) as long as we
> continue to carry kvm.h.
> 
>> 2. Upstream does not, and it's unclear if it ever will (if we push
>>    recent headers into kvm-kmod, I think there is no urgent need
>>    anymore). At least for code to-be-pushed upstream, we must not
>>    rely in this anyway.
> 
> Yes.
> 
> Adding the headers to kvm-kmod.h is the right thing technically, but
> something tells me we'll get a lot of failures by people compiling first
> and installing later, rather than the sequence needed to make things
> work: compile and install kvm-kmod, compile and install qemu[-kvm].  Not
> all of the failures will be visible at compile time.
> 

That could (and probably should - independent of in-tree headers) be
caught by making all KVM_CAPs mandatory, ie. check for the latest and
greatest ones during configure and drop all the #ifdefs from the code.

Whatever the strategy will be, it should be one with the clear goal to
converge over the same approach with upstream.

Jan


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

Re: qemu-kvm broken after ./configure --disable-kvm

[Avi Kivity]

Jan Kiszka wrote:
>>> 2. Upstream does not, and it's unclear if it ever will (if we push
>>>    recent headers into kvm-kmod, I think there is no urgent need
>>>    anymore). At least for code to-be-pushed upstream, we must not
>>>    rely in this anyway.
>>>       
>> Yes.
>>
>> Adding the headers to kvm-kmod.h is the right thing technically, but
>> something tells me we'll get a lot of failures by people compiling first
>> and installing later, rather than the sequence needed to make things
>> work: compile and install kvm-kmod, compile and install qemu[-kvm].  Not
>> all of the failures will be visible at compile time.
>>
>>     
>
> That could (and probably should - independent of in-tree headers) be
> caught by making all KVM_CAPs mandatory, ie. check for the latest and
> greatest ones during configure and drop all the #ifdefs from the code.
>   

Not with out-of-tree headers.  qemu-kvm-0.10.x ought to build against 
Linux 2.6.27, kvm-kmod-2.6.30, and kvm-91.

Making all KVM_CAPs mandatory only works if we carry the headers with qemu.

> Whatever the strategy will be, it should be one with the clear goal to
> converge over the same approach with upstream.
>   

Definitely.  In this case I'm still not sure what we want, though.

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

Re: qemu-kvm broken after ./configure --disable-kvm

[Jan Kiszka]

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

Avi Kivity wrote:
> Jan Kiszka wrote:
>>>> 2. Upstream does not, and it's unclear if it ever will (if we push
>>>>    recent headers into kvm-kmod, I think there is no urgent need
>>>>    anymore). At least for code to-be-pushed upstream, we must not
>>>>    rely in this anyway.
>>>>       
>>> Yes.
>>>
>>> Adding the headers to kvm-kmod.h is the right thing technically, but
>>> something tells me we'll get a lot of failures by people compiling first
>>> and installing later, rather than the sequence needed to make things
>>> work: compile and install kvm-kmod, compile and install qemu[-kvm].  Not
>>> all of the failures will be visible at compile time.
>>>
>>>     
>>
>> That could (and probably should - independent of in-tree headers) be
>> caught by making all KVM_CAPs mandatory, ie. check for the latest and
>> greatest ones during configure and drop all the #ifdefs from the code.
>>   
> 
> Not with out-of-tree headers.  qemu-kvm-0.10.x ought to build against
> Linux 2.6.27, kvm-kmod-2.6.30, and kvm-91.
> 
> Making all KVM_CAPs mandatory only works if we carry the headers with qemu.

If we continue to carry our own headers, the #ifdefs are pointless. If
we don't, the configure checks should issue an overview on all the
features that will be missing and give a hint how to resolve this
(kvm-kmod...).

> 
>> Whatever the strategy will be, it should be one with the clear goal to
>> converge over the same approach with upstream.
>>   
> 
> Definitely.  In this case I'm still not sure what we want, though.
> 

Maybe a good topic for next Tuesday.

Jan


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

Re: qemu-kvm broken after ./configure --disable-kvm

[Avi Kivity]

Jan Kiszka wrote:

    

>>> That could (and probably should - independent of in-tree headers) be
>>> caught by making all KVM_CAPs mandatory, ie. check for the latest and
>>> greatest ones during configure and drop all the #ifdefs from the code.
>>>   
>>>       
>> Not with out-of-tree headers.  qemu-kvm-0.10.x ought to build against
>> Linux 2.6.27, kvm-kmod-2.6.30, and kvm-91.
>>
>> Making all KVM_CAPs mandatory only works if we carry the headers with qemu.
>>     
>
> If we continue to carry our own headers, the #ifdefs are pointless. 

Yes.

> If
> we don't, the configure checks should issue an overview on all the
> features that will be missing and give a hint how to resolve this
> (kvm-kmod...).
>   

Some features are optional (and we try to make most features optional so 
as not to force users to upgrade).

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

Re: qemu-kvm broken after ./configure --disable-kvm

[Dustin Kirkland]

On Thu, Jun 11, 2009 at 7:42 AM, Beth Kon<eak@us.ibm.com> wrote:
> Building latest git with ./configure --disable-kvm breaks with errors in
> pcspk.c

I'm building from latest git, as well, but I'm not using --disable-kvm.

Still, I seem to be affected by this issue too.  Am I missing a header
or dependency?

...
  CC    mips-softmmu/pcspk.o
/tmp/tmp.OdSUhAEONG/qemu-kvm-0.10.50.20090622191901/hw/pcspk.c:85:
error: expected declaration specifiers or ‘...’ before ‘kvm_pit_state’
/tmp/tmp.OdSUhAEONG/qemu-kvm-0.10.50.20090622191901/hw/pcspk.c:87:
error: expected declaration specifiers or ‘...’ before ‘kvm_pit_state’
/tmp/tmp.OdSUhAEONG/qemu-kvm-0.10.50.20090622191901/hw/pcspk.c: In
function ‘pcspk_callback’:
/tmp/tmp.OdSUhAEONG/qemu-kvm-0.10.50.20090622191901/hw/pcspk.c:114:
error: too many arguments to function ‘kvm_get_pit_ch2’
/tmp/tmp.OdSUhAEONG/qemu-kvm-0.10.50.20090622191901/hw/pcspk.c: In
function ‘pcspk_ioport_read’:
/tmp/tmp.OdSUhAEONG/qemu-kvm-0.10.50.20090622191901/hw/pcspk.c:161:
error: too many arguments to function ‘kvm_get_pit_ch2’
/tmp/tmp.OdSUhAEONG/qemu-kvm-0.10.50.20090622191901/hw/pcspk.c: In
function ‘pcspk_ioport_write’:
/tmp/tmp.OdSUhAEONG/qemu-kvm-0.10.50.20090622191901/hw/pcspk.c:171:
error: storage size of ‘inkernel_state’ isn’t known
/tmp/tmp.OdSUhAEONG/qemu-kvm-0.10.50.20090622191901/hw/pcspk.c:175:
error: too many arguments to function ‘kvm_get_pit_ch2’
/tmp/tmp.OdSUhAEONG/qemu-kvm-0.10.50.20090622191901/hw/pcspk.c:185:
error: too many arguments to function ‘kvm_set_pit_ch2’
/tmp/tmp.OdSUhAEONG/qemu-kvm-0.10.50.20090622191901/hw/pcspk.c:171:
warning: unused variable ‘inkernel_state’
make[2]: *** [pcspk.o] Error 1
make[1]: *** [subdir-mips-softmmu] Error 2
...

More complete log at:
 * http://launchpadlibrarian.net/28223193/buildlog_ubuntu-karmic-amd64.qemu-kvm_0.10.50.20090622162008-0daily1_FAILEDTOBUILD.txt.gz

Thanks,
:-Dustin