[PATCH 1/4] BIOS changes for configuring irq0->inti2 override (v3)

[PATCH 1/4] BIOS changes for configuring irq0->inti2 override (v3)

[Beth Kon]

These patches resolve the irq0->inti2 override issue, and get the hpet working
on kvm. 

Override and HPET changes are sent as a series because HPET depends on the 
override. Win2k8 expects the HPET interrupt on inti2, regardless of whether 
an override exists in the BIOS. And the HPET spec states that in legacy mode, 
timer interrupt is on inti2.

The irq0->inti2 override will always be used unless the kernel cannot do irq 
routing (i.e., compatibility with old kernels). So if the kernel is capable, 
userspace sets up irq0->inti2 via the irq routing interface, and adds the 
irq0->inti2 override to the MADT interrupt source override table, 
and the mp table (for the no-acpi case).

A couple of months ago, Marcelo was seeing RHEL5 guests complain of invalid
checksum with these patches, but later he couldn't reproduce it, and I'm not 
seeing it now. While all guests still need to be fully tested, everything 
appears to be in order.  I've tested on win2k864, win2k832, RHEL5.3 32 bit, 
and ubuntu 8.10 64 bit. 

Changes from v2:

- rebased on latest kvm 
- fixed build problems with --disable-kvm (kvm_kpit_enable/disable)  

Signed-off-by: Beth Kon <eak@us.ibm.com>

diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
index cbd5f15..53359b8 100755
--- a/kvm/bios/rombios32.c
+++ b/kvm/bios/rombios32.c
@@ -444,6 +444,9 @@ uint32_t cpuid_features;
 uint32_t cpuid_ext_features;
 unsigned long ram_size;
 uint64_t ram_end;
+#ifdef BX_QEMU
+uint8_t irq0_override;
+#endif
 #ifdef BX_USE_EBDA_TABLES
 unsigned long ebda_cur_addr;
 #endif
@@ -485,6 +488,7 @@ void wrmsr_smp(uint32_t index, uint64_t val)
 #define QEMU_CFG_ARCH_LOCAL     0x8000
 #define QEMU_CFG_ACPI_TABLES  (QEMU_CFG_ARCH_LOCAL + 0)
 #define QEMU_CFG_SMBIOS_ENTRIES  (QEMU_CFG_ARCH_LOCAL + 1)
+#define QEMU_CFG_IRQ0_OVERRIDE   (QEMU_CFG_ARCH_LOCAL + 2)
 
 int qemu_cfg_port;
 
@@ -553,6 +557,18 @@ uint64_t qemu_cfg_get64 (void)
 }
 #endif
 
+#ifdef BX_QEMU
+void irq0_override_probe(void)
+{
+    if(qemu_cfg_port) {
+        qemu_cfg_select(QEMU_CFG_IRQ0_OVERRIDE);
+        qemu_cfg_read(&irq0_override, 1);
+        return;
+    }
+    memset(&irq0_override, 0, 1);
+}
+#endif
+
 void cpu_probe(void)
 {
     uint32_t eax, ebx, ecx, edx;
@@ -1195,6 +1211,13 @@ static void mptable_init(void)
 
     /* irqs */
     for(i = 0; i < 16; i++) {
+#ifdef BX_QEMU
+        /* One entry per ioapic interrupt destination. Destination 2 is covered
+         * by irq0->inti2 override (i == 0). Source IRQ 2 is unused 
+         */
+        if (irq0_override && i == 2)
+            continue;
+#endif
         putb(&q, 3); /* entry type = I/O interrupt */
         putb(&q, 0); /* interrupt type = vectored interrupt */
         putb(&q, 0); /* flags: po=0, el=0 */
@@ -1202,7 +1225,12 @@ static void mptable_init(void)
         putb(&q, 0); /* source bus ID = ISA */
         putb(&q, i); /* source bus IRQ */
         putb(&q, ioapic_id); /* dest I/O APIC ID */
-        putb(&q, i); /* dest I/O APIC interrupt in */
+#ifdef BX_QEMU
+        if (irq0_override && i == 0)
+            putb(&q, 2); /* dest I/O APIC interrupt in */
+        else
+#endif
+            putb(&q, i); /* dest I/O APIC interrupt in */
     }
     /* patch length */
     len = q - mp_config_table;
@@ -1665,16 +1693,18 @@ void acpi_bios_init(void)
 
     addr = (addr + 7) & ~7;
     madt_addr = addr;
+    madt = (void *)(addr);
     madt_size = sizeof(*madt) +
         sizeof(struct madt_processor_apic) * MAX_CPUS +
-#ifdef BX_QEMU
-        sizeof(struct madt_io_apic) /* + sizeof(struct madt_int_override) */;
-#else
         sizeof(struct madt_io_apic);
+#ifdef BX_QEMU
+    for (i = 0; i < 16; i++)
+        if (PCI_ISA_IRQ_MASK & (1U << i))
+            madt_size += sizeof(struct madt_int_override);
+    if (irq0_override)
+        madt_size += sizeof(struct madt_int_override);
 #endif
-    madt = (void *)(addr);
     addr += madt_size;
-
 #ifdef BX_QEMU
 #ifdef HPET_WORKS_IN_KVM
     addr = (addr + 7) & ~7;
@@ -1758,23 +1788,20 @@ void acpi_bios_init(void)
         io_apic->io_apic_id = smp_cpus;
         io_apic->address = cpu_to_le32(0xfec00000);
         io_apic->interrupt = cpu_to_le32(0);
+        int_override = (struct madt_int_override*)(io_apic + 1);
 #ifdef BX_QEMU
-#ifdef HPET_WORKS_IN_KVM
-        io_apic++;
-
-        int_override = (void *)io_apic;
-        int_override->type = APIC_XRUPT_OVERRIDE;
-        int_override->length = sizeof(*int_override);
-        int_override->bus = cpu_to_le32(0);
-        int_override->source = cpu_to_le32(0);
-        int_override->gsi = cpu_to_le32(2);
-        int_override->flags = cpu_to_le32(0);
-#endif
+        if (irq0_override) {
+            memset(int_override, 0, sizeof(*int_override));
+            int_override->type = APIC_XRUPT_OVERRIDE;
+            int_override->length = sizeof(*int_override);
+            int_override->source = 0;
+            int_override->gsi = 2;
+            int_override->flags = 0; /* conforms to bus specifications */
+            int_override++;
+        }
 #endif
-
-        int_override = (struct madt_int_override*)(io_apic + 1);
         for ( i = 0; i < 16; i++ ) {
-            if ( PCI_ISA_IRQ_MASK & (1U << i) ) {
+            if (PCI_ISA_IRQ_MASK & (1U << i)) {
                 memset(int_override, 0, sizeof(*int_override));
                 int_override->type   = APIC_XRUPT_OVERRIDE;
                 int_override->length = sizeof(*int_override);
@@ -1786,7 +1813,6 @@ void acpi_bios_init(void)
                 continue;
             }
             int_override++;
-            madt_size += sizeof(struct madt_int_override);
         }
         acpi_build_table_header((struct acpi_table_header *)madt,
                                 "APIC", madt_size, 1);
@@ -2697,6 +2723,9 @@ void rombios32_init(uint32_t *s3_resume_vector, uint8_t *shutdown_flag)
 
     if (bios_table_cur_addr != 0) {
 
+#ifdef BX_QEMU
+        irq0_override_probe();
+#endif        
         mptable_init();
 
         smbios_init();

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

[Beth Kon]

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);
 
     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) 
 #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);
+            }
             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;
     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);

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

[Gleb Natapov]

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.

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

[Avi Kivity]

Gleb Natapov wrote:
>>          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?
>   

irq 2 is the PIC cascade interrupt.  If it is somehow triggered, the 
kernel will ignore it.

-- 
error compiling committee.c: too many arguments to function

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

[Gleb Natapov]

On Tue, May 12, 2009 at 01:22:06PM +0300, Avi Kivity wrote:
> Gleb Natapov wrote:
>>>          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?
>>   
>
> irq 2 is the PIC cascade interrupt.  If it is somehow triggered, the  
> kernel will ignore it.
>
But here we configure IOAPIC routing. What if IOAPIC is used for
interrupt delivery and something triggers irq2. There is no entry
describing it in IOAPIC routing table, so what gsi it will be mapped
to?

--
			Gleb.

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

[Beth Kon]

Gleb Natapov wrote:
> On Tue, May 12, 2009 at 01:22:06PM +0300, Avi Kivity wrote:
>   
>> Gleb Natapov wrote:
>>     
>>>>          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?
>>>   
>>>       
>> irq 2 is the PIC cascade interrupt.  If it is somehow triggered, the  
>> kernel will ignore it.
>>
>>     
> But here we configure IOAPIC routing. What if IOAPIC is used for
> interrupt delivery and something triggers irq2. There is no entry
> describing it in IOAPIC routing table, so what gsi it will be mapped to?
>
> --
>   
The ACPI spec states that systems that support both APIC and dual-8259 
interrupt models must map system interrupt vectors 0-15 to 8259 IRQs 
0-15, except where interrupt source overrides are provided. We provide 
an irq0->inti2 override, and no irq2 override, so irq2 must be unused.

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

[Gleb Natapov]

On Tue, May 12, 2009 at 09:20:36AM -0400, Beth Kon wrote:
> Gleb Natapov wrote:
>> On Tue, May 12, 2009 at 01:22:06PM +0300, Avi Kivity wrote:
>>   
>>> Gleb Natapov wrote:
>>>     
>>>>>          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?
>>>>         
>>> irq 2 is the PIC cascade interrupt.  If it is somehow triggered, the  
>>> kernel will ignore it.
>>>
>>>     
>> But here we configure IOAPIC routing. What if IOAPIC is used for
>> interrupt delivery and something triggers irq2. There is no entry
>> describing it in IOAPIC routing table, so what gsi it will be mapped to?
>>
>> --
>>   
> The ACPI spec states that systems that support both APIC and dual-8259  
> interrupt models must map system interrupt vectors 0-15 to 8259 IRQs  
> 0-15, except where interrupt source overrides are provided. We provide  
> an irq0->inti2 override, and no irq2 override, so irq2 must be unused.
OK. I hope we do what ACPI spec states and irq2 never reaches IOAPIC.


--
			Gleb.

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

[Beth Kon]

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

[PATCH 3/4] BIOS changes for KVM HPET (v3)

[Beth Kon]

Signed-off-by: Beth Kon <eak@us.ibm.com>


diff --git a/kvm/bios/acpi-dsdt.dsl b/kvm/bios/acpi-dsdt.dsl
index c756fed..0e142be 100755
--- a/kvm/bios/acpi-dsdt.dsl
+++ b/kvm/bios/acpi-dsdt.dsl
@@ -308,7 +308,6 @@ DefinitionBlock (
             })
         }
 #ifdef BX_QEMU
-#ifdef HPET_WORKS_IN_KVM
         Device(HPET) {
             Name(_HID,  EISAID("PNP0103"))
             Name(_UID, 0)
@@ -328,7 +327,6 @@ DefinitionBlock (
             })
         }
 #endif
-#endif
     }
 
     Scope(\_SB.PCI0) {
diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
index 53359b8..df83ee7 100755
--- a/kvm/bios/rombios32.c
+++ b/kvm/bios/rombios32.c
@@ -1519,8 +1519,8 @@ struct acpi_20_generic_address {
 } __attribute__((__packed__));
 
 /*
- *  * HPET Description Table
- *   */
+ *  HPET Description Table
+ */
 struct acpi_20_hpet {
     ACPI_TABLE_HEADER_DEF                           /* ACPI common table header */
     uint32_t           timer_block_id;
@@ -1706,13 +1706,11 @@ void acpi_bios_init(void)
 #endif
     addr += madt_size;
 #ifdef BX_QEMU
-#ifdef HPET_WORKS_IN_KVM
     addr = (addr + 7) & ~7;
     hpet_addr = addr;
     hpet = (void *)(addr);
     addr += sizeof(*hpet);
 #endif
-#endif
 
     /* RSDP */
     memset(rsdp, 0, sizeof(*rsdp));
@@ -1884,7 +1882,6 @@ void acpi_bios_init(void)
     }
 
     /* HPET */
-#ifdef HPET_WORKS_IN_KVM
     memset(hpet, 0, sizeof(*hpet));
     /* Note timer_block_id value must be kept in sync with value advertised by
      * emulated hpet
@@ -1893,7 +1890,6 @@ void acpi_bios_init(void)
     hpet->addr.address = cpu_to_le32(ACPI_HPET_ADDRESS);
     acpi_build_table_header((struct  acpi_table_header *)hpet,
                              "HPET", sizeof(*hpet), 1);
-#endif
 
     acpi_additional_tables(); /* resets cfg to required entry */
     for(i = 0; i < external_tables; i++) {
@@ -1913,8 +1909,7 @@ void acpi_bios_init(void)
     /* kvm has no ssdt (processors are in dsdt) */
 //  rsdt->table_offset_entry[nb_rsdt_entries++] = cpu_to_le32(ssdt_addr);
 #ifdef BX_QEMU
-    /* No HPET (yet) */
-//  rsdt->table_offset_entry[nb_rsdt_entries++] = cpu_to_le32(hpet_addr);
+    rsdt->table_offset_entry[nb_rsdt_entries++] = cpu_to_le32(hpet_addr);
     if (nb_numa_nodes > 0)
         rsdt->table_offset_entry[nb_rsdt_entries++] = cpu_to_le32(srat_addr);
 #endif

[PATCH 4/4] Userspace changes for KVM HPET (v3)

[Beth Kon]

Signed-off-by: Beth Kon <eak@us.ibm.com>


diff --git a/hw/hpet.c b/hw/hpet.c
index c7945ec..100abf5 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -30,6 +30,7 @@
 #include "console.h"
 #include "qemu-timer.h"
 #include "hpet_emul.h"
+#include "qemu-kvm.h"
 
 //#define HPET_DEBUG
 #ifdef HPET_DEBUG
@@ -48,6 +49,28 @@ uint32_t hpet_in_legacy_mode(void)
         return 0;
 }
 
+static void hpet_legacy_enable(void)
+{
+    if (qemu_kvm_pit_in_kernel()) {
+        kvm_kpit_disable();
+        dprintf("qemu: hpet disabled kernel pit\n");
+    } else {
+        hpet_pit_disable();
+        dprintf("qemu: hpet disabled userspace pit\n");
+    }
+}
+
+static void hpet_legacy_disable(void)
+{
+    if (qemu_kvm_pit_in_kernel()) {
+        kvm_kpit_enable();
+        dprintf("qemu: hpet enabled kernel pit\n");
+    } else {
+        hpet_pit_enable();
+        dprintf("qemu: hpet enabled userspace pit\n");
+    }
+}
+
 static uint32_t timer_int_route(struct HPETTimer *timer)
 {
     uint32_t route;
@@ -475,9 +498,9 @@ static void hpet_ram_writel(void *opaque, target_phys_addr_t addr,
                 }
                 /* i8254 and RTC are disabled when HPET is in legacy mode */
                 if (activating_bit(old_val, new_val, HPET_CFG_LEGACY)) {
-                    hpet_pit_disable();
+                    hpet_legacy_enable();
                 } else if (deactivating_bit(old_val, new_val, HPET_CFG_LEGACY)) {
-                    hpet_pit_enable();
+                    hpet_legacy_disable();
                 }
                 break;
             case HPET_CFG + 4:
@@ -560,7 +583,7 @@ static void hpet_reset(void *opaque) {
          * hpet_reset is called due to system reset. At this point control must
          * be returned to pit until SW reenables hpet.
          */
-        hpet_pit_enable();
+        hpet_legacy_disable();
     count = 1;
 }
 
diff --git a/qemu-kvm.c b/qemu-kvm.c
index f55cee8..1bb853b 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -463,6 +463,25 @@ void kvm_init_vcpu(CPUState *env)
 	qemu_cond_wait(&qemu_vcpu_cond);
 }
 
+void kvm_kpit_enable(void)
+{
+    struct kvm_pit_state ps;
+    if (qemu_kvm_pit_in_kernel()) {
+        kvm_get_pit(kvm_context, &ps);
+        kvm_set_pit(kvm_context, &ps);
+    }
+}
+
+void kvm_kpit_disable(void)
+{
+    struct kvm_pit_state ps;
+    if (qemu_kvm_pit_in_kernel()) {
+        kvm_get_pit(kvm_context, &ps);
+        ps.channels[0].mode = 0xff;
+        kvm_set_pit(kvm_context, &ps);
+    }
+}
+
 int kvm_init_ap(void)
 {
 #ifdef TARGET_I386
diff --git a/qemu-kvm.h b/qemu-kvm.h
index 6a1968a..13353ec 100644
--- a/qemu-kvm.h
+++ b/qemu-kvm.h
@@ -31,6 +31,8 @@ int kvm_update_guest_debug(CPUState *env, unsigned long reinject_trap);
 int kvm_qemu_init_env(CPUState *env);
 int kvm_qemu_check_extension(int ext);
 void kvm_apic_init(CPUState *env);
+void kvm_kpit_enable(void);
+void kvm_kpit_disable(void);
 int kvm_set_irq(int irq, int level, int *status);
 
 int kvm_physical_memory_set_dirty_tracking(int enable);
diff --git a/vl.c b/vl.c
index 0bffc82..8f120c5 100644
--- a/vl.c
+++ b/vl.c
@@ -6132,10 +6132,15 @@ int main(int argc, char **argv, char **envp)
     }
 
     if (kvm_enabled()) {
-       kvm_init_ap();
+        kvm_init_ap();
 #ifdef USE_KVM
         if (kvm_irqchip && !qemu_kvm_has_gsi_routing()) {
             irq0override = 0;
+            /* if kernel can't do irq routing, interrupt source
+             * override 0->2 can not be set up as required by hpet,
+             * so disable hpet.
+             */
+            no_hpet=1;
         }
 #endif
     }

Re: [PATCH 4/4] Userspace changes for KVM HPET (v3)

[Avi Kivity]

Beth Kon wrote:
> Signed-off-by: Beth Kon <eak@us.ibm.com>
>
>
> diff --git a/hw/hpet.c b/hw/hpet.c
> index c7945ec..100abf5 100644
> --- a/hw/hpet.c
> +++ b/hw/hpet.c
> @@ -30,6 +30,7 @@
>  #include "console.h"
>  #include "qemu-timer.h"
>  #include "hpet_emul.h"
> +#include "qemu-kvm.h"
>  
>  //#define HPET_DEBUG
>  #ifdef HPET_DEBUG
> @@ -48,6 +49,28 @@ uint32_t hpet_in_legacy_mode(void)
>          return 0;
>  }
>  
> +static void hpet_legacy_enable(void)
> +{
> +    if (qemu_kvm_pit_in_kernel()) {
> +        kvm_kpit_disable();
> +        dprintf("qemu: hpet disabled kernel pit\n");
> +    } else {
> +        hpet_pit_disable();
> +        dprintf("qemu: hpet disabled userspace pit\n");
> +    }
> +}
> +
> +static void hpet_legacy_disable(void)
> +{
> +    if (qemu_kvm_pit_in_kernel()) {
> +        kvm_kpit_enable();
> +        dprintf("qemu: hpet enabled kernel pit\n");
> +    } else {
> +        hpet_pit_enable();
> +        dprintf("qemu: hpet enabled userspace pit\n");
> +    }
> +}
>   
I think it's better to move these into hpet_pit_enable() and 
hpet_pit_enable().  This avoids changing the calls below, and puts pit 
stuff in i8254.c instead of hpet.c.

Might also need to be called from hpet_load(); probably a problem in 
upstream as well.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 4/4] Userspace changes for KVM HPET (v3)

[Beth Kon]

Avi Kivity wrote:
> Beth Kon wrote:
>> Signed-off-by: Beth Kon <eak@us.ibm.com>
>>
>>
>> diff --git a/hw/hpet.c b/hw/hpet.c
>> index c7945ec..100abf5 100644
>> --- a/hw/hpet.c
>> +++ b/hw/hpet.c
>> @@ -30,6 +30,7 @@
>>  #include "console.h"
>>  #include "qemu-timer.h"
>>  #include "hpet_emul.h"
>> +#include "qemu-kvm.h"
>>  
>>  //#define HPET_DEBUG
>>  #ifdef HPET_DEBUG
>> @@ -48,6 +49,28 @@ uint32_t hpet_in_legacy_mode(void)
>>          return 0;
>>  }
>>  
>> +static void hpet_legacy_enable(void)
>> +{
>> +    if (qemu_kvm_pit_in_kernel()) {
>> +        kvm_kpit_disable();
>> +        dprintf("qemu: hpet disabled kernel pit\n");
>> +    } else {
>> +        hpet_pit_disable();
>> +        dprintf("qemu: hpet disabled userspace pit\n");
>> +    }
>> +}
>> +
>> +static void hpet_legacy_disable(void)
>> +{
>> +    if (qemu_kvm_pit_in_kernel()) {
>> +        kvm_kpit_enable();
>> +        dprintf("qemu: hpet enabled kernel pit\n");
>> +    } else {
>> +        hpet_pit_enable();
>> +        dprintf("qemu: hpet enabled userspace pit\n");
>> +    }
>> +}
>>   
> I think it's better to move these into hpet_pit_enable() and 
> hpet_pit_enable().  This avoids changing the calls below, and puts pit 
> stuff in i8254.c instead of hpet.c.
>
> Might also need to be called from hpet_load(); probably a problem in 
> upstream as well.
>
My assumption about hpet_load was that the correct pit state would be 
established via pit_load (since all saves/loads are done together).  But 
when I wrote this, I was thinking only about the userspace pit (for 
qemu). I'm not sure how the "load" concept applies to kernel state.  Do 
I need to explicitly re-enable or disable the kernel pit during load?

Re: [PATCH 4/4] Userspace changes for KVM HPET (v3)

[Beth Kon]

Beth Kon wrote:
> Avi Kivity wrote:
>> Beth Kon wrote:
>>> Signed-off-by: Beth Kon <eak@us.ibm.com>
>>>
>>>
>>> diff --git a/hw/hpet.c b/hw/hpet.c
>>> index c7945ec..100abf5 100644
>>> --- a/hw/hpet.c
>>> +++ b/hw/hpet.c
>>> @@ -30,6 +30,7 @@
>>>  #include "console.h"
>>>  #include "qemu-timer.h"
>>>  #include "hpet_emul.h"
>>> +#include "qemu-kvm.h"
>>>  
>>>  //#define HPET_DEBUG
>>>  #ifdef HPET_DEBUG
>>> @@ -48,6 +49,28 @@ uint32_t hpet_in_legacy_mode(void)
>>>          return 0;
>>>  }
>>>  
>>> +static void hpet_legacy_enable(void)
>>> +{
>>> +    if (qemu_kvm_pit_in_kernel()) {
>>> +        kvm_kpit_disable();
>>> +        dprintf("qemu: hpet disabled kernel pit\n");
>>> +    } else {
>>> +        hpet_pit_disable();
>>> +        dprintf("qemu: hpet disabled userspace pit\n");
>>> +    }
>>> +}
>>> +
>>> +static void hpet_legacy_disable(void)
>>> +{
>>> +    if (qemu_kvm_pit_in_kernel()) {
>>> +        kvm_kpit_enable();
>>> +        dprintf("qemu: hpet enabled kernel pit\n");
>>> +    } else {
>>> +        hpet_pit_enable();
>>> +        dprintf("qemu: hpet enabled userspace pit\n");
>>> +    }
>>> +}
>>>   
>> I think it's better to move these into hpet_pit_enable() and 
>> hpet_pit_enable().  This avoids changing the calls below, and puts 
>> pit stuff in i8254.c instead of hpet.c.
>>
>> Might also need to be called from hpet_load(); probably a problem in 
>> upstream as well.
>>
> My assumption about hpet_load was that the correct pit state would be 
> established via pit_load (since all saves/loads are done together).  
> But when I wrote this, I was thinking only about the userspace pit 
> (for qemu). I'm not sure how the "load" concept applies to kernel 
> state.  Do I need to explicitly re-enable or disable the kernel pit 
> during load?
Looking further at the code, it looks like kvm_pit_load should take care 
of this. Agree?



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

Re: [PATCH 4/4] Userspace changes for KVM HPET (v3)

[Avi Kivity]

Beth Kon wrote:
> Beth Kon wrote:
>> Avi Kivity wrote:
>>> Beth Kon wrote:
>>>> Signed-off-by: Beth Kon <eak@us.ibm.com>
>>>>
>>>>
>>>> diff --git a/hw/hpet.c b/hw/hpet.c
>>>> index c7945ec..100abf5 100644
>>>> --- a/hw/hpet.c
>>>> +++ b/hw/hpet.c
>>>> @@ -30,6 +30,7 @@
>>>>  #include "console.h"
>>>>  #include "qemu-timer.h"
>>>>  #include "hpet_emul.h"
>>>> +#include "qemu-kvm.h"
>>>>  
>>>>  //#define HPET_DEBUG
>>>>  #ifdef HPET_DEBUG
>>>> @@ -48,6 +49,28 @@ uint32_t hpet_in_legacy_mode(void)
>>>>          return 0;
>>>>  }
>>>>  
>>>> +static void hpet_legacy_enable(void)
>>>> +{
>>>> +    if (qemu_kvm_pit_in_kernel()) {
>>>> +        kvm_kpit_disable();
>>>> +        dprintf("qemu: hpet disabled kernel pit\n");
>>>> +    } else {
>>>> +        hpet_pit_disable();
>>>> +        dprintf("qemu: hpet disabled userspace pit\n");
>>>> +    }
>>>> +}
>>>> +
>>>> +static void hpet_legacy_disable(void)
>>>> +{
>>>> +    if (qemu_kvm_pit_in_kernel()) {
>>>> +        kvm_kpit_enable();
>>>> +        dprintf("qemu: hpet enabled kernel pit\n");
>>>> +    } else {
>>>> +        hpet_pit_enable();
>>>> +        dprintf("qemu: hpet enabled userspace pit\n");
>>>> +    }
>>>> +}
>>>>   
>>> I think it's better to move these into hpet_pit_enable() and 
>>> hpet_pit_enable().  This avoids changing the calls below, and puts 
>>> pit stuff in i8254.c instead of hpet.c.
>>>
>>> Might also need to be called from hpet_load(); probably a problem in 
>>> upstream as well.
>>>
>> My assumption about hpet_load was that the correct pit state would be 
>> established via pit_load (since all saves/loads are done together).  
>> But when I wrote this, I was thinking only about the userspace pit 
>> (for qemu). I'm not sure how the "load" concept applies to kernel 
>> state.  Do I need to explicitly re-enable or disable the kernel pit 
>> during load?
> Looking further at the code, it looks like kvm_pit_load should take 
> care of this. Agree?
>

I doesn't save/load the "enabled" bit, does it?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH 4/4] Userspace changes for KVM HPET (v3)

[Avi Kivity]

Avi Kivity wrote:
>>> My assumption about hpet_load was that the correct pit state would 
>>> be established via pit_load (since all saves/loads are done 
>>> together).  But when I wrote this, I was thinking only about the 
>>> userspace pit (for qemu). I'm not sure how the "load" concept 
>>> applies to kernel state.  Do I need to explicitly re-enable or 
>>> disable the kernel pit during load?
>> Looking further at the code, it looks like kvm_pit_load should take 
>> care of this. Agree?
>>
>
> I doesn't save/load the "enabled" bit, does it?
>

Also, we might migrate between a host with pit-in-kernel and a host with 
pit-in-userspace, so this is should be handled at the pit level, not kvm.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

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

[Gleb Natapov]

On Mon, May 11, 2009 at 01:29:43PM -0400, Beth Kon wrote:
> Signed-off-by: Beth Kon <eak@us.ibm.com>
> 
> diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
> index cbd5f15..53359b8 100755
> --- a/kvm/bios/rombios32.c
> +++ b/kvm/bios/rombios32.c
> @@ -444,6 +444,9 @@ uint32_t cpuid_features;
>  uint32_t cpuid_ext_features;
>  unsigned long ram_size;
>  uint64_t ram_end;
> +#ifdef BX_QEMU
> +uint8_t irq0_override;
> +#endif
>  #ifdef BX_USE_EBDA_TABLES
>  unsigned long ebda_cur_addr;
>  #endif
> @@ -485,6 +488,7 @@ void wrmsr_smp(uint32_t index, uint64_t val)
>  #define QEMU_CFG_ARCH_LOCAL     0x8000
>  #define QEMU_CFG_ACPI_TABLES  (QEMU_CFG_ARCH_LOCAL + 0)
>  #define QEMU_CFG_SMBIOS_ENTRIES  (QEMU_CFG_ARCH_LOCAL + 1)
> +#define QEMU_CFG_IRQ0_OVERRIDE   (QEMU_CFG_ARCH_LOCAL + 2)
>  
>  int qemu_cfg_port;
>  
> @@ -553,6 +557,18 @@ uint64_t qemu_cfg_get64 (void)
>  }
>  #endif
>  
> +#ifdef BX_QEMU
> +void irq0_override_probe(void)
> +{
> +    if(qemu_cfg_port) {
> +        qemu_cfg_select(QEMU_CFG_IRQ0_OVERRIDE);
> +        qemu_cfg_read(&irq0_override, 1);
> +        return;
> +    }
> +    memset(&irq0_override, 0, 1);
> +}
Why memset and not irq0_override = 0, actually it should zero already.

--
			Gleb.

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

[Beth Kon]

Gleb Natapov wrote:
> On Mon, May 11, 2009 at 01:29:43PM -0400, Beth Kon wrote:
>   
>> Signed-off-by: Beth Kon <eak@us.ibm.com>
>>
>> diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
>> index cbd5f15..53359b8 100755
>> --- a/kvm/bios/rombios32.c
>> +++ b/kvm/bios/rombios32.c
>> @@ -444,6 +444,9 @@ uint32_t cpuid_features;
>>  uint32_t cpuid_ext_features;
>>  unsigned long ram_size;
>>  uint64_t ram_end;
>> +#ifdef BX_QEMU
>> +uint8_t irq0_override;
>> +#endif
>>  #ifdef BX_USE_EBDA_TABLES
>>  unsigned long ebda_cur_addr;
>>  #endif
>> @@ -485,6 +488,7 @@ void wrmsr_smp(uint32_t index, uint64_t val)
>>  #define QEMU_CFG_ARCH_LOCAL     0x8000
>>  #define QEMU_CFG_ACPI_TABLES  (QEMU_CFG_ARCH_LOCAL + 0)
>>  #define QEMU_CFG_SMBIOS_ENTRIES  (QEMU_CFG_ARCH_LOCAL + 1)
>> +#define QEMU_CFG_IRQ0_OVERRIDE   (QEMU_CFG_ARCH_LOCAL + 2)
>>  
>>  int qemu_cfg_port;
>>  
>> @@ -553,6 +557,18 @@ uint64_t qemu_cfg_get64 (void)
>>  }
>>  #endif
>>  
>> +#ifdef BX_QEMU
>> +void irq0_override_probe(void)
>> +{
>> +    if(qemu_cfg_port) {
>> +        qemu_cfg_select(QEMU_CFG_IRQ0_OVERRIDE);
>> +        qemu_cfg_read(&irq0_override, 1);
>> +        return;
>> +    }
>> +    memset(&irq0_override, 0, 1);
>> +}
>>     
> Why memset and not irq0_override = 0, actually it should zero already.
>
>   
This was an oversight, left over from some early cut-and-paste coding I 
was doing. You're right - not necessary. Thanks.
> --
> 			Gleb.
> --
> 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
>