Re: [PATCH 2/4] Userspace changes for configuring irq0->inti2override (v3)

[Beth Kon <eak@us.ibm.com>]

Gleb Natapov wrote:
> On Mon, May 11, 2009 at 01:29:44PM -0400, Beth Kon wrote:
>   
>> Signed-off-by: Beth Kon <eak@us.ibm.com>
>>
>> diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
>> index e1b19d7..bb74f38 100644
>> --- a/hw/fw_cfg.c
>> +++ b/hw/fw_cfg.c
>> @@ -279,6 +279,7 @@ void *fw_cfg_init(uint32_t ctl_port, uint32_t data_port,
>>      fw_cfg_add_bytes(s, FW_CFG_UUID, qemu_uuid, 16);
>>      fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)nographic);
>>      fw_cfg_add_i16(s, FW_CFG_NB_CPUS, (uint16_t)smp_cpus);
>> +    fw_cfg_add_i16(s, FW_CFG_IRQ0_OVERRIDE, (uint16_t)irq0override);
>>  
>>     
> It is read as 1 byte by the BIOS, but it is 2 bytes here. And arch
> specific config should be registered in arch specific place (hw/pc.c)
>   
ok.
>   
>>      register_savevm("fw_cfg", -1, 1, fw_cfg_save, fw_cfg_load, s);
>>      qemu_register_reset(fw_cfg_reset, s);
>> diff --git a/hw/fw_cfg.h b/hw/fw_cfg.h
>> index f616ed2..1de7360 100644
>> --- a/hw/fw_cfg.h
>> +++ b/hw/fw_cfg.h
>> @@ -19,6 +19,7 @@
>>  
>>  #define FW_CFG_WRITE_CHANNEL    0x4000
>>  #define FW_CFG_ARCH_LOCAL       0x8000
>> +#define FW_CFG_IRQ0_OVERRIDE    (FW_CFG_ARCH_LOCAL + 2)
>>     
> This should go to hw/pc.c
>   
ok.
>   
>>  #define FW_CFG_ENTRY_MASK       ~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL)
>>  
>>  #define FW_CFG_INVALID          0xffff
>> diff --git a/hw/ioapic.c b/hw/ioapic.c
>> index 0b70cf6..2d77a2c 100644
>> --- a/hw/ioapic.c
>> +++ b/hw/ioapic.c
>> @@ -23,6 +23,7 @@
>>  
>>  #include "hw.h"
>>  #include "pc.h"
>> +#include "sysemu.h"
>>  #include "qemu-timer.h"
>>  #include "host-utils.h"
>>  
>> @@ -95,14 +96,13 @@ void ioapic_set_irq(void *opaque, int vector, int level)
>>  {
>>      IOAPICState *s = opaque;
>>  
>> -#if 0
>>      /* ISA IRQs map to GSI 1-1 except for IRQ0 which maps
>>       * to GSI 2.  GSI maps to ioapic 1-1.  This is not
>>       * the cleanest way of doing it but it should work. */
>>  
>> -    if (vector == 0)
>> +    if (vector == 0 && irq0override) {
>>          vector = 2;
>> -#endif
>> +    }
>>  
>>      if (vector >= 0 && vector < IOAPIC_NUM_PINS) {
>>          uint32_t mask = 1 << vector;
>> diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
>> index 8cb6faa..2e52c87 100644
>> --- a/qemu-kvm-x86.c
>> +++ b/qemu-kvm-x86.c
>> @@ -879,7 +879,11 @@ int kvm_arch_init_irq_routing(void)
>>                  return r;
>>          }
>>          for (i = 0; i < 24; ++i) {
>> -            r = kvm_add_irq_route(kvm_context, i, KVM_IRQCHIP_IOAPIC, i);
>> +            if (i == 0) {
>> +                r = kvm_add_irq_route(kvm_context, i, KVM_IRQCHIP_IOAPIC, 2);
>> +            } else if (i != 2) {
>> +                r = kvm_add_irq_route(kvm_context, i, KVM_IRQCHIP_IOAPIC, i);
>> +            }
>>     
> There is no entry for IRQ2, is this OK? What happens if IRQ2 triggers?
>
>   
Answered in separate email.
>>              if (r < 0)
>>                  return r;
>>          }
>> diff --git a/qemu-kvm.h b/qemu-kvm.h
>> index dd045dd..6a1968a 100644
>> --- a/qemu-kvm.h
>> +++ b/qemu-kvm.h
>> @@ -165,6 +165,7 @@ void qemu_kvm_cpu_stop(CPUState *env);
>>  #define kvm_enabled() (kvm_allowed)
>>  #define qemu_kvm_irqchip_in_kernel() kvm_irqchip_in_kernel(kvm_context)
>>  #define qemu_kvm_pit_in_kernel() kvm_pit_in_kernel(kvm_context)
>> +#define qemu_kvm_has_gsi_routing() kvm_has_gsi_routing(kvm_context)
>>  #define kvm_has_sync_mmu() qemu_kvm_has_sync_mmu()
>>  void kvm_init_vcpu(CPUState *env);
>>  void kvm_load_tsc(CPUState *env);
>> @@ -173,6 +174,7 @@ void kvm_load_tsc(CPUState *env);
>>  #define kvm_nested 0
>>  #define qemu_kvm_irqchip_in_kernel() (0)
>>  #define qemu_kvm_pit_in_kernel() (0)
>> +#define qemu_kvm_has_gsi_routing() (0)
>>  #define kvm_has_sync_mmu() (0)
>>  #define kvm_load_registers(env) do {} while(0)
>>  #define kvm_save_registers(env) do {} while(0)
>> diff --git a/sysemu.h b/sysemu.h
>> index 1f45fd6..292bbc3 100644
>> --- a/sysemu.h
>> +++ b/sysemu.h
>> @@ -93,6 +93,7 @@ extern int graphic_width;
>>  extern int graphic_height;
>>  extern int graphic_depth;
>>  extern int nographic;
>> +extern int irq0override;
>>  extern const char *keyboard_layout;
>>  extern int win2k_install_hack;
>>  extern int rtc_td_hack;
>> diff --git a/vl.c b/vl.c
>> index d9f0607..0bffc82 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -207,6 +207,7 @@ static int vga_ram_size;
>>  enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
>>  static DisplayState *display_state;
>>  int nographic;
>> +int irq0override;
>>  static int curses;
>>  static int sdl;
>>  const char* keyboard_layout = NULL;
>> @@ -5035,6 +5036,7 @@ int main(int argc, char **argv, char **envp)
>>      vga_ram_size = VGA_RAM_SIZE;
>>      snapshot = 0;
>>      nographic = 0;
>> +    irq0override = 1;
>>     
> Why not do that when defining the variable? Yeah I realize this is how
> it is done for other variables too, but why?
>
>   
Good question. I don't think there is any good reason. I was conforming 
to the existing style.
>>      curses = 0;
>>      kernel_filename = NULL;
>>      kernel_cmdline = "";
>> @@ -6129,8 +6131,14 @@ int main(int argc, char **argv, char **envp)
>>          }
>>      }
>>  
>> -    if (kvm_enabled())
>> -	kvm_init_ap();
>> +    if (kvm_enabled()) {
>> +       kvm_init_ap();
>> +#ifdef USE_KVM
>> +        if (kvm_irqchip && !qemu_kvm_has_gsi_routing()) {
>> +            irq0override = 0;
>> +        }
>> +#endif
>> +    }
>>  
>>      machine->init(ram_size, vga_ram_size, boot_devices,
>>                    kernel_filename, kernel_cmdline, initrd_filename, cpu_model);
>> --
>> 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
>>     
>
> --
> 			Gleb.
>