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

[Anthony Liguori <anthony-rdkfGonbjUSkNkDKm+mE6A@public.gmane.org>]

Gregory Haskins wrote:
> Signed-off-by: Gregory Haskins <ghaskins-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org>
> ---
>
>  qemu/hw/apic.c  |   22 ++++++++++++++++++++--
>  qemu/hw/pc.c    |   13 ++++++++++++-
>  qemu/qemu-kvm.c |   18 ++++++++++--------
>  qemu/vl.h       |    2 +-
>  user/kvmctl.c   |   25 +++++++++++++++++++++++--
>  user/kvmctl.h   |    5 +++--
>  user/main.c     |    3 +--
>  7 files changed, 70 insertions(+), 18 deletions(-)
>
> diff --git a/qemu/hw/apic.c b/qemu/hw/apic.c
> index 5704224..8aa3ef1 100644
> --- a/qemu/hw/apic.c
> +++ b/qemu/hw/apic.c
> @@ -19,6 +19,11 @@
>   */
>  #include "vl.h"
>  
> +#ifdef USE_KVM
> +#include "kvmctl.h"
> +extern kvm_context_t kvm_context;
> +#endif
>   

extern's are usually a red-herring that something isn't right.

>  //#define DEBUG_APIC
>  //#define DEBUG_IOAPIC
>  
> @@ -87,6 +92,7 @@ typedef struct APICState {
>  } APICState;
>  
>  struct IOAPICState {
> +    CPUState *cpu_env;
>      uint8_t id;
>      uint8_t ioregsel;
>  
> @@ -895,10 +901,21 @@ static void ioapic_service(IOAPICState *s)
>                      vector = pic_read_irq(isa_pic);
>                  else
>                      vector = entry & 0xff;
> -                
> +             
> +#ifdef USE_KVM
> +		if (kvm_allowed) {
> +		    kvm_apic_bus_deliver(kvm_context, dest, trig_mode,
> +					 dest_mode, delivery_mode, vector);
>   

Perhaps a qemu-kvm.c wrapper that doesn't take a kvm_context is needed 
here.  That would eliminate the extern.

> +		    cpu_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
> +		} else {
> +#endif
>                  apic_get_delivery_bitmask(deliver_bitmask, dest, dest_mode);
>                  apic_bus_deliver(deliver_bitmask, delivery_mode, 
>                                   vector, polarity, trig_mode);
> +#ifdef USE_KVM
> +		}
> +#endif
> +
>              }
>          }
>      }
> @@ -1052,7 +1069,7 @@ static CPUWriteMemoryFunc *ioapic_mem_write[3] = {
>      ioapic_mem_writel,
>  };
>  
> -IOAPICState *ioapic_init(void)
> +IOAPICState *ioapic_init(CPUState *env)
>  {
>      IOAPICState *s;
>      int io_memory;
> @@ -1061,6 +1078,7 @@ IOAPICState *ioapic_init(void)
>      if (!s)
>          return NULL;
>      ioapic_reset(s);
> +    s->cpu_env = env;
>      s->id = last_apic_id++;
>  
>      io_memory = cpu_register_io_memory(0, ioapic_mem_read, 
> diff --git a/qemu/hw/pc.c b/qemu/hw/pc.c
> index eda49cf..3862a8f 100644
> --- a/qemu/hw/pc.c
> +++ b/qemu/hw/pc.c
> @@ -91,6 +91,9 @@ int cpu_get_pic_interrupt(CPUState *env)
>  {
>      int intno;
>  
> +#ifdef USE_KVM
> +    if (!kvm_allowed) {
> +#endif
>      intno = apic_get_interrupt(env);
>      if (intno >= 0) {
>   

Indent the inner block please :-)

>          /* set irq request if a PIC irq is still pending */
> @@ -98,10 +101,15 @@ int cpu_get_pic_interrupt(CPUState *env)
>          pic_update_irq(isa_pic); 
>          return intno;
>      }
> +
>      /* read the irq from the PIC */
>      if (!apic_accept_pic_intr(env))
>          return -1;
>  
> +#ifdef USE_KVM
> +    }
> +#endif
> +
>      intno = pic_read_irq(isa_pic);
>      return intno;
>  }
> @@ -483,6 +491,9 @@ 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);
> +#ifdef USE_KVM
> +	if (!kvm_allowed)
> +#endif
>          if (pci_enabled) {
>              apic_init(env);
>          }
>   

Same here.  Perhaps we should always define kvm_allowed and set it to 
zero when !defined(USE_KVM).  We can probably just #if stuff out that's 
performance critical.

>  
> -    sregs.apic_base = cpu_get_apic_base(env);
> +    /* These two are no longer used once the in-kernel APIC is enabled */
> +    sregs.apic_base = 0;
> +    sregs.cr8 = 0;
> +
>      sregs.efer = env->efer;
> -    sregs.cr8 = cpu_get_apic_tpr(env);
>  
>      kvm_set_sregs(kvm_context, 0, &sregs);
>  
> @@ -321,7 +323,7 @@ static void save_regs(CPUState *env)
>      env->cr[3] = sregs.cr3;
>      env->cr[4] = sregs.cr4;
>  
> -    cpu_set_apic_base(env, sregs.apic_base);
> +    //cpu_set_apic_base(env, sregs.apic_base);

I take it this code cannot support an in-QEMU APIC?  So it breaks -no-kvm?

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/