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

[Gleb Natapov <gleb@redhat.com>]

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)

>      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
 
>  #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?

>              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?

>      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.