[Qemu-devel] [PATCH 1/2] hpet: entitle more irq pins for hpet

[Qemu-devel] [PATCH 1/2] 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.

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

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 648b383..cd95d39 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -41,6 +41,8 @@
 #endif
 
 #define HPET_MSI_SUPPORT        0
+/* Hpet can use non-legacy IRQ16~23, and an IRQ2 ,IRQ8 */
+#define HPET_TN_INT_CAP (0xff0104ULL << 32)
 
 #define TYPE_HPET "hpet"
 #define HPET(obj) OBJECT_CHECK(HPETState, (obj), TYPE_HPET)
@@ -653,8 +655,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 |=  HPET_TN_INT_CAP;
         timer->period = 0ULL;
         timer->wrap_flag = 0;
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH 2/2] 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>
---
kernel has a bug with ioapic, refer to
   https://lkml.org/lkml/2013/8/23/98
With all these patch, linux-2.6/Documentation/timers/hpet_example.c can work
on qemu 
---
 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 cd95d39..a6626e2 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -200,13 +200,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

Re: [Qemu-devel] [PATCH 2/2] hpet: inverse polarity when pin above ISA_NUM_IRQS

[Paolo Bonzini]

Il 25/08/2013 04:16, Liu Ping Fan ha scritto:
> 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>
> ---
> kernel has a bug with ioapic, refer to
>    https://lkml.org/lkml/2013/8/23/98
> With all these patch, linux-2.6/Documentation/timers/hpet_example.c can work
> on qemu 

Can you explain "in qemu q35 machine ioapic's ioredtbl[x] can never be
set as low-active, even if the hpet driver registered it"?

Paolo

Re: [Qemu-devel] [PATCH 1/2] hpet: entitle more irq pins for hpet

[Paolo Bonzini]

Il 25/08/2013 04:16, Liu Ping Fan ha scritto:
> 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.
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/timer/hpet.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 648b383..cd95d39 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -41,6 +41,8 @@
>  #endif
>  
>  #define HPET_MSI_SUPPORT        0
> +/* Hpet can use non-legacy IRQ16~23, and an IRQ2 ,IRQ8 */
> +#define HPET_TN_INT_CAP (0xff0104ULL << 32)
>  
>  #define TYPE_HPET "hpet"
>  #define HPET(obj) OBJECT_CHECK(HPETState, (obj), TYPE_HPET)
> @@ -653,8 +655,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 |=  HPET_TN_INT_CAP;
>          timer->period = 0ULL;
>          timer->wrap_flag = 0;
>      }
> 

These high 32-bits of timer->config need to be a property of the HPET
devices, so that the old value (4) is used when running with old machine
types.  Also, this patch must be the second, not the first, otherwise
you have a commit with btoken polarity IRQs.

Paolo

Re: [Qemu-devel] [PATCH 2/2] hpet: inverse polarity when pin above ISA_NUM_IRQS

[liu ping fan]

On Sun, Aug 25, 2013 at 2:44 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 25/08/2013 04:16, Liu Ping Fan ha scritto:
>> 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>
>> ---
>> kernel has a bug with ioapic, refer to
>>    https://lkml.org/lkml/2013/8/23/98
>> With all these patch, linux-2.6/Documentation/timers/hpet_example.c can work
>> on qemu
>
> Can you explain "in qemu q35 machine ioapic's ioredtbl[x] can never be
> set as low-active, even if the hpet driver registered it"?
>
>From kernel side, in drivers/char/hpet.c
hpet_timer_set_irq()
                gsi = acpi_register_gsi(NULL, irq, ACPI_LEVEL_SENSITIVE,
                                        ACPI_ACTIVE_LOW);

So we expect the ioredtbl[x] will be set as low-active polarity, but
on q35, I found this did not happen. Scene: a former call to
io_apic_setup_irq_pin_once(), then the later one(hpet, which decided
to share this pin with other device) will not actually program the
pin.  Here, notice the name  xx_once(), but it ignored the second
request without a check.

Regards,
Pingfan

> Paolo

Re: [Qemu-devel] [PATCH 1/2] hpet: entitle more irq pins for hpet

[liu ping fan]

On Sun, Aug 25, 2013 at 2:45 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 25/08/2013 04:16, Liu Ping Fan ha scritto:
>> 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.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  hw/timer/hpet.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
>> index 648b383..cd95d39 100644
>> --- a/hw/timer/hpet.c
>> +++ b/hw/timer/hpet.c
>> @@ -41,6 +41,8 @@
>>  #endif
>>
>>  #define HPET_MSI_SUPPORT        0
>> +/* Hpet can use non-legacy IRQ16~23, and an IRQ2 ,IRQ8 */
>> +#define HPET_TN_INT_CAP (0xff0104ULL << 32)
>>
>>  #define TYPE_HPET "hpet"
>>  #define HPET(obj) OBJECT_CHECK(HPETState, (obj), TYPE_HPET)
>> @@ -653,8 +655,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 |=  HPET_TN_INT_CAP;
>>          timer->period = 0ULL;
>>          timer->wrap_flag = 0;
>>      }
>>
>
> These high 32-bits of timer->config need to be a property of the HPET
> devices, so that the old value (4) is used when running with old machine

Sorry, but I had included old value (4) in macro HPET_TN_INT_CAP.
> types.  Also, this patch must be the second, not the first, otherwise
> you have a commit with btoken polarity IRQs.
>
Will fix

Thx,
Pingfan

Re: [Qemu-devel] [PATCH 1/2] hpet: entitle more irq pins for hpet

[Paolo Bonzini]

Il 26/08/2013 04:53, liu ping fan ha scritto:
> On Sun, Aug 25, 2013 at 2:45 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 25/08/2013 04:16, Liu Ping Fan ha scritto:
>>> 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.
>>>
>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>> ---
>>>  hw/timer/hpet.c | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
>>> index 648b383..cd95d39 100644
>>> --- a/hw/timer/hpet.c
>>> +++ b/hw/timer/hpet.c
>>> @@ -41,6 +41,8 @@
>>>  #endif
>>>
>>>  #define HPET_MSI_SUPPORT        0
>>> +/* Hpet can use non-legacy IRQ16~23, and an IRQ2 ,IRQ8 */
>>> +#define HPET_TN_INT_CAP (0xff0104ULL << 32)
>>>
>>>  #define TYPE_HPET "hpet"
>>>  #define HPET(obj) OBJECT_CHECK(HPETState, (obj), TYPE_HPET)
>>> @@ -653,8 +655,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 |=  HPET_TN_INT_CAP;
>>>          timer->period = 0ULL;
>>>          timer->wrap_flag = 0;
>>>      }
>>>
>>
>> These high 32-bits of timer->config need to be a property of the HPET
>> devices, so that the old value (4) is used when running with old machine
> 
> Sorry, but I had included old value (4) in macro HPET_TN_INT_CAP.

No, *only* GSI 2 must be available on old machine types (pc-1.6 and older).

Paolo

Re: [Qemu-devel] [PATCH 1/2] hpet: entitle more irq pins for hpet

[liu ping fan]

On Mon, Aug 26, 2013 at 3:59 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 26/08/2013 04:53, liu ping fan ha scritto:
>> On Sun, Aug 25, 2013 at 2:45 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> Il 25/08/2013 04:16, Liu Ping Fan ha scritto:
>>>> 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.
>>>>
>>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>> ---
>>>>  hw/timer/hpet.c | 6 ++++--
>>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
>>>> index 648b383..cd95d39 100644
>>>> --- a/hw/timer/hpet.c
>>>> +++ b/hw/timer/hpet.c
>>>> @@ -41,6 +41,8 @@
>>>>  #endif
>>>>
>>>>  #define HPET_MSI_SUPPORT        0
>>>> +/* Hpet can use non-legacy IRQ16~23, and an IRQ2 ,IRQ8 */
>>>> +#define HPET_TN_INT_CAP (0xff0104ULL << 32)
>>>>
>>>>  #define TYPE_HPET "hpet"
>>>>  #define HPET(obj) OBJECT_CHECK(HPETState, (obj), TYPE_HPET)
>>>> @@ -653,8 +655,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 |=  HPET_TN_INT_CAP;
>>>>          timer->period = 0ULL;
>>>>          timer->wrap_flag = 0;
>>>>      }
>>>>
>>>
>>> These high 32-bits of timer->config need to be a property of the HPET
>>> devices, so that the old value (4) is used when running with old machine
>>
>> Sorry, but I had included old value (4) in macro HPET_TN_INT_CAP.
>
> No, *only* GSI 2 must be available on old machine types (pc-1.6 and older).
>
Oh, got your meaning, will fix.

Thx,
Pingfan