[Qemu-devel] [PATCH v4 0/3] bugs fix for hpet

[Qemu-devel] [PATCH v4 0/3] bugs fix for hpet

[Liu Ping Fan]

note: I rebase it onto Stefan's net-next tree, since pc-1.7 has already been defined there.

v4:
  use standard compat property to set hpet's interrupt compatibility

v3:
  change hpet interrupt capablity on board's demand


Liu Ping Fan (3):
  hpet: inverse polarity when pin above ISA_NUM_IRQS
  hpet: entitle more irq pins for hpet
  pc-1.6: add compatibility for hpet intcap on pc-*-1.6

 hw/timer/hpet.c      | 27 +++++++++++++++++++++++----
 include/hw/i386/pc.h |  5 +++++
 2 files changed, 28 insertions(+), 4 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH v4 1/3] 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.(And this is observed on
bare metal).

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 648b383..1139448 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 v4 2/3] hpet: entitle more irq pins for hpet

[Liu Ping Fan]

On PC, 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.
(Will enable them after introducing pc 1.6 compat)

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

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 1139448..888be66 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -25,6 +25,7 @@
  */
 
 #include "hw/hw.h"
+#include "hw/boards.h"
 #include "hw/i386/pc.h"
 #include "ui/console.h"
 #include "qemu/timer.h"
@@ -42,6 +43,12 @@
 
 #define HPET_MSI_SUPPORT        0
 
+/* For bug compat, using only IRQ2. Soon it will be fixed as
+ * 0xff0104ULL, i.e using IRQ16~23, IRQ8 and IRQ2 after
+ * introducing pc-1.6 compat.
+ */
+#define HPET_TN_INT_CAP_DEFAULT 0x4ULL
+
 #define TYPE_HPET "hpet"
 #define HPET(obj) OBJECT_CHECK(HPETState, (obj), TYPE_HPET)
 
@@ -73,6 +80,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 +671,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;
     }
@@ -753,6 +761,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("intcap", HPETState, intcap, HPET_TN_INT_CAP_DEFAULT),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
1.8.1.4

[Qemu-devel] [PATCH v4 3/3] pc-1.6: add compatibility for hpet intcap on pc-*-1.6

[Liu Ping Fan]

For guest bug compat, we limit hpet's interrupt compatibility on
ioapic's IRQ2 for pc-*-1.6. As to pc-*-1.7 and newer, IRQ2, IRQ8,
and IRQ16~23 are allowed.

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

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 888be66..b6e8c12 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -43,11 +43,7 @@
 
 #define HPET_MSI_SUPPORT        0
 
-/* For bug compat, using only IRQ2. Soon it will be fixed as
- * 0xff0104ULL, i.e using IRQ16~23, IRQ8 and IRQ2 after
- * introducing pc-1.6 compat.
- */
-#define HPET_TN_INT_CAP_DEFAULT 0x4ULL
+#define HPET_TN_INT_CAP_DEFAULT 0xff0104ULL
 
 #define TYPE_HPET "hpet"
 #define HPET(obj) OBJECT_CHECK(HPETState, (obj), TYPE_HPET)
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 894c124..ef481bc 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -219,6 +219,10 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
             .driver   = "e1000",\
             .property = "mitigation",\
             .value    = "off",\
+        },{\
+            .driver   = "hpet",\
+            .property = "intcap",\
+            .value    = stringify(4),\
         }
 
 #define PC_COMPAT_1_5 \
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v4 0/3] bugs fix for hpet

[Stefan Hajnoczi]

On Mon, Sep 02, 2013 at 03:06:08PM +0800, Liu Ping Fan wrote:
> note: I rebase it onto Stefan's net-next tree, since pc-1.7 has already been defined there.

I have just sent the pull request so it can be merged into
qemu.git/master.

Stefan

Re: [Qemu-devel] [PATCH v4 0/3] bugs fix for hpet

[Paolo Bonzini]

Il 02/09/2013 09:06, Liu Ping Fan ha scritto:
> note: I rebase it onto Stefan's net-next tree, since pc-1.7 has already been defined there.
> 
> v4:
>   use standard compat property to set hpet's interrupt compatibility
> 
> v3:
>   change hpet interrupt capablity on board's demand
> 
> 
> Liu Ping Fan (3):
>   hpet: inverse polarity when pin above ISA_NUM_IRQS
>   hpet: entitle more irq pins for hpet
>   pc-1.6: add compatibility for hpet intcap on pc-*-1.6
> 
>  hw/timer/hpet.c      | 27 +++++++++++++++++++++++----
>  include/hw/i386/pc.h |  5 +++++
>  2 files changed, 28 insertions(+), 4 deletions(-)
> 

Looks good.  But I have one question; should this be changed for PIIX
too, or should the 1.7 PIIX machine keep the old behavior?  (I have no
idea).

Paolo

Re: [Qemu-devel] [PATCH v4 0/3] bugs fix for hpet

[liu ping fan]

On Tue, Sep 3, 2013 at 7:17 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 02/09/2013 09:06, Liu Ping Fan ha scritto:
>> note: I rebase it onto Stefan's net-next tree, since pc-1.7 has already been defined there.
>>
>> v4:
>>   use standard compat property to set hpet's interrupt compatibility
>>
>> v3:
>>   change hpet interrupt capablity on board's demand
>>
>>
>> Liu Ping Fan (3):
>>   hpet: inverse polarity when pin above ISA_NUM_IRQS
>>   hpet: entitle more irq pins for hpet
>>   pc-1.6: add compatibility for hpet intcap on pc-*-1.6
>>
>>  hw/timer/hpet.c      | 27 +++++++++++++++++++++++----
>>  include/hw/i386/pc.h |  5 +++++
>>  2 files changed, 28 insertions(+), 4 deletions(-)
>>
>
> Looks good.  But I have one question; should this be changed for PIIX
> too, or should the 1.7 PIIX machine keep the old behavior?  (I have no
> idea).
>
Your suspicion is right.  When going through PIIX4 spec, I found that
the chipset was without ioapic integrated. So there is divergence for
the compatibility of pc-piix-* and pc-q35-*.  Can I code the hpet's
compatiblity in pc-piix-1.7 to resolve this?

Regards,
Pingfan
> Paolo

Re: [Qemu-devel] [PATCH v4 0/3] bugs fix for hpet

[Paolo Bonzini]

Il 04/09/2013 07:29, liu ping fan ha scritto:
> On Tue, Sep 3, 2013 at 7:17 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 02/09/2013 09:06, Liu Ping Fan ha scritto:
>>> note: I rebase it onto Stefan's net-next tree, since pc-1.7 has already been defined there.
>>>
>>> v4:
>>>   use standard compat property to set hpet's interrupt compatibility
>>>
>>> v3:
>>>   change hpet interrupt capablity on board's demand
>>>
>>>
>>> Liu Ping Fan (3):
>>>   hpet: inverse polarity when pin above ISA_NUM_IRQS
>>>   hpet: entitle more irq pins for hpet
>>>   pc-1.6: add compatibility for hpet intcap on pc-*-1.6
>>>
>>>  hw/timer/hpet.c      | 27 +++++++++++++++++++++++----
>>>  include/hw/i386/pc.h |  5 +++++
>>>  2 files changed, 28 insertions(+), 4 deletions(-)
>>>
>>
>> Looks good.  But I have one question; should this be changed for PIIX
>> too, or should the 1.7 PIIX machine keep the old behavior?  (I have no
>> idea).
>>
> Your suspicion is right.  When going through PIIX4 spec, I found that
> the chipset was without ioapic integrated. So there is divergence for
> the compatibility of pc-piix-* and pc-q35-*.  Can I code the hpet's
> compatiblity in pc-piix-1.7 to resolve this?

You could do something like what people suggested earlier in the review.
 Change sysbus_try_create_simple to
qdev_try_create/sysbus_mmio_map/qdev_init in pc_basic_device_init (this
is one patch, going before v4 3/3).  Then add a new argument to
pc_basic_device_init, and set the intcap property if the new argument is
nonzero (another patch, going before v4 3/3; where the new argument is
always 0).  Finally, piix can pass 4 for the new argument, while q35 can
keep passing 0 (squashed in v4 3/3).

In other words, PIIX hardcodes the value 4, while Q35 uses compat
properties.

I still have a doubt.  Can the PIIX HPET use interrupt 8?  If so, this
won't work and we have to go back to the drawing board.  But if it can
use interrupt 2 only, it will be okay.

Paolo

Re: [Qemu-devel] [PATCH v4 0/3] bugs fix for hpet

[liu ping fan]

On Wed, Sep 4, 2013 at 3:03 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 04/09/2013 07:29, liu ping fan ha scritto:
>> On Tue, Sep 3, 2013 at 7:17 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> Il 02/09/2013 09:06, Liu Ping Fan ha scritto:
>>>> note: I rebase it onto Stefan's net-next tree, since pc-1.7 has already been defined there.
>>>>
>>>> v4:
>>>>   use standard compat property to set hpet's interrupt compatibility
>>>>
>>>> v3:
>>>>   change hpet interrupt capablity on board's demand
>>>>
>>>>
>>>> Liu Ping Fan (3):
>>>>   hpet: inverse polarity when pin above ISA_NUM_IRQS
>>>>   hpet: entitle more irq pins for hpet
>>>>   pc-1.6: add compatibility for hpet intcap on pc-*-1.6
>>>>
>>>>  hw/timer/hpet.c      | 27 +++++++++++++++++++++++----
>>>>  include/hw/i386/pc.h |  5 +++++
>>>>  2 files changed, 28 insertions(+), 4 deletions(-)
>>>>
>>>
>>> Looks good.  But I have one question; should this be changed for PIIX
>>> too, or should the 1.7 PIIX machine keep the old behavior?  (I have no
>>> idea).
>>>
>> Your suspicion is right.  When going through PIIX4 spec, I found that
>> the chipset was without ioapic integrated. So there is divergence for
>> the compatibility of pc-piix-* and pc-q35-*.  Can I code the hpet's
>> compatiblity in pc-piix-1.7 to resolve this?
>
> You could do something like what people suggested earlier in the review.
>  Change sysbus_try_create_simple to
> qdev_try_create/sysbus_mmio_map/qdev_init in pc_basic_device_init (this
> is one patch, going before v4 3/3).  Then add a new argument to
> pc_basic_device_init, and set the intcap property if the new argument is
> nonzero (another patch, going before v4 3/3; where the new argument is
> always 0).  Finally, piix can pass 4 for the new argument, while q35 can
> keep passing 0 (squashed in v4 3/3).
>
Will follow this, thanks.

> In other words, PIIX hardcodes the value 4, while Q35 uses compat
> properties.
>
> I still have a doubt.  Can the PIIX HPET use interrupt 8?  If so, this

No. on ich9, hpet can work with LegacyReplacement Mode to block
rtc(IRQ8). But on PIIX, spec said nothing about this, so I think we
had better keep it unchanged.

Thanks and regards,
Pingfan
> won't work and we have to go back to the drawing board.  But if it can
> use interrupt 2 only, it will be okay.
>
> Paolo