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

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

[Liu Ping Fan]

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 (5):
  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: add hpet compat to trace compatability version
  PC: differentiate hpet's interrupt capability on piix and q35

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

-- 
1.8.1.4

[Qemu-devel] [PATCH v6 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. (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 v6 2/5] 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 | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 8429eb3..5b11be4 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,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 +77,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 +668,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 +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("intcap", HPETState, intcap, HPET_TN_INT_CAP_DEFAULT),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
1.8.1.4

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

[Paolo Bonzini]

Il 10/10/2013 09:56, Liu Ping Fan ha scritto:
> 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 | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 8429eb3..5b11be4 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,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 +77,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 +668,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 +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("intcap", HPETState, intcap, HPET_TN_INT_CAP_DEFAULT),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> 

According to Michael's request, a zero intcap should be detected in
hpet_realize and give an error.

Paolo

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

[liu ping fan]

On Thu, Oct 10, 2013 at 5:11 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 10/10/2013 09:56, Liu Ping Fan ha scritto:
>> 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 | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
>> index 8429eb3..5b11be4 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,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 +77,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 +668,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 +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("intcap", HPETState, intcap, HPET_TN_INT_CAP_DEFAULT),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>
>>
>
> According to Michael's request, a zero intcap should be detected in
> hpet_realize and give an error.
>
Will fix.

Thanks and regards,
Ping Fan

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

[Michael S. Tsirkin]

On Thu, Oct 10, 2013 at 03:56:16PM +0800, Liu Ping Fan wrote:
> 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 | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 8429eb3..5b11be4 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,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 +77,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 +668,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 +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("intcap", HPETState, intcap, HPET_TN_INT_CAP_DEFAULT),
>      DEFINE_PROP_END_OF_LIST(),
>  };

Please add a macro for this name as you use it in other
files later.

>  
> -- 
> 1.8.1.4

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

[Paolo Bonzini]

Il 10/10/2013 11:16, Michael S. Tsirkin ha scritto:
> On Thu, Oct 10, 2013 at 03:56:16PM +0800, Liu Ping Fan wrote:
>> 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 | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
>> index 8429eb3..5b11be4 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,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 +77,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 +668,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 +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("intcap", HPETState, intcap, HPET_TN_INT_CAP_DEFAULT),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
> 
> Please add a macro for this name as you use it in other
> files later.

Are you sure?  This is not done for any other compat property.

Paolo

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

[Michael S. Tsirkin]

On Thu, Oct 10, 2013 at 11:33:07AM +0200, Paolo Bonzini wrote:
> Il 10/10/2013 11:16, Michael S. Tsirkin ha scritto:
> > On Thu, Oct 10, 2013 at 03:56:16PM +0800, Liu Ping Fan wrote:
> >> 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 | 10 ++++++++--
> >>  1 file changed, 8 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> >> index 8429eb3..5b11be4 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,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 +77,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 +668,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 +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("intcap", HPETState, intcap, HPET_TN_INT_CAP_DEFAULT),
> >>      DEFINE_PROP_END_OF_LIST(),
> >>  };
> > 
> > Please add a macro for this name as you use it in other
> > files later.
> 
> Are you sure?  This is not done for any other compat property.
> 
> Paolo

It's done if we use the property from C.
See PCI_HOST_PROP_PCI_HOLE64_SIZE.

You want compiler to catch errors, that's
much better than a runtime failure.

-- 
MST

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

[Paolo Bonzini]

Il 10/10/2013 11:41, Michael S. Tsirkin ha scritto:
>> > Are you sure?  This is not done for any other compat property.
>> > 
>> > Paolo
> It's done if we use the property from C.
> See PCI_HOST_PROP_PCI_HOLE64_SIZE.
> 
> You want compiler to catch errors, that's
> much better than a runtime failure.

I agree, but I think there should be no need to use the property from C.

Paolo

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

[Michael S. Tsirkin]

On Thu, Oct 10, 2013 at 11:46:42AM +0200, Paolo Bonzini wrote:
> Il 10/10/2013 11:41, Michael S. Tsirkin ha scritto:
> >> > Are you sure?  This is not done for any other compat property.
> >> > 
> >> > Paolo
> > It's done if we use the property from C.
> > See PCI_HOST_PROP_PCI_HOLE64_SIZE.
> > 
> > You want compiler to catch errors, that's
> > much better than a runtime failure.
> 
> I agree, but I think there should be no need to use the property from C.
> 
> Paolo

Well this patchset does use it from C.
If it's done it needs a macro.

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

[liu ping fan]

On Thu, Oct 10, 2013 at 7:41 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Oct 10, 2013 at 11:46:42AM +0200, Paolo Bonzini wrote:
>> Il 10/10/2013 11:41, Michael S. Tsirkin ha scritto:
>> >> > Are you sure?  This is not done for any other compat property.
>> >> >
>> >> > Paolo
>> > It's done if we use the property from C.
>> > See PCI_HOST_PROP_PCI_HOLE64_SIZE.
>> >
>> > You want compiler to catch errors, that's
>> > much better than a runtime failure.
>>
>> I agree, but I think there should be no need to use the property from C.
>>
>> Paolo
>
> Well this patchset does use it from C.
> If it's done it needs a macro.

hpet.h is the ideal place to put the macro, so pc.c can see it. But
what about PC_COMPAT_1_7 in pc.h? I think it is not right to include
hpet.h. So can I do not use marco in pc.h?

Thanks and regards,
Ping Fan

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

[Paolo Bonzini]

Il 11/10/2013 04:59, liu ping fan ha scritto:
> On Thu, Oct 10, 2013 at 7:41 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Thu, Oct 10, 2013 at 11:46:42AM +0200, Paolo Bonzini wrote:
>>> Il 10/10/2013 11:41, Michael S. Tsirkin ha scritto:
>>>>>> Are you sure?  This is not done for any other compat property.
>>>>>>
>>>>>> Paolo
>>>> It's done if we use the property from C.
>>>> See PCI_HOST_PROP_PCI_HOLE64_SIZE.
>>>>
>>>> You want compiler to catch errors, that's
>>>> much better than a runtime failure.
>>>
>>> I agree, but I think there should be no need to use the property from C.
>>>
>>> Paolo
>>
>> Well this patchset does use it from C.
>> If it's done it needs a macro.
> 
> hpet.h is the ideal place to put the macro, so pc.c can see it. But
> what about PC_COMPAT_1_7 in pc.h? I think it is not right to include
> hpet.h. So can I do not use marco in pc.h?

I think you shouldn't need the macro (no need to use the property from
C, only from compat properties).

Paolo

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

[liu ping fan]

On Fri, Oct 11, 2013 at 4:38 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 11/10/2013 04:59, liu ping fan ha scritto:
>> On Thu, Oct 10, 2013 at 7:41 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> On Thu, Oct 10, 2013 at 11:46:42AM +0200, Paolo Bonzini wrote:
>>>> Il 10/10/2013 11:41, Michael S. Tsirkin ha scritto:
>>>>>>> Are you sure?  This is not done for any other compat property.
>>>>>>>
>>>>>>> Paolo
>>>>> It's done if we use the property from C.
>>>>> See PCI_HOST_PROP_PCI_HOLE64_SIZE.
>>>>>
>>>>> You want compiler to catch errors, that's
>>>>> much better than a runtime failure.
>>>>
>>>> I agree, but I think there should be no need to use the property from C.
>>>>
>>>> Paolo
>>>
>>> Well this patchset does use it from C.
>>> If it's done it needs a macro.
>>
>> hpet.h is the ideal place to put the macro, so pc.c can see it. But
>> what about PC_COMPAT_1_7 in pc.h? I think it is not right to include
>> hpet.h. So can I do not use marco in pc.h?
>
> I think you shouldn't need the macro (no need to use the property from
> C, only from compat properties).
>
We need to tell the compat and then decide to set "intcap" in
pc_basic_device_init()

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

Regards,
Ping Fan

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

[Paolo Bonzini]

Il 11/10/2013 11:18, liu ping fan ha scritto:
> On Fri, Oct 11, 2013 at 4:38 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 11/10/2013 04:59, liu ping fan ha scritto:
>>> On Thu, Oct 10, 2013 at 7:41 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> On Thu, Oct 10, 2013 at 11:46:42AM +0200, Paolo Bonzini wrote:
>>>>> Il 10/10/2013 11:41, Michael S. Tsirkin ha scritto:
>>>>>>>> Are you sure?  This is not done for any other compat property.
>>>>>>>>
>>>>>>>> Paolo
>>>>>> It's done if we use the property from C.
>>>>>> See PCI_HOST_PROP_PCI_HOLE64_SIZE.
>>>>>>
>>>>>> You want compiler to catch errors, that's
>>>>>> much better than a runtime failure.
>>>>>
>>>>> I agree, but I think there should be no need to use the property from C.
>>>>>
>>>>> Paolo
>>>>
>>>> Well this patchset does use it from C.
>>>> If it's done it needs a macro.
>>>
>>> hpet.h is the ideal place to put the macro, so pc.c can see it. But
>>> what about PC_COMPAT_1_7 in pc.h? I think it is not right to include
>>> hpet.h. So can I do not use marco in pc.h?
>>
>> I think you shouldn't need the macro (no need to use the property from
>> C, only from compat properties).
>>
> We need to tell the compat and then decide to set "intcap" in
> pc_basic_device_init()
> 
>                 uint8_t compat = object_property_get_int(OBJECT(hpet),
>                         "intcap", NULL);
>                 if (!compat) {
>                     qdev_prop_set_uint32(hpet, "intcap", 0xff0104);
>                 }

No, that can be done via global properties as well.

Paolo

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

[Michael S. Tsirkin]

On Fri, Oct 11, 2013 at 05:18:30PM +0800, liu ping fan wrote:
> On Fri, Oct 11, 2013 at 4:38 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > Il 11/10/2013 04:59, liu ping fan ha scritto:
> >> On Thu, Oct 10, 2013 at 7:41 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>> On Thu, Oct 10, 2013 at 11:46:42AM +0200, Paolo Bonzini wrote:
> >>>> Il 10/10/2013 11:41, Michael S. Tsirkin ha scritto:
> >>>>>>> Are you sure?  This is not done for any other compat property.
> >>>>>>>
> >>>>>>> Paolo
> >>>>> It's done if we use the property from C.
> >>>>> See PCI_HOST_PROP_PCI_HOLE64_SIZE.
> >>>>>
> >>>>> You want compiler to catch errors, that's
> >>>>> much better than a runtime failure.
> >>>>
> >>>> I agree, but I think there should be no need to use the property from C.
> >>>>
> >>>> Paolo
> >>>
> >>> Well this patchset does use it from C.
> >>> If it's done it needs a macro.
> >>
> >> hpet.h is the ideal place to put the macro, so pc.c can see it. But
> >> what about PC_COMPAT_1_7 in pc.h? I think it is not right to include
> >> hpet.h. So can I do not use marco in pc.h?
> >
> > I think you shouldn't need the macro (no need to use the property from
> > C, only from compat properties).
> >
> We need to tell the compat and then decide to set "intcap" in
> pc_basic_device_init()
> 
>                 uint8_t compat = object_property_get_int(OBJECT(hpet),
>                         "intcap", NULL);
>                 if (!compat) {
>                     qdev_prop_set_uint32(hpet, "intcap", 0xff0104);
>                 }
> 
> Regards,
> Ping Fan

So if you use it from C, please use a macro.
If not, no need to.

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

[Michael S. Tsirkin]

On Fri, Oct 11, 2013 at 10:59:40AM +0800, liu ping fan wrote:
> On Thu, Oct 10, 2013 at 7:41 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Thu, Oct 10, 2013 at 11:46:42AM +0200, Paolo Bonzini wrote:
> >> Il 10/10/2013 11:41, Michael S. Tsirkin ha scritto:
> >> >> > Are you sure?  This is not done for any other compat property.
> >> >> >
> >> >> > Paolo
> >> > It's done if we use the property from C.
> >> > See PCI_HOST_PROP_PCI_HOLE64_SIZE.
> >> >
> >> > You want compiler to catch errors, that's
> >> > much better than a runtime failure.
> >>
> >> I agree, but I think there should be no need to use the property from C.
> >>
> >> Paolo
> >
> > Well this patchset does use it from C.
> > If it's done it needs a macro.
> 
> hpet.h is the ideal place to put the macro, so pc.c can see it. But
> what about PC_COMPAT_1_7 in pc.h? I think it is not right to include
> hpet.h.

Why not?

> So can I do not use marco in pc.h?
> 
> Thanks and regards,
> Ping Fan

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

[liu ping fan]

On Mon, Oct 14, 2013 at 10:18 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Oct 11, 2013 at 10:59:40AM +0800, liu ping fan wrote:
>> On Thu, Oct 10, 2013 at 7:41 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Thu, Oct 10, 2013 at 11:46:42AM +0200, Paolo Bonzini wrote:
>> >> Il 10/10/2013 11:41, Michael S. Tsirkin ha scritto:
>> >> >> > Are you sure?  This is not done for any other compat property.
>> >> >> >
>> >> >> > Paolo
>> >> > It's done if we use the property from C.
>> >> > See PCI_HOST_PROP_PCI_HOLE64_SIZE.
>> >> >
>> >> > You want compiler to catch errors, that's
>> >> > much better than a runtime failure.
>> >>
>> >> I agree, but I think there should be no need to use the property from C.
>> >>
>> >> Paolo
>> >
>> > Well this patchset does use it from C.
>> > If it's done it needs a macro.
>>
>> hpet.h is the ideal place to put the macro, so pc.c can see it. But
>> what about PC_COMPAT_1_7 in pc.h? I think it is not right to include
>> hpet.h.
>
> Why not?
>
Since pc.h is included by so many hpet unrelated drivers, if pc.h
include hpet.h, then we will export the internal of hpet struct.

Regards,
Ping Fan

[Qemu-devel] [PATCH v6 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 +++++++++--
 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..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-iix 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]);
             }
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 5b11be4..69ce587 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -43,8 +43,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)
@@ -758,7 +756,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_UINT32("intcap", HPETState, intcap, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v6 3/5] PC: use qdev_xx to create hpet instead of sysbus_create_xx

[Paolo Bonzini]

Il 10/10/2013 09:56, Liu Ping Fan ha scritto:
> 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..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-iix 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]);
>              }
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 5b11be4..69ce587 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -43,8 +43,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)
> @@ -758,7 +756,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_UINT32("intcap", HPETState, intcap, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> 

This should not be needed anymore, except for changing the intcap
default to 0 (which would go in patch 5).

Paolo

Re: [Qemu-devel] [PATCH v6 3/5] PC: use qdev_xx to create hpet instead of sysbus_create_xx

[Michael S. Tsirkin]

On Thu, Oct 10, 2013 at 03:56:17PM +0800, Liu Ping Fan wrote:
> 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>


I would merge patches 3,4,5 together.
It's generally not a good idea to change
same line of code in multiple patches in a patchset,
makes review harder instead of easier.

> ---
>  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..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-iix 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]);
>              }
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 5b11be4..69ce587 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -43,8 +43,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)
> @@ -758,7 +756,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_UINT32("intcap", HPETState, intcap, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> -- 
> 1.8.1.4

[Qemu-devel] [PATCH v6 4/5] PC: add hpet compat to trace compatability version

[Liu Ping Fan]

For guest bug compat, we need to limit hpet's intcap on IRQ2
for pc-q35-1.7 and earlier. We use hpet's compat property to
indicate the PC version.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index c6042c7..90f1ea4 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -346,6 +346,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..569f946 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -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/hw/timer/hpet.c b/hw/timer/hpet.c
index 69ce587..3cbe71e 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -76,6 +76,7 @@ typedef struct HPETState {
     qemu_irq pit_enabled;
     uint8_t num_timers;
     uint32_t intcap;
+    uint8_t compat;
     HPETTimer timer[HPET_MAX_TIMERS];
 
     /* Memory-mapped, software visible registers */
@@ -757,6 +758,7 @@ 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, 0),
+    DEFINE_PROP_UINT8("compat", HPETState, compat, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 9b2ddc4..80aa7bd 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -225,7 +225,15 @@ void pvpanic_init(ISABus *bus);
 
 int e820_add_entry(uint64_t, uint64_t, uint32_t);
 
+#define PC_COMPAT_1_7 \
+        {\
+            .driver   = "hpet",\
+            .property = "compat",\
+            .value    = stringify(1),\
+        }
+
 #define PC_COMPAT_1_6 \
+        PC_COMPAT_1_7, \
         {\
             .driver   = "e1000",\
             .property = "mitigation",\

Re: [Qemu-devel] [PATCH v6 4/5] PC: add hpet compat to trace compatability version

[Paolo Bonzini]

Il 10/10/2013 09:56, Liu Ping Fan ha scritto:
> For guest bug compat, we need to limit hpet's intcap on IRQ2
> for pc-q35-1.7 and earlier. We use hpet's compat property to
> indicate the PC version.
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index c6042c7..90f1ea4 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -346,6 +346,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..569f946 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -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/hw/timer/hpet.c b/hw/timer/hpet.c
> index 69ce587..3cbe71e 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -76,6 +76,7 @@ typedef struct HPETState {
>      qemu_irq pit_enabled;
>      uint8_t num_timers;
>      uint32_t intcap;
> +    uint8_t compat;
>      HPETTimer timer[HPET_MAX_TIMERS];
>  
>      /* Memory-mapped, software visible registers */
> @@ -757,6 +758,7 @@ 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, 0),
> +    DEFINE_PROP_UINT8("compat", HPETState, compat, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 9b2ddc4..80aa7bd 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -225,7 +225,15 @@ void pvpanic_init(ISABus *bus);
>  
>  int e820_add_entry(uint64_t, uint64_t, uint32_t);
>  
> +#define PC_COMPAT_1_7 \
> +        {\
> +            .driver   = "hpet",\
> +            .property = "compat",\
> +            .value    = stringify(1),\
> +        }
> +
>  #define PC_COMPAT_1_6 \
> +        PC_COMPAT_1_7, \
>          {\
>              .driver   = "e1000",\
>              .property = "mitigation",\
> 

You can set the intcap property directly instead of adding this indirection.

Paolo

[Qemu-devel] [PATCH v6 5/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.
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         | 21 ++++++++++++++++-----
 hw/i386/pc_piix.c    |  3 ++-
 hw/i386/pc_q35.c     |  2 +-
 include/hw/i386/pc.h |  3 ++-
 4 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f2b7b6c..062019d 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,20 @@ 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. */
+            if (!hpet_irqs) {
+                qdev_prop_set_uint32(hpet, "intcap", 0x4);
+            } else {
+                /* For pc-q35-1.7 and earlier, use IRQ2 for compat. */
+                uint8_t compat = object_property_get_int(OBJECT(hpet),
+                        "compat", NULL);
+                if (compat) {
+                    qdev_prop_set_uint32(hpet, "intcap", 0x4);
+                } else {
+                    /* using IRQ16~23, IRQ8 and IRQ2 */
+                    qdev_prop_set_uint32(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 90f1ea4..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);
 
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 80aa7bd..a49d9cd 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

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

[Michael S. Tsirkin]

On Thu, Oct 10, 2013 at 03:56:19PM +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         | 21 ++++++++++++++++-----
>  hw/i386/pc_piix.c    |  3 ++-
>  hw/i386/pc_q35.c     |  2 +-
>  include/hw/i386/pc.h |  3 ++-
>  4 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f2b7b6c..062019d 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,20 @@ 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. */
> +            if (!hpet_irqs) {
> +                qdev_prop_set_uint32(hpet, "intcap", 0x4);
> +            } else {
> +                /* For pc-q35-1.7 and earlier, use IRQ2 for compat. */
> +                uint8_t compat = object_property_get_int(OBJECT(hpet),
> +                        "compat", NULL);
> +                if (compat) {
> +                    qdev_prop_set_uint32(hpet, "intcap", 0x4);
> +                } else {
> +                    /* using IRQ16~23, IRQ8 and IRQ2 */
> +                    qdev_prop_set_uint32(hpet, "intcap", 0xff0104);
> +                }
> +            }

So why do we need an extra property?

	                uint8_t compat = object_property_get_int(OBJECT(hpet),
	                        "intcap", NULL);
	                if (!intcap) {
			    /* For pc-piix-*, hpet's intcap is IRQ2. */
	                    /* For Q35, using IRQ16~23, IRQ8 and IRQ2 */
				uint32_t intcap = hpet_irqs ?  0xff0104 : 0x4;
	                        qdev_prop_set_uint32(hpet, "intcap", intcap);
	                }

now all you need to do for compat is set intcap property.


>              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 90f1ea4..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);
>  
> 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 80aa7bd..a49d9cd 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