[Qemu-devel] [PATCH v7 0/4] bugs fix for hpet

[Qemu-devel] [PATCH v7 0/4] bugs fix for hpet

[Liu Ping Fan]

v7:
  use macro to define "intcap" in pc.h
  (as to 3/4 and 4/4, I am not sure about whether to merge them or not, so keep them separate")

v6:
  move the setting of intcap to board, and keep the init value as zero. (thanks for the discussion from Paolo and Michael)
  introduce an extra hpet property "compat" to tell PC version

v5:
  use stand compat property to fix hpet intcap on pc-q35, while on pc-piix, hard code intcap as IRQ2

v4:
  use stand compat property to fix hpet intcap

v3:
  change hpet interrupt capablity on board's demand



Liu Ping Fan (4):
  hpet: inverse polarity when pin above ISA_NUM_IRQS
  hpet: enable to entitle more irq pins for hpet
  PC: use qdev_xx to create hpet instead of sysbus_create_xx
  PC: differentiate hpet's interrupt capability on piix and q35

 hw/i386/pc.c         | 23 ++++++++++++++++++++---
 hw/i386/pc_piix.c    |  7 ++++++-
 hw/i386/pc_q35.c     |  6 +++++-
 hw/timer/hpet.c      | 24 ++++++++++++++++++++----
 include/hw/i386/pc.h | 13 ++++++++++++-
 5 files changed, 63 insertions(+), 10 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH v7 1/4] hpet: inverse polarity when pin above ISA_NUM_IRQS

[Liu Ping Fan]

According to hpet spec, hpet irq is high active. But according to
ICH spec, there is inversion before the input of ioapic. So the OS
will expect low active on this IRQ line. (On bare metal, if OS driver
claims high active on this line, spurious irq is generated)

We fold the emulation of this inversion inside the hpet logic.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/timer/hpet.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index fcd22ae..8429eb3 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -198,13 +198,23 @@ static void update_irq(struct HPETTimer *timer, int set)
     if (!set || !timer_enabled(timer) || !hpet_enabled(timer->state)) {
         s->isr &= ~mask;
         if (!timer_fsb_route(timer)) {
-            qemu_irq_lower(s->irqs[route]);
+            /* fold the ICH PIRQ# pin's internal inversion logic into hpet */
+            if (route >= ISA_NUM_IRQS) {
+                qemu_irq_raise(s->irqs[route]);
+            } else {
+                qemu_irq_lower(s->irqs[route]);
+            }
         }
     } else if (timer_fsb_route(timer)) {
         stl_le_phys(timer->fsb >> 32, timer->fsb & 0xffffffff);
     } else if (timer->config & HPET_TN_TYPE_LEVEL) {
         s->isr |= mask;
-        qemu_irq_raise(s->irqs[route]);
+        /* fold the ICH PIRQ# pin's internal inversion logic into hpet */
+        if (route >= ISA_NUM_IRQS) {
+            qemu_irq_lower(s->irqs[route]);
+        } else {
+            qemu_irq_raise(s->irqs[route]);
+        }
     } else {
         s->isr &= ~mask;
         qemu_irq_pulse(s->irqs[route]);
-- 
1.8.1.4

[Qemu-devel] [PATCH v7 2/4] hpet: enable to entitle more irq pins for hpet

[Liu Ping Fan]

On q35, IRQ2/8 can be reserved for hpet timer 0/1. And pin 16~23
of ioapic can be dynamically assigned to hpet as guest chooses.
So we introduce intcap property to do that. (currently, its value
is IRQ2. Later, it should be set by board.)

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/timer/hpet.c      | 12 ++++++++++--
 include/hw/i386/pc.h |  2 ++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 8429eb3..48a13bf 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -42,6 +42,9 @@
 
 #define HPET_MSI_SUPPORT        0
 
+/* Will fix: intcap is set by board, and should be 0 if nobody sets. */
+#define HPET_TN_INT_CAP_DEFAULT 0x4ULL
+
 #define TYPE_HPET "hpet"
 #define HPET(obj) OBJECT_CHECK(HPETState, (obj), TYPE_HPET)
 
@@ -73,6 +76,7 @@ typedef struct HPETState {
     uint8_t rtc_irq_level;
     qemu_irq pit_enabled;
     uint8_t num_timers;
+    uint32_t intcap;
     HPETTimer timer[HPET_MAX_TIMERS];
 
     /* Memory-mapped, software visible registers */
@@ -663,8 +667,8 @@ static void hpet_reset(DeviceState *d)
         if (s->flags & (1 << HPET_MSI_SUPPORT)) {
             timer->config |= HPET_TN_FSB_CAP;
         }
-        /* advertise availability of ioapic inti2 */
-        timer->config |=  0x00000004ULL << 32;
+        /* advertise availability of ioapic int */
+        timer->config |=  (uint64_t)s->intcap << 32;
         timer->period = 0ULL;
         timer->wrap_flag = 0;
     }
@@ -713,6 +717,9 @@ static void hpet_realize(DeviceState *dev, Error **errp)
     int i;
     HPETTimer *timer;
 
+    if (!s->intcap) {
+        error_printf("Hpet's intcap not initialized.\n");
+    }
     if (hpet_cfg.count == UINT8_MAX) {
         /* first instance */
         hpet_cfg.count = 0;
@@ -753,6 +760,7 @@ static void hpet_realize(DeviceState *dev, Error **errp)
 static Property hpet_device_properties[] = {
     DEFINE_PROP_UINT8("timers", HPETState, num_timers, HPET_MIN_TIMERS),
     DEFINE_PROP_BIT("msi", HPETState, flags, HPET_MSI_SUPPORT, false),
+    DEFINE_PROP_UINT32(HPET_INTCAP, HPETState, intcap, HPET_TN_INT_CAP_DEFAULT),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 9b2ddc4..1361a27 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -10,6 +10,8 @@
 
 #include "qemu/range.h"
 
+#define HPET_INTCAP "hpet-intcap"
+
 /* PC-style peripherals (also used by other machines).  */
 
 typedef struct PcPciInfo {
-- 
1.8.1.4

[Qemu-devel] [PATCH v7 3/4] PC: use qdev_xx to create hpet instead of sysbus_create_xx

[Liu Ping Fan]

sysbus_create_xx func does not allow us to set a device's extra
properties.  While hpet need to set its compat property before
initialization, so we abandon the wrapper function, and spread
its logic "inline"

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/i386/pc.c    | 11 +++++++++--
 hw/timer/hpet.c |  4 +---
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0c313fe..72cc850 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1246,9 +1246,16 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
      * when the HPET wants to take over. Thus we have to disable the latter.
      */
     if (!no_hpet && (!kvm_irqchip_in_kernel() || kvm_has_pit_state2())) {
-        hpet = sysbus_try_create_simple("hpet", HPET_BASE, NULL);
-
+        /* In order to set property, here not using sysbus_try_create_simple */
+        hpet = qdev_try_create(NULL, "hpet");
         if (hpet) {
+            /* tmp fix. For compat, hard code to IRQ2 until we have correct
+             * compat property and differentiate pc-iix with pc-q35
+             */
+            qdev_prop_set_uint32(hpet, HPET_INTCAP, 0x4);
+            qdev_init_nofail(hpet);
+            sysbus_mmio_map(SYS_BUS_DEVICE(hpet), 0, HPET_BASE);
+
             for (i = 0; i < GSI_NUM_PINS; i++) {
                 sysbus_connect_irq(SYS_BUS_DEVICE(hpet), i, gsi[i]);
             }
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 48a13bf..d783849 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -42,8 +42,6 @@
 
 #define HPET_MSI_SUPPORT        0
 
-/* Will fix: intcap is set by board, and should be 0 if nobody sets. */
-#define HPET_TN_INT_CAP_DEFAULT 0x4ULL
 
 #define TYPE_HPET "hpet"
 #define HPET(obj) OBJECT_CHECK(HPETState, (obj), TYPE_HPET)
@@ -760,7 +758,7 @@ static void hpet_realize(DeviceState *dev, Error **errp)
 static Property hpet_device_properties[] = {
     DEFINE_PROP_UINT8("timers", HPETState, num_timers, HPET_MIN_TIMERS),
     DEFINE_PROP_BIT("msi", HPETState, flags, HPET_MSI_SUPPORT, false),
-    DEFINE_PROP_UINT32(HPET_INTCAP, HPETState, intcap, HPET_TN_INT_CAP_DEFAULT),
+    DEFINE_PROP_UINT32(HPET_INTCAP, HPETState, intcap, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
1.8.1.4

[Qemu-devel] [PATCH v7 4/4] PC: differentiate hpet's interrupt capability on piix and q35

[Liu Ping Fan]

For pc-piix-*, hpet's intcap is always hard coded as IRQ2.
For q35, if it is pc-q35-1.7 and earlier, we use IRQ2 for compat
reason, otherwise IRQ2, IRQ8, and IRQ16~23 are allowed.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/i386/pc.c         | 20 +++++++++++++++-----
 hw/i386/pc_piix.c    |  7 ++++++-
 hw/i386/pc_q35.c     |  6 +++++-
 include/hw/i386/pc.h | 11 ++++++++++-
 4 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 72cc850..3e98ff0 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1219,7 +1219,8 @@ static const MemoryRegionOps ioportF0_io_ops = {
 void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
                           ISADevice **rtc_state,
                           ISADevice **floppy,
-                          bool no_vmport)
+                          bool no_vmport,
+                          bool hpet_irqs)
 {
     int i;
     DriveInfo *fd[MAX_FD];
@@ -1249,10 +1250,19 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
         /* In order to set property, here not using sysbus_try_create_simple */
         hpet = qdev_try_create(NULL, "hpet");
         if (hpet) {
-            /* tmp fix. For compat, hard code to IRQ2 until we have correct
-             * compat property and differentiate pc-iix with pc-q35
-             */
-            qdev_prop_set_uint32(hpet, HPET_INTCAP, 0x4);
+            /* For pc-piix-*, hpet's intcap is always IRQ2. */
+            if (!hpet_irqs) {
+                qdev_prop_set_uint32(hpet, HPET_INTCAP, 0x4);
+            } else {
+                /* For pc-q35-1.7 and earlier, use IRQ2 for compat.
+                 * Otherwise, use IRQ16~23, IRQ8 and IRQ2.
+                 */
+                uint8_t compat = object_property_get_int(OBJECT(hpet),
+                        HPET_INTCAP, NULL);
+                if (!compat) {
+                    qdev_prop_set_uint32(hpet, HPET_INTCAP, 0xff0104);
+                }
+            }
             qdev_init_nofail(hpet);
             sysbus_mmio_map(SYS_BUS_DEVICE(hpet), 0, HPET_BASE);
 
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index c6042c7..a45ce11 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -180,7 +180,8 @@ static void pc_init1(QEMUMachineInitArgs *args,
     pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
 
     /* init basic PC hardware */
-    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, xen_enabled());
+    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, xen_enabled(),
+        false);
 
     pc_nic_init(isa_bus, pci_bus);
 
@@ -346,6 +347,10 @@ static QEMUMachine pc_i440fx_machine_v1_7 = {
     .alias = "pc",
     .init = pc_init_pci,
     .is_default = 1,
+    .compat_props = (GlobalProperty[]) {
+        PC_COMPAT_1_7,
+        { /* end of list */ }
+    },
 };
 
 #define PC_I440FX_1_6_MACHINE_OPTIONS PC_I440FX_MACHINE_OPTIONS
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index ca84e1c..124ecc1 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -181,7 +181,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
     pc_register_ferr_irq(gsi[13]);
 
     /* init basic PC hardware */
-    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, false);
+    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, false, true);
 
     /* connect pm stuff to lpc */
     ich9_lpc_pm_init(lpc);
@@ -270,6 +270,10 @@ static QEMUMachine pc_q35_machine_v1_7 = {
     .name = "pc-q35-1.7",
     .alias = "q35",
     .init = pc_q35_init,
+    .compat_props = (GlobalProperty[]) {
+        PC_COMPAT_1_7,
+        { /* end of list */ }
+    },
 };
 
 #define PC_Q35_1_6_MACHINE_OPTIONS PC_Q35_MACHINE_OPTIONS
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 1361a27..bfeccf2 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -136,7 +136,8 @@ DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
 void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
                           ISADevice **rtc_state,
                           ISADevice **floppy,
-                          bool no_vmport);
+                          bool no_vmport,
+                          bool hpet_irqs);
 void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd);
 void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
                   const char *boot_device,
@@ -227,7 +228,15 @@ void pvpanic_init(ISABus *bus);
 
 int e820_add_entry(uint64_t, uint64_t, uint32_t);
 
+#define PC_COMPAT_1_7 \
+        {\
+            .driver   = "hpet",\
+            .property = HPET_INTCAP,\
+            .value    = stringify(4),\
+        }
+
 #define PC_COMPAT_1_6 \
+        PC_COMPAT_1_7, \
         {\
             .driver   = "e1000",\
             .property = "mitigation",\
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v7 4/4] PC: differentiate hpet's interrupt capability on piix and q35

[Michael S. Tsirkin]

On Thu, Oct 17, 2013 at 11:16:05AM +0800, Liu Ping Fan wrote:
> For pc-piix-*, hpet's intcap is always hard coded as IRQ2.
> For q35, if it is pc-q35-1.7 and earlier, we use IRQ2 for compat
> reason, otherwise IRQ2, IRQ8, and IRQ16~23 are allowed.
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/i386/pc.c         | 20 +++++++++++++++-----
>  hw/i386/pc_piix.c    |  7 ++++++-
>  hw/i386/pc_q35.c     |  6 +++++-
>  include/hw/i386/pc.h | 11 ++++++++++-
>  4 files changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 72cc850..3e98ff0 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1219,7 +1219,8 @@ static const MemoryRegionOps ioportF0_io_ops = {
>  void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>                            ISADevice **rtc_state,
>                            ISADevice **floppy,
> -                          bool no_vmport)
> +                          bool no_vmport,
> +                          bool hpet_irqs)
>  {
>      int i;
>      DriveInfo *fd[MAX_FD];
> @@ -1249,10 +1250,19 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>          /* In order to set property, here not using sysbus_try_create_simple */
>          hpet = qdev_try_create(NULL, "hpet");
>          if (hpet) {
> -            /* tmp fix. For compat, hard code to IRQ2 until we have correct
> -             * compat property and differentiate pc-iix with pc-q35
> -             */
> -            qdev_prop_set_uint32(hpet, HPET_INTCAP, 0x4);
> +            /* For pc-piix-*, hpet's intcap is always IRQ2. */
> +            if (!hpet_irqs) {

So for piix the property is ignored even if set.

> +                qdev_prop_set_uint32(hpet, HPET_INTCAP, 0x4);
> +            } else {
> +                /* For pc-q35-1.7 and earlier, use IRQ2 for compat.

Now you lost me. It's still 4 for 1.7?
You only target 1.8 then?

> +                 * Otherwise, use IRQ16~23, IRQ8 and IRQ2.
> +                 */
> +                uint8_t compat = object_property_get_int(OBJECT(hpet),
> +                        HPET_INTCAP, NULL);
> +                if (!compat) {
> +                    qdev_prop_set_uint32(hpet, HPET_INTCAP, 0xff0104);
> +                }
> +            }
>              qdev_init_nofail(hpet);
>              sysbus_mmio_map(SYS_BUS_DEVICE(hpet), 0, HPET_BASE);
>  
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index c6042c7..a45ce11 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -180,7 +180,8 @@ static void pc_init1(QEMUMachineInitArgs *args,
>      pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
>  
>      /* init basic PC hardware */
> -    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, xen_enabled());
> +    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, xen_enabled(),
> +        false);
>  
>      pc_nic_init(isa_bus, pci_bus);
>  
> @@ -346,6 +347,10 @@ static QEMUMachine pc_i440fx_machine_v1_7 = {
>      .alias = "pc",
>      .init = pc_init_pci,
>      .is_default = 1,
> +    .compat_props = (GlobalProperty[]) {
> +        PC_COMPAT_1_7,

So you add this property for PIIX but it's then ignored?
That's kind of ugly.
Not sure how to fix this. Paolo?

> +        { /* end of list */ }
> +    },
>  };
>  
>  #define PC_I440FX_1_6_MACHINE_OPTIONS PC_I440FX_MACHINE_OPTIONS
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index ca84e1c..124ecc1 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -181,7 +181,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>      pc_register_ferr_irq(gsi[13]);
>  
>      /* init basic PC hardware */
> -    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, false);
> +    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, false, true);
>  
>      /* connect pm stuff to lpc */
>      ich9_lpc_pm_init(lpc);
> @@ -270,6 +270,10 @@ static QEMUMachine pc_q35_machine_v1_7 = {
>      .name = "pc-q35-1.7",
>      .alias = "q35",
>      .init = pc_q35_init,
> +    .compat_props = (GlobalProperty[]) {
> +        PC_COMPAT_1_7,
> +        { /* end of list */ }
> +    },
>  };
>  
>  #define PC_Q35_1_6_MACHINE_OPTIONS PC_Q35_MACHINE_OPTIONS
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 1361a27..bfeccf2 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -136,7 +136,8 @@ DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
>  void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>                            ISADevice **rtc_state,
>                            ISADevice **floppy,
> -                          bool no_vmport);
> +                          bool no_vmport,
> +                          bool hpet_irqs);
>  void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd);
>  void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>                    const char *boot_device,
> @@ -227,7 +228,15 @@ void pvpanic_init(ISABus *bus);
>  
>  int e820_add_entry(uint64_t, uint64_t, uint32_t);
>  
> +#define PC_COMPAT_1_7 \
> +        {\
> +            .driver   = "hpet",\
> +            .property = HPET_INTCAP,\
> +            .value    = stringify(4),\
> +        }
> +
>  #define PC_COMPAT_1_6 \
> +        PC_COMPAT_1_7, \
>          {\
>              .driver   = "e1000",\
>              .property = "mitigation",\
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH v7 0/4] bugs fix for hpet

[Michael S. Tsirkin]

On Thu, Oct 17, 2013 at 11:16:01AM +0800, Liu Ping Fan wrote:
> v7:
>   use macro to define "intcap" in pc.h
>   (as to 3/4 and 4/4, I am not sure about whether to merge them or not, so keep them separate")
> 

Yes, please smash 2 3 and 4 together.
There's no need to waste time reviewing code that gets
deleted in the next patch.

> v6:
>   move the setting of intcap to board, and keep the init value as zero. (thanks for the discussion from Paolo and Michael)
>   introduce an extra hpet property "compat" to tell PC version
> 
> v5:
>   use stand compat property to fix hpet intcap on pc-q35, while on pc-piix, hard code intcap as IRQ2
> 
> v4:
>   use stand compat property to fix hpet intcap
> 
> v3:
>   change hpet interrupt capablity on board's demand
> 
> 
> 
> Liu Ping Fan (4):
>   hpet: inverse polarity when pin above ISA_NUM_IRQS
>   hpet: enable to entitle more irq pins for hpet
>   PC: use qdev_xx to create hpet instead of sysbus_create_xx
>   PC: differentiate hpet's interrupt capability on piix and q35
> 
>  hw/i386/pc.c         | 23 ++++++++++++++++++++---
>  hw/i386/pc_piix.c    |  7 ++++++-
>  hw/i386/pc_q35.c     |  6 +++++-
>  hw/timer/hpet.c      | 24 ++++++++++++++++++++----
>  include/hw/i386/pc.h | 13 ++++++++++++-
>  5 files changed, 63 insertions(+), 10 deletions(-)
> 
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH v7 4/4] PC: differentiate hpet's interrupt capability on piix and q35

[liu ping fan]

On Thu, Oct 17, 2013 at 1:44 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Oct 17, 2013 at 11:16:05AM +0800, Liu Ping Fan wrote:
>> For pc-piix-*, hpet's intcap is always hard coded as IRQ2.
>> For q35, if it is pc-q35-1.7 and earlier, we use IRQ2 for compat
>> reason, otherwise IRQ2, IRQ8, and IRQ16~23 are allowed.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  hw/i386/pc.c         | 20 +++++++++++++++-----
>>  hw/i386/pc_piix.c    |  7 ++++++-
>>  hw/i386/pc_q35.c     |  6 +++++-
>>  include/hw/i386/pc.h | 11 ++++++++++-
>>  4 files changed, 36 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 72cc850..3e98ff0 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1219,7 +1219,8 @@ static const MemoryRegionOps ioportF0_io_ops = {
>>  void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>>                            ISADevice **rtc_state,
>>                            ISADevice **floppy,
>> -                          bool no_vmport)
>> +                          bool no_vmport,
>> +                          bool hpet_irqs)
>>  {
>>      int i;
>>      DriveInfo *fd[MAX_FD];
>> @@ -1249,10 +1250,19 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>>          /* In order to set property, here not using sysbus_try_create_simple */
>>          hpet = qdev_try_create(NULL, "hpet");
>>          if (hpet) {
>> -            /* tmp fix. For compat, hard code to IRQ2 until we have correct
>> -             * compat property and differentiate pc-iix with pc-q35
>> -             */
>> -            qdev_prop_set_uint32(hpet, HPET_INTCAP, 0x4);
>> +            /* For pc-piix-*, hpet's intcap is always IRQ2. */
>> +            if (!hpet_irqs) {
>
> So for piix the property is ignored even if set.
>
Yes.
>> +                qdev_prop_set_uint32(hpet, HPET_INTCAP, 0x4);
>> +            } else {
>> +                /* For pc-q35-1.7 and earlier, use IRQ2 for compat.
>
> Now you lost me. It's still 4 for 1.7?
> You only target 1.8 then?
>
Yes, since the compat for guest. And start this fix with 1.8

>> +                 * Otherwise, use IRQ16~23, IRQ8 and IRQ2.
>> +                 */
>> +                uint8_t compat = object_property_get_int(OBJECT(hpet),
>> +                        HPET_INTCAP, NULL);
>> +                if (!compat) {
>> +                    qdev_prop_set_uint32(hpet, HPET_INTCAP, 0xff0104);
>> +                }
>> +            }
>>              qdev_init_nofail(hpet);
>>              sysbus_mmio_map(SYS_BUS_DEVICE(hpet), 0, HPET_BASE);
>>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index c6042c7..a45ce11 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -180,7 +180,8 @@ static void pc_init1(QEMUMachineInitArgs *args,
>>      pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
>>
>>      /* init basic PC hardware */
>> -    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, xen_enabled());
>> +    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, xen_enabled(),
>> +        false);
>>
>>      pc_nic_init(isa_bus, pci_bus);
>>
>> @@ -346,6 +347,10 @@ static QEMUMachine pc_i440fx_machine_v1_7 = {
>>      .alias = "pc",
>>      .init = pc_init_pci,
>>      .is_default = 1,
>> +    .compat_props = (GlobalProperty[]) {
>> +        PC_COMPAT_1_7,
>
> So you add this property for PIIX but it's then ignored?
> That's kind of ugly.

Yes a little ugly, but PIIX and ich9 diverge for hpet's hardware.

> Not sure how to fix this. Paolo?
>
>> +        { /* end of list */ }
>> +    },
>>  };
>>
>>  #define PC_I440FX_1_6_MACHINE_OPTIONS PC_I440FX_MACHINE_OPTIONS
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index ca84e1c..124ecc1 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -181,7 +181,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
>>      pc_register_ferr_irq(gsi[13]);
>>
>>      /* init basic PC hardware */
>> -    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, false);
>> +    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, false, true);
>>
>>      /* connect pm stuff to lpc */
>>      ich9_lpc_pm_init(lpc);
>> @@ -270,6 +270,10 @@ static QEMUMachine pc_q35_machine_v1_7 = {
>>      .name = "pc-q35-1.7",
>>      .alias = "q35",
>>      .init = pc_q35_init,
>> +    .compat_props = (GlobalProperty[]) {
>> +        PC_COMPAT_1_7,
>> +        { /* end of list */ }
>> +    },
>>  };
>>
>>  #define PC_Q35_1_6_MACHINE_OPTIONS PC_Q35_MACHINE_OPTIONS
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 1361a27..bfeccf2 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -136,7 +136,8 @@ DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
>>  void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>>                            ISADevice **rtc_state,
>>                            ISADevice **floppy,
>> -                          bool no_vmport);
>> +                          bool no_vmport,
>> +                          bool hpet_irqs);
>>  void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd);
>>  void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>>                    const char *boot_device,
>> @@ -227,7 +228,15 @@ void pvpanic_init(ISABus *bus);
>>
>>  int e820_add_entry(uint64_t, uint64_t, uint32_t);
>>
>> +#define PC_COMPAT_1_7 \
>> +        {\
>> +            .driver   = "hpet",\
>> +            .property = HPET_INTCAP,\
>> +            .value    = stringify(4),\
>> +        }
>> +
>>  #define PC_COMPAT_1_6 \
>> +        PC_COMPAT_1_7, \
>>          {\
>>              .driver   = "e1000",\
>>              .property = "mitigation",\
>> --
>> 1.8.1.4

Re: [Qemu-devel] [PATCH v7 4/4] PC: differentiate hpet's interrupt capability on piix and q35

[Michael S. Tsirkin]

On Thu, Oct 17, 2013 at 02:27:43PM +0800, liu ping fan wrote:
> On Thu, Oct 17, 2013 at 1:44 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Thu, Oct 17, 2013 at 11:16:05AM +0800, Liu Ping Fan wrote:
> >> For pc-piix-*, hpet's intcap is always hard coded as IRQ2.
> >> For q35, if it is pc-q35-1.7 and earlier, we use IRQ2 for compat
> >> reason, otherwise IRQ2, IRQ8, and IRQ16~23 are allowed.
> >>
> >> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> >> ---
> >>  hw/i386/pc.c         | 20 +++++++++++++++-----
> >>  hw/i386/pc_piix.c    |  7 ++++++-
> >>  hw/i386/pc_q35.c     |  6 +++++-
> >>  include/hw/i386/pc.h | 11 ++++++++++-
> >>  4 files changed, 36 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index 72cc850..3e98ff0 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -1219,7 +1219,8 @@ static const MemoryRegionOps ioportF0_io_ops = {
> >>  void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
> >>                            ISADevice **rtc_state,
> >>                            ISADevice **floppy,
> >> -                          bool no_vmport)
> >> +                          bool no_vmport,
> >> +                          bool hpet_irqs)
> >>  {
> >>      int i;
> >>      DriveInfo *fd[MAX_FD];
> >> @@ -1249,10 +1250,19 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
> >>          /* In order to set property, here not using sysbus_try_create_simple */
> >>          hpet = qdev_try_create(NULL, "hpet");
> >>          if (hpet) {
> >> -            /* tmp fix. For compat, hard code to IRQ2 until we have correct
> >> -             * compat property and differentiate pc-iix with pc-q35
> >> -             */
> >> -            qdev_prop_set_uint32(hpet, HPET_INTCAP, 0x4);
> >> +            /* For pc-piix-*, hpet's intcap is always IRQ2. */
> >> +            if (!hpet_irqs) {
> >
> > So for piix the property is ignored even if set.
> >
> Yes.
> >> +                qdev_prop_set_uint32(hpet, HPET_INTCAP, 0x4);
> >> +            } else {
> >> +                /* For pc-q35-1.7 and earlier, use IRQ2 for compat.
> >
> > Now you lost me. It's still 4 for 1.7?
> > You only target 1.8 then?
> >
> Yes, since the compat for guest. And start this fix with 1.8
> >> +                 * Otherwise, use IRQ16~23, IRQ8 and IRQ2.
> >> +                 */
> >> +                uint8_t compat = object_property_get_int(OBJECT(hpet),
> >> +                        HPET_INTCAP, NULL);
> >> +                if (!compat) {
> >> +                    qdev_prop_set_uint32(hpet, HPET_INTCAP, 0xff0104);
> >> +                }
> >> +            }
> >>              qdev_init_nofail(hpet);
> >>              sysbus_mmio_map(SYS_BUS_DEVICE(hpet), 0, HPET_BASE);
> >>
> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >> index c6042c7..a45ce11 100644
> >> --- a/hw/i386/pc_piix.c
> >> +++ b/hw/i386/pc_piix.c
> >> @@ -180,7 +180,8 @@ static void pc_init1(QEMUMachineInitArgs *args,
> >>      pc_vga_init(isa_bus, pci_enabled ? pci_bus : NULL);
> >>
> >>      /* init basic PC hardware */
> >> -    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, xen_enabled());
> >> +    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, xen_enabled(),
> >> +        false);
> >>
> >>      pc_nic_init(isa_bus, pci_bus);
> >>
> >> @@ -346,6 +347,10 @@ static QEMUMachine pc_i440fx_machine_v1_7 = {
> >>      .alias = "pc",
> >>      .init = pc_init_pci,
> >>      .is_default = 1,
> >> +    .compat_props = (GlobalProperty[]) {
> >> +        PC_COMPAT_1_7,
> >
> > So you add this property for PIIX but it's then ignored?
> > That's kind of ugly.
> 
> Yes a little ugly, but PIIX and ich9 diverge for hpet's hardware.

Maybe only add it for Q35 then?
Create PC_Q35_COMPAT_1_7 and use everywhere ...

> > Not sure how to fix this. Paolo?
> >
> >> +        { /* end of list */ }
> >> +    },
> >>  };
> >>
> >>  #define PC_I440FX_1_6_MACHINE_OPTIONS PC_I440FX_MACHINE_OPTIONS
> >> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> >> index ca84e1c..124ecc1 100644
> >> --- a/hw/i386/pc_q35.c
> >> +++ b/hw/i386/pc_q35.c
> >> @@ -181,7 +181,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
> >>      pc_register_ferr_irq(gsi[13]);
> >>
> >>      /* init basic PC hardware */
> >> -    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, false);
> >> +    pc_basic_device_init(isa_bus, gsi, &rtc_state, &floppy, false, true);
> >>
> >>      /* connect pm stuff to lpc */
> >>      ich9_lpc_pm_init(lpc);
> >> @@ -270,6 +270,10 @@ static QEMUMachine pc_q35_machine_v1_7 = {
> >>      .name = "pc-q35-1.7",
> >>      .alias = "q35",
> >>      .init = pc_q35_init,
> >> +    .compat_props = (GlobalProperty[]) {
> >> +        PC_COMPAT_1_7,
> >> +        { /* end of list */ }
> >> +    },
> >>  };
> >>
> >>  #define PC_Q35_1_6_MACHINE_OPTIONS PC_Q35_MACHINE_OPTIONS
> >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> >> index 1361a27..bfeccf2 100644
> >> --- a/include/hw/i386/pc.h
> >> +++ b/include/hw/i386/pc.h
> >> @@ -136,7 +136,8 @@ DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
> >>  void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
> >>                            ISADevice **rtc_state,
> >>                            ISADevice **floppy,
> >> -                          bool no_vmport);
> >> +                          bool no_vmport,
> >> +                          bool hpet_irqs);
> >>  void pc_init_ne2k_isa(ISABus *bus, NICInfo *nd);
> >>  void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
> >>                    const char *boot_device,
> >> @@ -227,7 +228,15 @@ void pvpanic_init(ISABus *bus);
> >>
> >>  int e820_add_entry(uint64_t, uint64_t, uint32_t);
> >>
> >> +#define PC_COMPAT_1_7 \
> >> +        {\
> >> +            .driver   = "hpet",\
> >> +            .property = HPET_INTCAP,\
> >> +            .value    = stringify(4),\
> >> +        }
> >> +
> >>  #define PC_COMPAT_1_6 \
> >> +        PC_COMPAT_1_7, \
> >>          {\
> >>              .driver   = "e1000",\
> >>              .property = "mitigation",\
> >> --
> >> 1.8.1.4

Re: [Qemu-devel] [PATCH v7 4/4] PC: differentiate hpet's interrupt capability on piix and q35

[Paolo Bonzini]

Il 17/10/2013 05:16, Liu Ping Fan ha scritto:
> +                          bool hpet_irqs)
>  {
>      int i;
>      DriveInfo *fd[MAX_FD];
> @@ -1249,10 +1250,19 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>          /* In order to set property, here not using sysbus_try_create_simple */
>          hpet = qdev_try_create(NULL, "hpet");
>          if (hpet) {
> -            /* tmp fix. For compat, hard code to IRQ2 until we have correct
> -             * compat property and differentiate pc-iix with pc-q35
> -             */
> -            qdev_prop_set_uint32(hpet, HPET_INTCAP, 0x4);
> +            /* For pc-piix-*, hpet's intcap is always IRQ2. */
> +            if (!hpet_irqs) {
> +                qdev_prop_set_uint32(hpet, HPET_INTCAP, 0x4);
> +            } else {
> +                /* For pc-q35-1.7 and earlier, use IRQ2 for compat.
> +                 * Otherwise, use IRQ16~23, IRQ8 and IRQ2.
> +                 */
> +                uint8_t compat = object_property_get_int(OBJECT(hpet),
> +                        HPET_INTCAP, NULL);
> +                if (!compat) {
> +                    qdev_prop_set_uint32(hpet, HPET_INTCAP, 0xff0104);
> +                }
> +            }

Simpler:

    if (hpet) {
        uint8_t compat = object_property_get_int(OBJECT(hpet),
                    HPET_INTCAP, NULL);
        if (!compat) {
             qdev_prop_set_uint32(hpet, HPET_INTCAP, 0xff0104);
        }
    }

and just pass 0x4 or 0xff0104 to the function.

I think the "unused" property is okay.  It is a matter of fact that
before these patches intcap was hardcoded to 0x4.  Who cares if it
remains 0x4 in some cases.

Note that when this will go into 1.8 you will need to add the 1.8 machines.

Paolo

Re: [Qemu-devel] [PATCH v7 4/4] PC: differentiate hpet's interrupt capability on piix and q35

[Paolo Bonzini]

Il 17/10/2013 05:16, Liu Ping Fan ha scritto:
> +                          bool hpet_irqs)
>  {
>      int i;
>      DriveInfo *fd[MAX_FD];
> @@ -1249,10 +1250,19 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>          /* In order to set property, here not using sysbus_try_create_simple */
>          hpet = qdev_try_create(NULL, "hpet");
>          if (hpet) {
> -            /* tmp fix. For compat, hard code to IRQ2 until we have correct
> -             * compat property and differentiate pc-iix with pc-q35
> -             */
> -            qdev_prop_set_uint32(hpet, HPET_INTCAP, 0x4);
> +            /* For pc-piix-*, hpet's intcap is always IRQ2. */
> +            if (!hpet_irqs) {
> +                qdev_prop_set_uint32(hpet, HPET_INTCAP, 0x4);
> +            } else {
> +                /* For pc-q35-1.7 and earlier, use IRQ2 for compat.
> +                 * Otherwise, use IRQ16~23, IRQ8 and IRQ2.
> +                 */
> +                uint8_t compat = object_property_get_int(OBJECT(hpet),
> +                        HPET_INTCAP, NULL);
> +                if (!compat) {
> +                    qdev_prop_set_uint32(hpet, HPET_INTCAP, 0xff0104);
> +                }
> +            }

Simpler:

    if (hpet) {
        uint8_t compat = object_property_get_int(OBJECT(hpet),
                    HPET_INTCAP, NULL);
        if (!compat) {
             qdev_prop_set_uint32(hpet, HPET_INTCAP, hpet_irqs);
        }
    }

and just pass 0x4 or 0xff0104 to the function.

I think the "unused" property is okay.  It is a matter of fact that
before these patches intcap was hardcoded to 0x4.  Who cares if it
remains 0x4 in some cases.

Note that when this will go into 1.8 you will need to add the 1.8 machines.

Paolo