Re: [PATCH 4/4] KVM: in-kernel-apic modification to QEMU

[Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

Hi Gregory,

The other patches all look good.  Just some minor stuff here.

Gregory Haskins wrote:
> Signed-off-by: Gregory Haskins <ghaskins-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org>
> ---
>  
> @@ -888,10 +890,17 @@ static void ioapic_service(IOAPICState *s)
>                      vector = pic_read_irq(isa_pic);
>                  else
>                      vector = entry & 0xff;
> -                
> -                apic_get_delivery_bitmask(deliver_bitmask, dest, dest_mode);
> -                apic_bus_deliver(deliver_bitmask, delivery_mode, 
> -                                 vector, polarity, trig_mode);
> +             
> +		if (kvm_allowed && kvm_apic_level) {
> +			ext_apic_bus_deliver(dest, trig_mode, dest_mode,
> +					     delivery_mode, vector);
> +			cpu_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
> +		} else {
> +			apic_get_delivery_bitmask(deliver_bitmask, dest,
> +						  dest_mode);
> +			apic_bus_deliver(deliver_bitmask, delivery_mode, 
> +					 vector, polarity, trig_mode);
> +		}
>              }
>          }
>      }
>   

Could be my client, but I think you're spacing is off here.  QEMU tends 
to use spaces instead of tabs and I think you're using tabs.

>      intno = pic_read_irq(isa_pic);
>      return intno;
> @@ -483,9 +486,10 @@ static void pc_init1(int ram_size, int vga_ram_size, int boot_device,
>          }
>          register_savevm("cpu", i, 4, cpu_save, cpu_load, env);
>          qemu_register_reset(main_cpu_reset, env);
> -        if (pci_enabled) {
> -            apic_init(env);
> -        }
> +	if (!kvm_allowed || !kvm_apic_level)
> +	    if (pci_enabled) {
> +		apic_init(env);
> +	    }
>      }

Can probably just fold all three conditions into a single if clause:

if (!(kvm_allowed && kvm_apic_level) && pci_enabled)
   apic_init(env);

Further, it may be worthwhile to just introduce a static inline to 
reduce it to:

if (!use_kernel_apic() && pci_enabled)
  apic_init(env);

>  
>      /* allocate RAM */
> @@ -671,7 +675,7 @@ static void pc_init1(int ram_size, int vga_ram_size, int boot_device,
>      register_ioport_write(0x92, 1, 1, ioport92_write, NULL);
>  
>      if (pci_enabled) {
> -        ioapic = ioapic_init();
> +        ioapic = ioapic_init(env);
>      }
>   

Have you tested -smp 2 with -no-kvm by any chance?

> diff --git a/qemu/vl.c b/qemu/vl.c
> index 88e650e..986cea4 100644
> --- a/qemu/vl.c
> +++ b/qemu/vl.c
> @@ -7312,7 +7312,7 @@ int main(int argc, char **argv)
>  		kvm_allowed = 0;
>  		break;
>  	    case QEMU_OPTION_kvm_apic:
> -		kvm_apic_level = optarg;
> +		kvm_apic_level = atoi(optarg);
>  		break;
>  #endif
>   

Hrm, what are you diffing against?  I don't see QEMU_OPTION_kvm_apic in 
my tree.

Regards,

Anthony Liguori

> -------------------------------------------------------------------------
> This SF.net email is sponsored by DB2 Express
> Download DB2 Express C - the FREE version of DB2 express and take
> control of your XML. No limits. Just data. Click to get it now.
> http://sourceforge.net/powerbar/db2/
> _______________________________________________
> kvm-devel mailing list
> kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>
>   


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/