[Qemu-devel] [PATCH v5 0/5] bugs fix for hpet

[Qemu-devel] [PATCH v5 0/5] bugs fix for hpet

[Liu Ping Fan]

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 (5):
  hpet: inverse polarity when pin above ISA_NUM_IRQS
  hpet: 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
  PC-1.6: add compatibility for hpet intcap on pc-q35-1.6

 hw/i386/pc.c         | 17 ++++++++++++++---
 hw/i386/pc_piix.c    |  3 ++-
 hw/i386/pc_q35.c     |  2 +-
 hw/timer/hpet.c      | 24 ++++++++++++++++++++----
 include/hw/i386/pc.h |  7 ++++++-
 5 files changed, 43 insertions(+), 10 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH v5 1/5] 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 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

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

[Michael S. Tsirkin]

On Thu, Sep 12, 2013 at 11:25:14AM +0800, Liu Ping Fan wrote:
> 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).

How does one test this 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>


Doesn't this affect cross-version migration?
E.g. imagine that you migrate between systems
with/without this fix.

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

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

[liu ping fan]

On Sun, Sep 29, 2013 at 3:52 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Sep 12, 2013 at 11:25:14AM +0800, Liu Ping Fan wrote:
>> 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).
>
> How does one test this on bare metal?
>
If changing the func hpet_timer_set_irq() in linux's hpet driver,
from acpi_register_gsi(NULL, irq, ACPI_LEVEL_SENSITIVE,
ACPI_ACTIVE_LOW);  to ACPI_ACTIVE_HIGH,  then run hpet_example.c on
bare metal, the modified kernel will complain about spurious irq and
disable the irq line.
>>
>> We fold the emulation of this inversion inside the hpet logic.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>
>
> Doesn't this affect cross-version migration?
> E.g. imagine that you migrate between systems
> with/without this fix.
>
No. the changing only affect "route >= ISA_NUM_IRQS",  For linux
guest, it use IRQ2/8 for hpet0/hpet1 which is reserved by kernel. It
work without this fix (I think windows is the same). But the
hpet_example.c(in linux) can not work without this fix. So no such
run-time instance before this bug fix.

Regards,
Pingfan

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

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

[Michael S. Tsirkin]

On Sun, Sep 29, 2013 at 11:25:24AM +0800, liu ping fan wrote:
> On Sun, Sep 29, 2013 at 3:52 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Thu, Sep 12, 2013 at 11:25:14AM +0800, Liu Ping Fan wrote:
> >> 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).
> >
> > How does one test this on bare metal?
> >
> If changing the func hpet_timer_set_irq() in linux's hpet driver,
> from acpi_register_gsi(NULL, irq, ACPI_LEVEL_SENSITIVE,
> ACPI_ACTIVE_LOW);  to ACPI_ACTIVE_HIGH,  then run hpet_example.c on
> bare metal, the modified kernel will complain about spurious irq and
> disable the irq line.

ok that's useful info for the changelog.

> >>
> >> We fold the emulation of this inversion inside the hpet logic.
> >>
> >> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> >
> >
> > Doesn't this affect cross-version migration?
> > E.g. imagine that you migrate between systems
> > with/without this fix.
> >
> No. the changing only affect "route >= ISA_NUM_IRQS",  For linux
> guest, it use IRQ2/8 for hpet0/hpet1 which is reserved by kernel. It
> work without this fix (I think windows is the same). But the
> hpet_example.c(in linux) can not work without this fix. So no such
> run-time instance before this bug fix.
> 
> Regards,
> Pingfan

aha so the argument is it's already too broken to even mostly work,
we don't need to worry about migrating it to/from old 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 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
> >>

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

[liu ping fan]

On Sun, Sep 29, 2013 at 12:20 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Sep 29, 2013 at 11:25:24AM +0800, liu ping fan wrote:
>> On Sun, Sep 29, 2013 at 3:52 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Thu, Sep 12, 2013 at 11:25:14AM +0800, Liu Ping Fan wrote:
>> >> 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).
>> >
>> > How does one test this on bare metal?
>> >
>> If changing the func hpet_timer_set_irq() in linux's hpet driver,
>> from acpi_register_gsi(NULL, irq, ACPI_LEVEL_SENSITIVE,
>> ACPI_ACTIVE_LOW);  to ACPI_ACTIVE_HIGH,  then run hpet_example.c on
>> bare metal, the modified kernel will complain about spurious irq and
>> disable the irq line.
>
> ok that's useful info for the changelog.
>
Will document them.

>> >>
>> >> We fold the emulation of this inversion inside the hpet logic.
>> >>
>> >> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> >
>> >
>> > Doesn't this affect cross-version migration?
>> > E.g. imagine that you migrate between systems
>> > with/without this fix.
>> >
>> No. the changing only affect "route >= ISA_NUM_IRQS",  For linux
>> guest, it use IRQ2/8 for hpet0/hpet1 which is reserved by kernel. It
>> work without this fix (I think windows is the same). But the
>> hpet_example.c(in linux) can not work without this fix. So no such
>> run-time instance before this bug fix.
>>
>> Regards,
>> Pingfan
>
> aha so the argument is it's already too broken to even mostly work,
> we don't need to worry about migrating it to/from old qemu.
>
Yes :)

Thanks,
Pingfan

[Qemu-devel] [PATCH v5 2/5] 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 8429eb3..46903b9 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

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

[Michael S. Tsirkin]

On Thu, Sep 12, 2013 at 11:25:15AM +0800, Liu Ping Fan wrote:
> 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 8429eb3..46903b9 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

So users are expected to stick a bitmask of legal
pins here?
I think that's a bit too much rope to give to users.
Don't you think?

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

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

[liu ping fan]

On Sun, Sep 29, 2013 at 3:56 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Sep 12, 2013 at 11:25:15AM +0800, Liu Ping Fan wrote:
>> 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 8429eb3..46903b9 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
>
> So users are expected to stick a bitmask of legal
> pins here?
> I think that's a bit too much rope to give to users.
> Don't you think?
>
Sorry, not understand your meaning exactly.  But the scene will be:
guest kernel polls the ability bitmask, and pick up one pin which is
not occupied or can be shared with the level-trigger and low-active.
So is it rope?

Thanks and regards,
Pingfan
>> 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
>>

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

[Michael S. Tsirkin]

On Sun, Sep 29, 2013 at 11:49:41AM +0800, liu ping fan wrote:
> On Sun, Sep 29, 2013 at 3:56 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Thu, Sep 12, 2013 at 11:25:15AM +0800, Liu Ping Fan wrote:
> >> 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 8429eb3..46903b9 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
> >
> > So users are expected to stick a bitmask of legal
> > pins here?
> > I think that's a bit too much rope to give to users.
> > Don't you think?
> >
> Sorry, not understand your meaning exactly.  But the scene will be:
> guest kernel polls the ability bitmask, and pick up one pin which is
> not occupied or can be shared with the level-trigger and low-active.
> So is it rope?

I merely say that it's better to make this a bool or bit property.
UINT32 is too much flexibility imho.

> Thanks and regards,
> Pingfan
> >> 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
> >>

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

[liu ping fan]

On Sun, Sep 29, 2013 at 12:15 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Sep 29, 2013 at 11:49:41AM +0800, liu ping fan wrote:
>> On Sun, Sep 29, 2013 at 3:56 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Thu, Sep 12, 2013 at 11:25:15AM +0800, Liu Ping Fan wrote:
>> >> 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 8429eb3..46903b9 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
>> >
>> > So users are expected to stick a bitmask of legal
>> > pins here?
>> > I think that's a bit too much rope to give to users.
>> > Don't you think?
>> >
>> Sorry, not understand your meaning exactly.  But the scene will be:
>> guest kernel polls the ability bitmask, and pick up one pin which is
>> not occupied or can be shared with the level-trigger and low-active.
>> So is it rope?
>
> I merely say that it's better to make this a bool or bit property.
> UINT32 is too much flexibility imho.
>
The interrupt capability is export to guest by register
Tn_INT_ROUTE_CAP[63:32]. So it is useless to make them as a bit
property. Do you think so?

Regards
Pingfan

config register in hpet con
>> Thanks and regards,
>> Pingfan
>> >> 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
>> >>

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

[Michael S. Tsirkin]

On Mon, Sep 30, 2013 at 04:02:29PM +0800, liu ping fan wrote:
> On Sun, Sep 29, 2013 at 12:15 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Sep 29, 2013 at 11:49:41AM +0800, liu ping fan wrote:
> >> On Sun, Sep 29, 2013 at 3:56 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > On Thu, Sep 12, 2013 at 11:25:15AM +0800, Liu Ping Fan wrote:
> >> >> 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 8429eb3..46903b9 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
> >> >
> >> > So users are expected to stick a bitmask of legal
> >> > pins here?
> >> > I think that's a bit too much rope to give to users.
> >> > Don't you think?
> >> >
> >> Sorry, not understand your meaning exactly.  But the scene will be:
> >> guest kernel polls the ability bitmask, and pick up one pin which is
> >> not occupied or can be shared with the level-trigger and low-active.
> >> So is it rope?
> >
> > I merely say that it's better to make this a bool or bit property.
> > UINT32 is too much flexibility imho.
> >
> The interrupt capability is export to guest by register
> Tn_INT_ROUTE_CAP[63:32]. So it is useless to make them as a bit
> property. Do you think so?
> 
> Regards
> Pingfan

I think we merely need to support two modes:
- qemu 1.6 and older compatible
- compatible to actual hardware

Why would we let users configure an arbitrary
configuration which isn't compatible to either
old qemu or real hardware?


> config register in hpet con
> >> Thanks and regards,
> >> Pingfan
> >> >> 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
> >> >>

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

[Paolo Bonzini]

Il 30/09/2013 11:06, Michael S. Tsirkin ha scritto:
> On Mon, Sep 30, 2013 at 04:02:29PM +0800, liu ping fan wrote:
>> On Sun, Sep 29, 2013 at 12:15 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> On Sun, Sep 29, 2013 at 11:49:41AM +0800, liu ping fan wrote:
>>>> On Sun, Sep 29, 2013 at 3:56 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>> On Thu, Sep 12, 2013 at 11:25:15AM +0800, Liu Ping Fan wrote:
>>>>>> 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 8429eb3..46903b9 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
>>>>>
>>>>> So users are expected to stick a bitmask of legal
>>>>> pins here?
>>>>> I think that's a bit too much rope to give to users.
>>>>> Don't you think?
>>>>>
>>>> Sorry, not understand your meaning exactly.  But the scene will be:
>>>> guest kernel polls the ability bitmask, and pick up one pin which is
>>>> not occupied or can be shared with the level-trigger and low-active.
>>>> So is it rope?
>>>
>>> I merely say that it's better to make this a bool or bit property.
>>> UINT32 is too much flexibility imho.
>>>
>> The interrupt capability is export to guest by register
>> Tn_INT_ROUTE_CAP[63:32]. So it is useless to make them as a bit
>> property. Do you think so?
>>
>> Regards
>> Pingfan
> 
> I think we merely need to support two modes:
> - qemu 1.6 and older compatible
> - compatible to actual hardware
> 
> Why would we let users configure an arbitrary
> configuration which isn't compatible to either
> old qemu or real hardware?

The actual setting depends on the chipset.  For example, the "real"
PIIX4 is the same as the QEMU PIIX4, only Q35 uses the new value.

If in the future we had a chipset with more than 24 GSIs, you would have
a third possibility.

Paolo

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

[Michael S. Tsirkin]

On Mon, Sep 30, 2013 at 11:06:48AM +0200, Paolo Bonzini wrote:
> Il 30/09/2013 11:06, Michael S. Tsirkin ha scritto:
> > On Mon, Sep 30, 2013 at 04:02:29PM +0800, liu ping fan wrote:
> >> On Sun, Sep 29, 2013 at 12:15 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>> On Sun, Sep 29, 2013 at 11:49:41AM +0800, liu ping fan wrote:
> >>>> On Sun, Sep 29, 2013 at 3:56 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>> On Thu, Sep 12, 2013 at 11:25:15AM +0800, Liu Ping Fan wrote:
> >>>>>> 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 8429eb3..46903b9 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
> >>>>>
> >>>>> So users are expected to stick a bitmask of legal
> >>>>> pins here?
> >>>>> I think that's a bit too much rope to give to users.
> >>>>> Don't you think?
> >>>>>
> >>>> Sorry, not understand your meaning exactly.  But the scene will be:
> >>>> guest kernel polls the ability bitmask, and pick up one pin which is
> >>>> not occupied or can be shared with the level-trigger and low-active.
> >>>> So is it rope?
> >>>
> >>> I merely say that it's better to make this a bool or bit property.
> >>> UINT32 is too much flexibility imho.
> >>>
> >> The interrupt capability is export to guest by register
> >> Tn_INT_ROUTE_CAP[63:32]. So it is useless to make them as a bit
> >> property. Do you think so?
> >>
> >> Regards
> >> Pingfan
> > 
> > I think we merely need to support two modes:
> > - qemu 1.6 and older compatible
> > - compatible to actual hardware
> > 
> > Why would we let users configure an arbitrary
> > configuration which isn't compatible to either
> > old qemu or real hardware?
> 
> The actual setting depends on the chipset.  For example, the "real"
> PIIX4 is the same as the QEMU PIIX4, only Q35 uses the new value.
> 
> If in the future we had a chipset with more than 24 GSIs, you would have
> a third possibility.
> 
> Paolo

I was really only talking about q35 here.
I thought it's ugly that users can control intcap
directly. Can object_set_property be used after
qdev_try_create?

PIIX has another issue:
the default value in hpet is really Q35 specific,
that's also kind of ugly, isn't it?


-- 
MST

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

[Paolo Bonzini]

Il 30/09/2013 11:30, Michael S. Tsirkin ha scritto:
> I was really only talking about q35 here.
> I thought it's ugly that users can control intcap
> directly. Can object_set_property be used after
> qdev_try_create?

Yes, after that and before qdev_init.  This is how Ping Fan is doing
PIIX right now.

> PIIX has another issue:
> the default value in hpet is really Q35 specific,
> that's also kind of ugly, isn't it?

Yeah, perhaps it is.  So are you suggesting the default to be 4 (GSI2 only)?

Paolo

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

[Michael S. Tsirkin]

On Mon, Sep 30, 2013 at 05:48:03PM +0200, Paolo Bonzini wrote:
> Il 30/09/2013 11:30, Michael S. Tsirkin ha scritto:
> > I was really only talking about q35 here.
> > I thought it's ugly that users can control intcap
> > directly. Can object_set_property be used after
> > qdev_try_create?
> 
> Yes, after that and before qdev_init.  This is how Ping Fan is doing
> PIIX right now.
> 
> > PIIX has another issue:
> > the default value in hpet is really Q35 specific,
> > that's also kind of ugly, isn't it?
> 
> Yeah, perhaps it is.  So are you suggesting the default to be 4 (GSI2 only)?
> 
> Paolo

I suggest it fails unless caller set the property.

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

[liu ping fan]

On Mon, Sep 30, 2013 at 11:58 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, Sep 30, 2013 at 05:48:03PM +0200, Paolo Bonzini wrote:
>> Il 30/09/2013 11:30, Michael S. Tsirkin ha scritto:
>> > I was really only talking about q35 here.
>> > I thought it's ugly that users can control intcap
>> > directly. Can object_set_property be used after
>> > qdev_try_create?
>>
>> Yes, after that and before qdev_init.  This is how Ping Fan is doing
>> PIIX right now.
>>
>> > PIIX has another issue:
>> > the default value in hpet is really Q35 specific,
>> > that's also kind of ugly, isn't it?
>>
>> Yeah, perhaps it is.  So are you suggesting the default to be 4 (GSI2 only)?
>>
>> Paolo
>
> I suggest it fails unless caller set the property.
>
Sorry, out of office for a long time, and did not keep up with this
thread in time.
When letting the caller set the intcap, we should consider the
compatibility of q35. For pc-q35-1.7 or later, the caller should set
the property, otherwise not. But how can the caller tell that it runs
on q35-1.7?
The essential problem is that "set the property" will always overwrite
the property which is set up by compatible mechanism. So it is hard to
implement without breaking the current mechanism. Do you think so?

Thanks and regards,
Ping fan

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

[Michael S. Tsirkin]

On Wed, Oct 09, 2013 at 11:27:24AM +0800, liu ping fan wrote:
> On Mon, Sep 30, 2013 at 11:58 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Mon, Sep 30, 2013 at 05:48:03PM +0200, Paolo Bonzini wrote:
> >> Il 30/09/2013 11:30, Michael S. Tsirkin ha scritto:
> >> > I was really only talking about q35 here.
> >> > I thought it's ugly that users can control intcap
> >> > directly. Can object_set_property be used after
> >> > qdev_try_create?
> >>
> >> Yes, after that and before qdev_init.  This is how Ping Fan is doing
> >> PIIX right now.
> >>
> >> > PIIX has another issue:
> >> > the default value in hpet is really Q35 specific,
> >> > that's also kind of ugly, isn't it?
> >>
> >> Yeah, perhaps it is.  So are you suggesting the default to be 4 (GSI2 only)?
> >>
> >> Paolo
> >
> > I suggest it fails unless caller set the property.
> >
> Sorry, out of office for a long time, and did not keep up with this
> thread in time.
> When letting the caller set the intcap, we should consider the
> compatibility of q35. For pc-q35-1.7 or later, the caller should set
> the property, otherwise not.

Set it always - just set it to a compatible value for 1.6.

> But how can the caller tell that it runs
> on q35-1.7?
> The essential problem is that "set the property" will always overwrite
> the property which is set up by compatible mechanism. So it is hard to
> implement without breaking the current mechanism. Do you think so?
> 
> Thanks and regards,
> Ping fan

Not that hard.  Fail init if it wasn't set.

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

[Paolo Bonzini]

Il 09/10/2013 09:24, Michael S. Tsirkin ha scritto:
>> > Sorry, out of office for a long time, and did not keep up with this
>> > thread in time.
>> > When letting the caller set the intcap, we should consider the
>> > compatibility of q35. For pc-q35-1.7 or later, the caller should set
>> > the property, otherwise not.
> Set it always - just set it to a compatible value for 1.6.
> 

So you're suggesting skipping the mechanism we have for compatibility
properties, and instead using a global variable or something like that?

Paolo

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

[Michael S. Tsirkin]

On Wed, Oct 09, 2013 at 09:41:29AM +0200, Paolo Bonzini wrote:
> Il 09/10/2013 09:24, Michael S. Tsirkin ha scritto:
> >> > Sorry, out of office for a long time, and did not keep up with this
> >> > thread in time.
> >> > When letting the caller set the intcap, we should consider the
> >> > compatibility of q35. For pc-q35-1.7 or later, the caller should set
> >> > the property, otherwise not.
> > Set it always - just set it to a compatible value for 1.6.
> > 
> 
> So you're suggesting skipping the mechanism we have for compatibility
> properties, and instead using a global variable or something like that?
> 
> Paolo

That's one option.
Or we can use global properties - just set for 1.7 too.

All this is not ideal: properties really need to
integrate better with QOM, and using global
properties for compat always was a hack.


-- 
MST

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

[Paolo Bonzini]

Il 09/10/2013 10:01, Michael S. Tsirkin ha scritto:
> > So you're suggesting skipping the mechanism we have for compatibility
> > properties, and instead using a global variable or something like that?
> 
> That's one option.
> Or we can use global properties - just set for 1.7 too.

That's better.

Paolo

[Qemu-devel] [PATCH v5 3/5] 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 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0c313fe..f2b7b6c 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-piix with pc-q35
+             */
+            qdev_prop_set_uint32(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]);
             }
-- 
1.8.1.4

[Qemu-devel] [PATCH v5 4/5] 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. While
for pc-q35-*, we resort to compat property to fix it (a later patch).

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f2b7b6c..b8e7cee 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,13 @@ 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, "intcap", 0x4);
+            /* For pc-piix-*, hpet's intcap is always IRQ2. While for pc-q35-*,
+             * we resort to compat property to fix it.
+             */
+            if (!hpet_irqs) {
+                qdev_prop_set_uint32(hpet, "intcap", 0x4);
+            }
             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 907792b..8dcd0d6 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);
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index ca84e1c..9e41f4a 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);
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 9b2ddc4..9dd077f 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -134,7 +134,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,
-- 
1.8.1.4

[Qemu-devel] [PATCH v5 5/5] PC-1.6: add compatibility for hpet intcap on pc-q35-1.6

[Liu Ping Fan]

For guest bug compat, we limit hpet's interrupt compatibility on
ioapic's IRQ2 for pc-q35-1.6. As to pc-35-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      | 7 ++-----
 include/hw/i386/pc.h | 4 ++++
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 46903b9..7af495a 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -43,11 +43,8 @@
 
 #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
+/* using IRQ16~23, IRQ8 and IRQ2 */
+#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 9dd077f..79c63b3 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -231,6 +231,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 v5 0/5] bugs fix for hpet

[Paolo Bonzini]

Il 12/09/2013 05:25, Liu Ping Fan ha scritto:
> 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 (5):
>   hpet: inverse polarity when pin above ISA_NUM_IRQS
>   hpet: 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
>   PC-1.6: add compatibility for hpet intcap on pc-q35-1.6
> 
>  hw/i386/pc.c         | 17 ++++++++++++++---
>  hw/i386/pc_piix.c    |  3 ++-
>  hw/i386/pc_q35.c     |  2 +-
>  hw/timer/hpet.c      | 24 ++++++++++++++++++++----
>  include/hw/i386/pc.h |  7 ++++++-
>  5 files changed, 43 insertions(+), 10 deletions(-)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Nice series, patches are split well.  Thank you very much!

Paolo

Re: [Qemu-devel] [PATCH v5 0/5] bugs fix for hpet

[liu ping fan]

On Thu, Sep 12, 2013 at 2:29 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 12/09/2013 05:25, Liu Ping Fan ha scritto:
>> 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 (5):
>>   hpet: inverse polarity when pin above ISA_NUM_IRQS
>>   hpet: 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
>>   PC-1.6: add compatibility for hpet intcap on pc-q35-1.6
>>
>>  hw/i386/pc.c         | 17 ++++++++++++++---
>>  hw/i386/pc_piix.c    |  3 ++-
>>  hw/i386/pc_q35.c     |  2 +-
>>  hw/timer/hpet.c      | 24 ++++++++++++++++++++----
>>  include/hw/i386/pc.h |  7 ++++++-
>>  5 files changed, 43 insertions(+), 10 deletions(-)
>>
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Nice series, patches are split well.  Thank you very much!
>
Thanks for your guide :)  and learn much through it.

Regards,
Pingfan
> Paolo

Re: [Qemu-devel] [PATCH v5 0/5] bugs fix for hpet

[liu ping fan]

Hi, is hpet orphan? Or who can help me to merge this patch-set if my
patch is fine.

Thanks.

On Thu, Sep 12, 2013 at 11:25 AM, Liu Ping Fan <qemulist@gmail.com> wrote:
> 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 (5):
>   hpet: inverse polarity when pin above ISA_NUM_IRQS
>   hpet: 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
>   PC-1.6: add compatibility for hpet intcap on pc-q35-1.6
>
>  hw/i386/pc.c         | 17 ++++++++++++++---
>  hw/i386/pc_piix.c    |  3 ++-
>  hw/i386/pc_q35.c     |  2 +-
>  hw/timer/hpet.c      | 24 ++++++++++++++++++++----
>  include/hw/i386/pc.h |  7 ++++++-
>  5 files changed, 43 insertions(+), 10 deletions(-)
>
> --
> 1.8.1.4
>

Re: [Qemu-devel] [PATCH v5 0/5] bugs fix for hpet

[Paolo Bonzini]

Il 25/09/2013 08:27, liu ping fan ha scritto:
> Hi, is hpet orphan? Or who can help me to merge this patch-set if my
> patch is fine.

Anthony, Michael?

Paolo

> Thanks.
> 
> On Thu, Sep 12, 2013 at 11:25 AM, Liu Ping Fan <qemulist@gmail.com> wrote:
>> 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 (5):
>>   hpet: inverse polarity when pin above ISA_NUM_IRQS
>>   hpet: 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
>>   PC-1.6: add compatibility for hpet intcap on pc-q35-1.6
>>
>>  hw/i386/pc.c         | 17 ++++++++++++++---
>>  hw/i386/pc_piix.c    |  3 ++-
>>  hw/i386/pc_q35.c     |  2 +-
>>  hw/timer/hpet.c      | 24 ++++++++++++++++++++----
>>  include/hw/i386/pc.h |  7 ++++++-
>>  5 files changed, 43 insertions(+), 10 deletions(-)
>>
>> --
>> 1.8.1.4
>>

Re: [Qemu-devel] [PATCH v5 0/5] bugs fix for hpet

[Mike Day]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 25/09/2013 08:27, liu ping fan ha scritto:
>> Hi, is hpet orphan? Or who can help me to merge this patch-set if my
>> patch is fine.
>
> Anthony, Michael?

Yes, happy to help out with this. I'll start looking at it now and work
with Liu Ping, 

Mike

-- 

Mike Day | + 1 919 371-8786 | ncmike@ncultra.org
"Endurance is a Virtue"

Re: [Qemu-devel] [PATCH v5 0/5] bugs fix for hpet

[Mike Day]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 25/09/2013 08:27, liu ping fan ha scritto:
>> Hi, is hpet orphan? Or who can help me to merge this patch-set if my
>> patch is fine.
>
> Anthony, Michael?

Sorry, wrong Michael - 

Mike

-- 

Mike Day | + 1 919 371-8786 | ncmike@ncultra.org
"Endurance is a Virtue"