[PATCH 0/2] Fix compilation with --disable-kvm

[PATCH 0/2] Fix compilation with --disable-kvm

[Glauber Costa]

I just came across two errors in --disable-kvm builds.

here's the patches to fix it.

[PATCH 1/2] surround kvm function with kvm_enabled

[Glauber Costa]

otherwise, compilation breaks with kvm disabled.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/pc.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index cf84416..afab0ad 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1131,7 +1131,9 @@ static void pc_init1(ram_addr_t ram_size,
 #endif
     }
 
-    kvm_set_boot_cpu_id(0);
+    if (kvm_enabled()) {
+        kvm_set_boot_cpu_id(0);
+    }
     for (i = 0; i < smp_cpus; i++) {
         env = pc_new_cpu(cpu_model);
     }
-- 
1.6.2.2

[PATCH 2/2] fix compilation with kvm disabled

[Glauber Costa]

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/msix.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index b6f3948..8636f69 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -63,6 +63,7 @@
 /* Flag for interrupt controller to declare MSI-X support */
 int msix_supported;
 
+#ifdef USE_KVM
 /* KVM specific MSIX helpers */
 static void kvm_msix_free(PCIDevice *dev)
 {
@@ -157,6 +158,14 @@ static void kvm_msix_del(PCIDevice *dev, unsigned vector)
     kvm_del_routing_entry(kvm_context, &dev->msix_irq_entries[vector]);
     kvm_commit_irq_routes(kvm_context);
 }
+#else
+
+static void kvm_msix_free(PCIDevice *dev) {}
+static void kvm_msix_update(PCIDevice *dev, int vector,
+                            int was_masked, int is_masked) {}
+static int kvm_msix_add(PCIDevice *dev, unsigned vector) { return -1; }
+static void kvm_msix_del(PCIDevice *dev, unsigned vector) {}
+#endif
 
 /* Add MSI-X capability to the config space for the device. */
 /* Given a bar and its size, add MSI-X table on top of it
@@ -337,10 +346,12 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
     if (nentries > MSIX_MAX_ENTRIES)
         return -EINVAL;
 
+#ifdef KVM_CAP_IRQCHIP
     if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
         dev->msix_irq_entries = qemu_malloc(nentries *
                                             sizeof *dev->msix_irq_entries);
     }
+#endif
     dev->msix_entry_used = qemu_mallocz(MSIX_MAX_ENTRIES *
                                         sizeof *dev->msix_entry_used);
 
@@ -454,10 +465,13 @@ void msix_notify(PCIDevice *dev, unsigned vector)
         msix_set_pending(dev, vector);
         return;
     }
+
+#ifdef KVM_CAP_IRQCHIP
     if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
         kvm_set_irq(dev->msix_irq_entries[vector].gsi, 1, NULL);
         return;
     }
+#endif
 
     address = pci_get_long(table_entry + MSIX_MSG_UPPER_ADDR);
     address = (address << 32) | pci_get_long(table_entry + MSIX_MSG_ADDR);
-- 
1.6.2.2

Re: [PATCH 2/2] fix compilation with kvm disabled

[Michael S. Tsirkin]

On Tue, Jul 07, 2009 at 02:36:31PM -0400, Glauber Costa wrote:
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  hw/msix.c |   14 ++++++++++++++
>  1 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index b6f3948..8636f69 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -63,6 +63,7 @@
>  /* Flag for interrupt controller to declare MSI-X support */
>  int msix_supported;
>  
> +#ifdef USE_KVM
>  /* KVM specific MSIX helpers */
>  static void kvm_msix_free(PCIDevice *dev)
>  {
> @@ -157,6 +158,14 @@ static void kvm_msix_del(PCIDevice *dev, unsigned vector)
>      kvm_del_routing_entry(kvm_context, &dev->msix_irq_entries[vector]);
>      kvm_commit_irq_routes(kvm_context);
>  }
> +#else
> +
> +static void kvm_msix_free(PCIDevice *dev) {}
> +static void kvm_msix_update(PCIDevice *dev, int vector,
> +                            int was_masked, int is_masked) {}
> +static int kvm_msix_add(PCIDevice *dev, unsigned vector) { return -1; }
> +static void kvm_msix_del(PCIDevice *dev, unsigned vector) {}
> +#endif
>  
>  /* Add MSI-X capability to the config space for the device. */
>  /* Given a bar and its size, add MSI-X table on top of it

Thanks for fix!

> @@ -337,10 +346,12 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>      if (nentries > MSIX_MAX_ENTRIES)
>          return -EINVAL;
>  
> +#ifdef KVM_CAP_IRQCHIP
>      if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
>          dev->msix_irq_entries = qemu_malloc(nentries *
>                                              sizeof *dev->msix_irq_entries);
>      }
> +#endif
>      dev->msix_entry_used = qemu_mallocz(MSIX_MAX_ENTRIES *
>                                          sizeof *dev->msix_entry_used);
>  
> @@ -454,10 +465,13 @@ void msix_notify(PCIDevice *dev, unsigned vector)
>          msix_set_pending(dev, vector);
>          return;
>      }
> +
> +#ifdef KVM_CAP_IRQCHIP
>      if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
>          kvm_set_irq(dev->msix_irq_entries[vector].gsi, 1, NULL);
>          return;
>      }
> +#endif
>  
>      address = pci_get_long(table_entry + MSIX_MSG_UPPER_ADDR);
>      address = (address << 32) | pci_get_long(table_entry + MSIX_MSG_ADDR);

I think it's time we stopped worrying about builds against old kernel
headers or without them. What do we gain from it?

I believe that the right thing to do is to define kvm_enabled as a macro
returning 0, and let compiler optimize the code out.

Avi?

-- 
MST

Re: [PATCH 2/2] fix compilation with kvm disabled

[Glauber Costa]

On Wed, Jul 08, 2009 at 10:59:13AM +0300, Michael S. Tsirkin wrote:
> On Tue, Jul 07, 2009 at 02:36:31PM -0400, Glauber Costa wrote:
> > Signed-off-by: Glauber Costa <glommer@redhat.com>
> > ---
> >  hw/msix.c |   14 ++++++++++++++
> >  1 files changed, 14 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/msix.c b/hw/msix.c
> > index b6f3948..8636f69 100644
> > --- a/hw/msix.c
> > +++ b/hw/msix.c
> > @@ -63,6 +63,7 @@
> >  /* Flag for interrupt controller to declare MSI-X support */
> >  int msix_supported;
> >  
> > +#ifdef USE_KVM
> >  /* KVM specific MSIX helpers */
> >  static void kvm_msix_free(PCIDevice *dev)
> >  {
> > @@ -157,6 +158,14 @@ static void kvm_msix_del(PCIDevice *dev, unsigned vector)
> >      kvm_del_routing_entry(kvm_context, &dev->msix_irq_entries[vector]);
> >      kvm_commit_irq_routes(kvm_context);
> >  }
> > +#else
> > +
> > +static void kvm_msix_free(PCIDevice *dev) {}
> > +static void kvm_msix_update(PCIDevice *dev, int vector,
> > +                            int was_masked, int is_masked) {}
> > +static int kvm_msix_add(PCIDevice *dev, unsigned vector) { return -1; }
> > +static void kvm_msix_del(PCIDevice *dev, unsigned vector) {}
> > +#endif
> >  
> >  /* Add MSI-X capability to the config space for the device. */
> >  /* Given a bar and its size, add MSI-X table on top of it
> 
> Thanks for fix!
> 
> > @@ -337,10 +346,12 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
> >      if (nentries > MSIX_MAX_ENTRIES)
> >          return -EINVAL;
> >  
> > +#ifdef KVM_CAP_IRQCHIP
> >      if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
> >          dev->msix_irq_entries = qemu_malloc(nentries *
> >                                              sizeof *dev->msix_irq_entries);
> >      }
> > +#endif
> >      dev->msix_entry_used = qemu_mallocz(MSIX_MAX_ENTRIES *
> >                                          sizeof *dev->msix_entry_used);
> >  
> > @@ -454,10 +465,13 @@ void msix_notify(PCIDevice *dev, unsigned vector)
> >          msix_set_pending(dev, vector);
> >          return;
> >      }
> > +
> > +#ifdef KVM_CAP_IRQCHIP
> >      if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
> >          kvm_set_irq(dev->msix_irq_entries[vector].gsi, 1, NULL);
> >          return;
> >      }
> > +#endif
> >  
> >      address = pci_get_long(table_entry + MSIX_MSG_UPPER_ADDR);
> >      address = (address << 32) | pci_get_long(table_entry + MSIX_MSG_ADDR);
> 
> I think it's time we stopped worrying about builds against old kernel
> headers or without them. What do we gain from it?
> 
> I believe that the right thing to do is to define kvm_enabled as a macro
> returning 0, and let compiler optimize the code out.
that already happens with kvm_enabled().
I have no idea why the compiler do not rip of code when we also test for
qemu_kvm_irqchip_in_kernel() or other things.

what we could do, is to only test for qemu_kvm_irqchip_in_kernel(),
using the hidden assumption that if kvm is not enabled, irqchip tests
will always return false.

It is a little bit messy, though.

Re: [PATCH 2/2] fix compilation with kvm disabled

[Avi Kivity]

On 07/08/2009 03:09 PM, Glauber Costa wrote:
>
>> I believe that the right thing to do is to define kvm_enabled as a macro
>> returning 0, and let compiler optimize the code out.
>>      
> that already happens with kvm_enabled().
> I have no idea why the compiler do not rip of code when we also test for
> qemu_kvm_irqchip_in_kernel() or other things.
>
> what we could do, is to only test for qemu_kvm_irqchip_in_kernel(),
> using the hidden assumption that if kvm is not enabled, irqchip tests
> will always return false.
>
> It is a little bit messy, though

We should make the call unconditional and do the 
kvm_enabled/irqchip_in_kernel checks in the kvm-specific function.  This 
way common code is only minimally affected.

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

Re: [PATCH 2/2] fix compilation with kvm disabled

[Avi Kivity]

On 07/08/2009 10:59 AM, Michael S. Tsirkin wrote:
>
>> @@ -337,10 +346,12 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>>       if (nentries>  MSIX_MAX_ENTRIES)
>>           return -EINVAL;
>>
>> +#ifdef KVM_CAP_IRQCHIP
>>       if (kvm_enabled()&&  qemu_kvm_irqchip_in_kernel()) {
>>           dev->msix_irq_entries = qemu_malloc(nentries *
>>                                               sizeof *dev->msix_irq_entries);
>>       }
>> +#endif
>>       dev->msix_entry_used = qemu_mallocz(MSIX_MAX_ENTRIES *
>>                                           sizeof *dev->msix_entry_used);
>>
>> @@ -454,10 +465,13 @@ void msix_notify(PCIDevice *dev, unsigned vector)
>>           msix_set_pending(dev, vector);
>>           return;
>>       }
>> +
>> +#ifdef KVM_CAP_IRQCHIP
>>       if (kvm_enabled()&&  qemu_kvm_irqchip_in_kernel()) {
>>           kvm_set_irq(dev->msix_irq_entries[vector].gsi, 1, NULL);
>>           return;
>>       }
>> +#endif
>>
>>       address = pci_get_long(table_entry + MSIX_MSG_UPPER_ADDR);
>>       address = (address<<  32) | pci_get_long(table_entry + MSIX_MSG_ADDR);
>>      
>
> I think it's time we stopped worrying about builds against old kernel
> headers or without them. What do we gain from it?
>    

qemu upstream doesn't carry its own headers, so it we want to merge, we 
need to work against old headers.

> I believe that the right thing to do is to define kvm_enabled as a macro
> returning 0, and let compiler optimize the code out.
>    

Doesn't work with -O0 (or if it does, we can't count on it).

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

Re: [PATCH 2/2] fix compilation with kvm disabled

[Michael S. Tsirkin]

On Wed, Jul 08, 2009 at 03:11:45PM +0300, Avi Kivity wrote:
> On 07/08/2009 10:59 AM, Michael S. Tsirkin wrote:
>>
>>> @@ -337,10 +346,12 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>>>       if (nentries>  MSIX_MAX_ENTRIES)
>>>           return -EINVAL;
>>>
>>> +#ifdef KVM_CAP_IRQCHIP
>>>       if (kvm_enabled()&&  qemu_kvm_irqchip_in_kernel()) {
>>>           dev->msix_irq_entries = qemu_malloc(nentries *
>>>                                               sizeof *dev->msix_irq_entries);
>>>       }
>>> +#endif
>>>       dev->msix_entry_used = qemu_mallocz(MSIX_MAX_ENTRIES *
>>>                                           sizeof *dev->msix_entry_used);
>>>
>>> @@ -454,10 +465,13 @@ void msix_notify(PCIDevice *dev, unsigned vector)
>>>           msix_set_pending(dev, vector);
>>>           return;
>>>       }
>>> +
>>> +#ifdef KVM_CAP_IRQCHIP
>>>       if (kvm_enabled()&&  qemu_kvm_irqchip_in_kernel()) {
>>>           kvm_set_irq(dev->msix_irq_entries[vector].gsi, 1, NULL);
>>>           return;
>>>       }
>>> +#endif
>>>
>>>       address = pci_get_long(table_entry + MSIX_MSG_UPPER_ADDR);
>>>       address = (address<<  32) | pci_get_long(table_entry + MSIX_MSG_ADDR);
>>>      
>>
>> I think it's time we stopped worrying about builds against old kernel
>> headers or without them. What do we gain from it?
>>    
>
> qemu upstream doesn't carry its own headers, so it we want to merge, we  
> need to work against old headers.

Was there ever discussion on this? I think the right thing to do is to
add own headers to qemu upstream.

>> I believe that the right thing to do is to define kvm_enabled as a macro
>> returning 0, and let compiler optimize the code out.
>>    
>
> Doesn't work with -O0 (or if it does, we can't count on it).

With -O0 you get a ton of dead code anyway. Who cares?

-- 
MST

Re: [PATCH 2/2] fix compilation with kvm disabled

[Avi Kivity]

On 07/08/2009 03:13 PM, Michael S. Tsirkin wrote:
>>> I think it's time we stopped worrying about builds against old kernel
>>> headers or without them. What do we gain from it?
>>>
>>>        
>> qemu upstream doesn't carry its own headers, so it we want to merge, we
>> need to work against old headers.
>>      
>
> Was there ever discussion on this? I think the right thing to do is to
> add own headers to qemu upstream.
>    

Discussion yes, conclusion no.

>>> I believe that the right thing to do is to define kvm_enabled as a macro
>>> returning 0, and let compiler optimize the code out.
>>>
>>>        
>> Doesn't work with -O0 (or if it does, we can't count on it).
>>      
>
> With -O0 you get a ton of dead code anyway. Who cares

People who debug (though -O1 works fine most of the time).  It also 
feels unclean to rely on optimization for correctness.

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

Re: [PATCH 2/2] fix compilation with kvm disabled

[Michael S. Tsirkin]

On Wed, Jul 08, 2009 at 03:20:02PM +0300, Avi Kivity wrote:
> On 07/08/2009 03:13 PM, Michael S. Tsirkin wrote:
>>>> I think it's time we stopped worrying about builds against old kernel
>>>> headers or without them. What do we gain from it?
>>>>
>>>>        
>>> qemu upstream doesn't carry its own headers, so it we want to merge, we
>>> need to work against old headers.
>>>      
>>
>> Was there ever discussion on this? I think the right thing to do is to
>> add own headers to qemu upstream.
>>    
>
> Discussion yes, conclusion no.
>
>>>> I believe that the right thing to do is to define kvm_enabled as a macro
>>>> returning 0, and let compiler optimize the code out.
>>>>
>>>>        
>>> Doesn't work with -O0 (or if it does, we can't count on it).
>>>      
>>
>> With -O0 you get a ton of dead code anyway. Who cares
>
> People who debug (though -O1 works fine most of the time).  It also  
> feels unclean to rely on optimization for correctness.

I do not propoise to rely on optimization for correctness.
The code under if (0) will get compiled with -O0 (and we need to carry
our headers for this) but will never run.

-- 
MST

Re: [PATCH 2/2] fix compilation with kvm disabled

[Michael S. Tsirkin]

On Tue, Jul 07, 2009 at 02:36:31PM -0400, Glauber Costa wrote:
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  hw/msix.c |   14 ++++++++++++++
>  1 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/msix.c b/hw/msix.c
> index b6f3948..8636f69 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -63,6 +63,7 @@
>  /* Flag for interrupt controller to declare MSI-X support */
>  int msix_supported;
>  
> +#ifdef USE_KVM
>  /* KVM specific MSIX helpers */
>  static void kvm_msix_free(PCIDevice *dev)
>  {
> @@ -157,6 +158,14 @@ static void kvm_msix_del(PCIDevice *dev, unsigned vector)
>      kvm_del_routing_entry(kvm_context, &dev->msix_irq_entries[vector]);
>      kvm_commit_irq_routes(kvm_context);
>  }
> +#else
> +
> +static void kvm_msix_free(PCIDevice *dev) {}
> +static void kvm_msix_update(PCIDevice *dev, int vector,
> +                            int was_masked, int is_masked) {}
> +static int kvm_msix_add(PCIDevice *dev, unsigned vector) { return -1; }
> +static void kvm_msix_del(PCIDevice *dev, unsigned vector) {}
> +#endif
>  
>  /* Add MSI-X capability to the config space for the device. */
>  /* Given a bar and its size, add MSI-X table on top of it

Avi, let's apply this first bit so that people can build with
kvm-disabled, and think a bit about the second one?

> @@ -337,10 +346,12 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>      if (nentries > MSIX_MAX_ENTRIES)
>          return -EINVAL;
>  
> +#ifdef KVM_CAP_IRQCHIP
>      if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
>          dev->msix_irq_entries = qemu_malloc(nentries *
>                                              sizeof *dev->msix_irq_entries);
>      }
> +#endif
>      dev->msix_entry_used = qemu_mallocz(MSIX_MAX_ENTRIES *
>                                          sizeof *dev->msix_entry_used);
>  
> @@ -454,10 +465,13 @@ void msix_notify(PCIDevice *dev, unsigned vector)
>          msix_set_pending(dev, vector);
>          return;
>      }
> +
> +#ifdef KVM_CAP_IRQCHIP
>      if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
>          kvm_set_irq(dev->msix_irq_entries[vector].gsi, 1, NULL);
>          return;
>      }
> +#endif
>  
>      address = pci_get_long(table_entry + MSIX_MSG_UPPER_ADDR);
>      address = (address << 32) | pci_get_long(table_entry + MSIX_MSG_ADDR);
> -- 
> 1.6.2.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] Fix compilation with --disable-kvm

[Jan Kiszka]

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

Glauber Costa wrote:
> I just came across two errors in --disable-kvm builds.
> 
> here's the patches to fix it.

Builds fine for me, too, resolving both issues I just reported.

Jan


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

Re: [PATCH 0/2] Fix compilation with --disable-kvm

[Avi Kivity]

On 07/07/2009 09:36 PM, Glauber Costa wrote:
> I just came across two errors in --disable-kvm builds.
>
> here's the patches to fix it.
>    

Applied, thanks.

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