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

["Gregory Haskins" <ghaskins-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org>]

Hehe..ya, your points are all valid.  Note that I never really intended this code to be reviewed quite yet.  Its included purely to enable testing of the kernel side.  I'll be the first to admit its a complete hack :).  I'll be working on polishing this up next.  Stay tuned.

-Greg

>>> On Thu, May 10, 2007 at 10:47 AM, in message <46433083.1080809-rdkfGonbjUSkNkDKm+mE6A@public.gmane.org>,
Anthony Liguori <anthony-rdkfGonbjUSkNkDKm+mE6A@public.gmane.org> wrote: 
> 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/