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

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

[Liu Ping Fan]

v3:
  change hpet interrupt capablity on board's demand

Liu Ping Fan (3):
  hpet: inverse polarity when pin above ISA_NUM_IRQS
  qdev: interface for SysBusDevice to change property on requirement
  hpet: entitle more irq pins for hpet

 hw/core/sysbus.c    |  5 ++++-
 hw/i386/pc.c        |  8 +++++++-
 hw/timer/hpet.c     | 26 ++++++++++++++++++++++----
 include/hw/sysbus.h |  8 +++++---
 4 files changed, 38 insertions(+), 9 deletions(-)

-- 
1.8.1.4

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

[Liu Ping Fan]

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

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

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

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

[Qemu-devel] [PATCH v3 2/3] qdev: interface for SysBusDevice to change property on requirement

[Liu Ping Fan]

qdev's property can not be set after realized, but there is a
requirement of adjusting device's behavior on different mother
boards.  So introducing a callback in sysbus_try_create_simple()
to adjust device's property on board's demand.

(This patch is needed by the later one which changes hpet's intcap
property)

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/core/sysbus.c    | 5 ++++-
 hw/i386/pc.c        | 2 +-
 include/hw/sysbus.h | 8 +++++---
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 9004d8c..e894bbb 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -172,7 +172,7 @@ DeviceState *sysbus_create_varargs(const char *name,
     return dev;
 }
 
-DeviceState *sysbus_try_create_varargs(const char *name,
+DeviceState *sysbus_try_create_varargs(const char *name, CompatSet set,
                                        hwaddr addr, ...)
 {
     DeviceState *dev;
@@ -185,6 +185,9 @@ DeviceState *sysbus_try_create_varargs(const char *name,
     if (!dev) {
         return NULL;
     }
+    if (set) {
+        set(dev);
+    }
     s = SYS_BUS_DEVICE(dev);
     qdev_init_nofail(dev);
     if (addr != (hwaddr)-1) {
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e8bc8ce..09c10ac 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1247,7 +1247,7 @@ 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);
+        hpet = sysbus_try_create_simple("hpet", NULL, HPET_BASE, NULL);
 
         if (hpet) {
             for (i = 0; i < GSI_NUM_PINS; i++) {
diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index bb50a87..47337f2 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -58,6 +58,8 @@ struct SysBusDevice {
     pio_addr_t pio[QDEV_MAX_PIO];
 };
 
+typedef void (*CompatSet)(DeviceState *dev);
+
 void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory);
 MemoryRegion *sysbus_mmio_get_region(SysBusDevice *dev, int n);
 void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p);
@@ -77,7 +79,7 @@ MemoryRegion *sysbus_address_space(SysBusDevice *dev);
 /* Legacy helper function for creating devices.  */
 DeviceState *sysbus_create_varargs(const char *name,
                                  hwaddr addr, ...);
-DeviceState *sysbus_try_create_varargs(const char *name,
+DeviceState *sysbus_try_create_varargs(const char *name, CompatSet set,
                                        hwaddr addr, ...);
 static inline DeviceState *sysbus_create_simple(const char *name,
                                               hwaddr addr,
@@ -86,11 +88,11 @@ static inline DeviceState *sysbus_create_simple(const char *name,
     return sysbus_create_varargs(name, addr, irq, NULL);
 }
 
-static inline DeviceState *sysbus_try_create_simple(const char *name,
+static inline DeviceState *sysbus_try_create_simple(const char *name, CompatSet set,
                                                     hwaddr addr,
                                                     qemu_irq irq)
 {
-    return sysbus_try_create_varargs(name, addr, irq, NULL);
+    return sysbus_try_create_varargs(name, set, addr, irq, NULL);
 }
 
 #endif /* !HW_SYSBUS_H */
-- 
1.8.1.4

[Qemu-devel] [PATCH v3 3/3] hpet: entitle more irq pins for hpet

[Liu Ping Fan]

On q35 machine, IRQ2/8 can be reserved for hpet timer 0/1. And pin 16~23
of ioapic can be dynamically assigned to hpet as guest chooses.

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 09c10ac..bb23d99 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1217,6 +1217,12 @@ static const MemoryRegionOps ioportF0_io_ops = {
     },
 };
 
+static void hpet_intcap_set(DeviceState *dev)
+{
+    /* For guest bug compatibility, only IRQ2 is reserved for hpet on q35 */
+    qdev_prop_set_uint32(dev, "intcap", 0x4);
+}
+
 void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
                           ISADevice **rtc_state,
                           ISADevice **floppy,
@@ -1247,7 +1253,7 @@ 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", NULL, HPET_BASE, NULL);
+        hpet = sysbus_try_create_simple("hpet", hpet_intcap_set, HPET_BASE, NULL);
 
         if (hpet) {
             for (i = 0; i < GSI_NUM_PINS; i++) {
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 1139448..2e19ff5 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,11 @@
 
 #define HPET_MSI_SUPPORT        0
 
+/* only IRQ2 allowed for pc-1.6 and former */
+#define HPET_TN_INT_CAP_PC (0x4ULL << 32)
+/* Hpet can use non-legacy IRQ16~23, and an IRQ2 ,IRQ8 */
+#define HPET_TN_INT_CAP_DEFAULT 0xff0104ULL
+
 #define TYPE_HPET "hpet"
 #define HPET(obj) OBJECT_CHECK(HPETState, (obj), TYPE_HPET)
 
@@ -73,6 +79,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 +670,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 +760,7 @@ static void hpet_realize(DeviceState *dev, Error **errp)
 static Property hpet_device_properties[] = {
     DEFINE_PROP_UINT8("timers", HPETState, num_timers, HPET_MIN_TIMERS),
     DEFINE_PROP_BIT("msi", HPETState, flags, HPET_MSI_SUPPORT, false),
+    DEFINE_PROP_UINT32("intcap", HPETState, intcap, HPET_TN_INT_CAP_DEFAULT),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v3 2/3] qdev: interface for SysBusDevice to change property on requirement

[Paolo Bonzini]

Il 30/08/2013 09:53, Liu Ping Fan ha scritto:
> qdev's property can not be set after realized, but there is a
> requirement of adjusting device's behavior on different mother
> boards.  So introducing a callback in sysbus_try_create_simple()
> to adjust device's property on board's demand.
> 
> (This patch is needed by the later one which changes hpet's intcap
> property)

I don't think it is useful to add a new mechanism since there is an
existing mechanism to set properties for compatibility (which I pointed
you to earlier).  It is also incorrect because this will have an effect
on all PC boards including pc-q35-1.7 and newer.

You need to create a 1.7 machine like commit 45053fd (pc: Create
pc-*-1.6 machine-types, 2013-05-27).  On top of this:

* the 1.6 machines need to have a compatibility property in hw/i386/pc.h.

* the pc-i440fx-1.7 machine needs to have a compatibility property for
the same thing in hw/i386/pc_piix.c

Paolo

> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/core/sysbus.c    | 5 ++++-
>  hw/i386/pc.c        | 2 +-
>  include/hw/sysbus.h | 8 +++++---
>  3 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index 9004d8c..e894bbb 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -172,7 +172,7 @@ DeviceState *sysbus_create_varargs(const char *name,
>      return dev;
>  }
>  
> -DeviceState *sysbus_try_create_varargs(const char *name,
> +DeviceState *sysbus_try_create_varargs(const char *name, CompatSet set,
>                                         hwaddr addr, ...)
>  {
>      DeviceState *dev;
> @@ -185,6 +185,9 @@ DeviceState *sysbus_try_create_varargs(const char *name,
>      if (!dev) {
>          return NULL;
>      }
> +    if (set) {
> +        set(dev);
> +    }
>      s = SYS_BUS_DEVICE(dev);
>      qdev_init_nofail(dev);
>      if (addr != (hwaddr)-1) {
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index e8bc8ce..09c10ac 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1247,7 +1247,7 @@ 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);
> +        hpet = sysbus_try_create_simple("hpet", NULL, HPET_BASE, NULL);
>  
>          if (hpet) {
>              for (i = 0; i < GSI_NUM_PINS; i++) {
> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
> index bb50a87..47337f2 100644
> --- a/include/hw/sysbus.h
> +++ b/include/hw/sysbus.h
> @@ -58,6 +58,8 @@ struct SysBusDevice {
>      pio_addr_t pio[QDEV_MAX_PIO];
>  };
>  
> +typedef void (*CompatSet)(DeviceState *dev);
> +
>  void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory);
>  MemoryRegion *sysbus_mmio_get_region(SysBusDevice *dev, int n);
>  void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p);
> @@ -77,7 +79,7 @@ MemoryRegion *sysbus_address_space(SysBusDevice *dev);
>  /* Legacy helper function for creating devices.  */
>  DeviceState *sysbus_create_varargs(const char *name,
>                                   hwaddr addr, ...);
> -DeviceState *sysbus_try_create_varargs(const char *name,
> +DeviceState *sysbus_try_create_varargs(const char *name, CompatSet set,
>                                         hwaddr addr, ...);
>  static inline DeviceState *sysbus_create_simple(const char *name,
>                                                hwaddr addr,
> @@ -86,11 +88,11 @@ static inline DeviceState *sysbus_create_simple(const char *name,
>      return sysbus_create_varargs(name, addr, irq, NULL);
>  }
>  
> -static inline DeviceState *sysbus_try_create_simple(const char *name,
> +static inline DeviceState *sysbus_try_create_simple(const char *name, CompatSet set,
>                                                      hwaddr addr,
>                                                      qemu_irq irq)
>  {
> -    return sysbus_try_create_varargs(name, addr, irq, NULL);
> +    return sysbus_try_create_varargs(name, set, addr, irq, NULL);
>  }
>  
>  #endif /* !HW_SYSBUS_H */
> 

Re: [Qemu-devel] [PATCH v3 2/3] qdev: interface for SysBusDevice to change property on requirement

[Peter Maydell]

On 30 August 2013 08:53, Liu Ping Fan <qemulist@gmail.com> wrote:
> qdev's property can not be set after realized, but there is a
> requirement of adjusting device's behavior on different mother
> boards.  So introducing a callback in sysbus_try_create_simple()
> to adjust device's property on board's demand.

The sysbus_create functions are just convenience wrappers
for "create; init; map MMIO; connect IRQs". If you need to
set properties or otherwise do something between create
and init then just don't use the convenience wrapper.

-- PMM

Re: [Qemu-devel] [PATCH v3 3/3] hpet: entitle more irq pins for hpet

[Andreas Färber]

Am 30.08.2013 09:53, schrieb Liu Ping Fan:
> On q35 machine, IRQ2/8 can be reserved for hpet timer 0/1. And pin 16~23
> of ioapic can be dynamically assigned to hpet as guest chooses.
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/i386/pc.c    |  8 +++++++-
>  hw/timer/hpet.c | 12 ++++++++++--
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 09c10ac..bb23d99 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1217,6 +1217,12 @@ static const MemoryRegionOps ioportF0_io_ops = {
>      },
>  };
>  
> +static void hpet_intcap_set(DeviceState *dev)
> +{
> +    /* For guest bug compatibility, only IRQ2 is reserved for hpet on q35 */
> +    qdev_prop_set_uint32(dev, "intcap", 0x4);
> +}
> +
>  void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>                            ISADevice **rtc_state,
>                            ISADevice **floppy,
> @@ -1247,7 +1253,7 @@ 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", NULL, HPET_BASE, NULL);
> +        hpet = sysbus_try_create_simple("hpet", hpet_intcap_set, HPET_BASE, NULL);
>  
>          if (hpet) {
>              for (i = 0; i < GSI_NUM_PINS; i++) {

As PMM has said, this is unnecessary. Just use qdev_create(),
qdev_set_prop_*(), qdev_init_nofail(), sysbus_mmio_map(). (This code
seems not much QOM'ified yet.)

> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 1139448..2e19ff5 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,11 @@
>  
>  #define HPET_MSI_SUPPORT        0
>  
> +/* only IRQ2 allowed for pc-1.6 and former */
> +#define HPET_TN_INT_CAP_PC (0x4ULL << 32)
> +/* Hpet can use non-legacy IRQ16~23, and an IRQ2 ,IRQ8 */
> +#define HPET_TN_INT_CAP_DEFAULT 0xff0104ULL
> +
>  #define TYPE_HPET "hpet"
>  #define HPET(obj) OBJECT_CHECK(HPETState, (obj), TYPE_HPET)
>  
> @@ -73,6 +79,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 +670,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 +760,7 @@ static void hpet_realize(DeviceState *dev, Error **errp)
>  static Property hpet_device_properties[] = {
>      DEFINE_PROP_UINT8("timers", HPETState, num_timers, HPET_MIN_TIMERS),
>      DEFINE_PROP_BIT("msi", HPETState, flags, HPET_MSI_SUPPORT, false),
> +    DEFINE_PROP_UINT32("intcap", HPETState, intcap, HPET_TN_INT_CAP_DEFAULT),

What is "intcap"? It sounds like capabilities? In that case
DEFINE_PROP_BIT() might be a more appropriate way to model individually
tweakable properties? Either way, the property name could probably use
some love for clarity - there is no explanation for users.

Regards,
Andreas

>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v3 2/3] qdev: interface for SysBusDevice to change property on requirement

[Andreas Färber]

Am 30.08.2013 10:17, schrieb Paolo Bonzini:
> Il 30/08/2013 09:53, Liu Ping Fan ha scritto:
>> qdev's property can not be set after realized, but there is a
>> requirement of adjusting device's behavior on different mother
>> boards.  So introducing a callback in sysbus_try_create_simple()
>> to adjust device's property on board's demand.
>>
>> (This patch is needed by the later one which changes hpet's intcap
>> property)
> 
> I don't think it is useful to add a new mechanism since there is an
> existing mechanism to set properties for compatibility (which I pointed
> you to earlier).  It is also incorrect because this will have an effect
> on all PC boards including pc-q35-1.7 and newer.
> 
> You need to create a 1.7 machine like commit 45053fd (pc: Create
> pc-*-1.6 machine-types, 2013-05-27).

Stefan already has than in his net tree and just needs to (rebase and)
flush his queue!

Andreas

>  On top of this:
> 
> * the 1.6 machines need to have a compatibility property in hw/i386/pc.h.
> 
> * the pc-i440fx-1.7 machine needs to have a compatibility property for
> the same thing in hw/i386/pc_piix.c
> 
> Paolo
> 
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  hw/core/sysbus.c    | 5 ++++-
>>  hw/i386/pc.c        | 2 +-
>>  include/hw/sysbus.h | 8 +++++---
>>  3 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
>> index 9004d8c..e894bbb 100644
>> --- a/hw/core/sysbus.c
>> +++ b/hw/core/sysbus.c
>> @@ -172,7 +172,7 @@ DeviceState *sysbus_create_varargs(const char *name,
>>      return dev;
>>  }
>>  
>> -DeviceState *sysbus_try_create_varargs(const char *name,
>> +DeviceState *sysbus_try_create_varargs(const char *name, CompatSet set,
>>                                         hwaddr addr, ...)
>>  {
>>      DeviceState *dev;
>> @@ -185,6 +185,9 @@ DeviceState *sysbus_try_create_varargs(const char *name,
>>      if (!dev) {
>>          return NULL;
>>      }
>> +    if (set) {
>> +        set(dev);
>> +    }
>>      s = SYS_BUS_DEVICE(dev);
>>      qdev_init_nofail(dev);
>>      if (addr != (hwaddr)-1) {
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index e8bc8ce..09c10ac 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1247,7 +1247,7 @@ 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);
>> +        hpet = sysbus_try_create_simple("hpet", NULL, HPET_BASE, NULL);
>>  
>>          if (hpet) {
>>              for (i = 0; i < GSI_NUM_PINS; i++) {
>> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
>> index bb50a87..47337f2 100644
>> --- a/include/hw/sysbus.h
>> +++ b/include/hw/sysbus.h
>> @@ -58,6 +58,8 @@ struct SysBusDevice {
>>      pio_addr_t pio[QDEV_MAX_PIO];
>>  };
>>  
>> +typedef void (*CompatSet)(DeviceState *dev);
>> +
>>  void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory);
>>  MemoryRegion *sysbus_mmio_get_region(SysBusDevice *dev, int n);
>>  void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p);
>> @@ -77,7 +79,7 @@ MemoryRegion *sysbus_address_space(SysBusDevice *dev);
>>  /* Legacy helper function for creating devices.  */
>>  DeviceState *sysbus_create_varargs(const char *name,
>>                                   hwaddr addr, ...);
>> -DeviceState *sysbus_try_create_varargs(const char *name,
>> +DeviceState *sysbus_try_create_varargs(const char *name, CompatSet set,
>>                                         hwaddr addr, ...);
>>  static inline DeviceState *sysbus_create_simple(const char *name,
>>                                                hwaddr addr,
>> @@ -86,11 +88,11 @@ static inline DeviceState *sysbus_create_simple(const char *name,
>>      return sysbus_create_varargs(name, addr, irq, NULL);
>>  }
>>  
>> -static inline DeviceState *sysbus_try_create_simple(const char *name,
>> +static inline DeviceState *sysbus_try_create_simple(const char *name, CompatSet set,
>>                                                      hwaddr addr,
>>                                                      qemu_irq irq)
>>  {
>> -    return sysbus_try_create_varargs(name, addr, irq, NULL);
>> +    return sysbus_try_create_varargs(name, set, addr, irq, NULL);
>>  }
>>  
>>  #endif /* !HW_SYSBUS_H */
>>
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v3 3/3] hpet: entitle more irq pins for hpet

[Paolo Bonzini]

Il 30/08/2013 14:31, Andreas Färber ha scritto:
>> >  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),
> What is "intcap"? It sounds like capabilities? In that case
> DEFINE_PROP_BIT() might be a more appropriate way to model individually
> tweakable properties? Either way, the property name could probably use
> some love for clarity - there is no explanation for users.

Unless you want 24 properties (one per pin), an UINT32 is fine. :)

Paolo

Re: [Qemu-devel] [PATCH v3 2/3] qdev: interface for SysBusDevice to change property on requirement

[liu ping fan]

On Fri, Aug 30, 2013 at 4:17 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 30/08/2013 09:53, Liu Ping Fan ha scritto:
>> qdev's property can not be set after realized, but there is a
>> requirement of adjusting device's behavior on different mother
>> boards.  So introducing a callback in sysbus_try_create_simple()
>> to adjust device's property on board's demand.
>>
>> (This patch is needed by the later one which changes hpet's intcap
>> property)
>
> I don't think it is useful to add a new mechanism since there is an
> existing mechanism to set properties for compatibility (which I pointed
> you to earlier).  It is also incorrect because this will have an effect

Oh, just aware of this tricky method to set qdev's property. Thanks,
will try this way!

Regards,
Pingfan
> on all PC boards including pc-q35-1.7 and newer.
>
> You need to create a 1.7 machine like commit 45053fd (pc: Create
> pc-*-1.6 machine-types, 2013-05-27).  On top of this:
>
> * the 1.6 machines need to have a compatibility property in hw/i386/pc.h.
>
> * the pc-i440fx-1.7 machine needs to have a compatibility property for
> the same thing in hw/i386/pc_piix.c
>
> Paolo
>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  hw/core/sysbus.c    | 5 ++++-
>>  hw/i386/pc.c        | 2 +-
>>  include/hw/sysbus.h | 8 +++++---
>>  3 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
>> index 9004d8c..e894bbb 100644
>> --- a/hw/core/sysbus.c
>> +++ b/hw/core/sysbus.c
>> @@ -172,7 +172,7 @@ DeviceState *sysbus_create_varargs(const char *name,
>>      return dev;
>>  }
>>
>> -DeviceState *sysbus_try_create_varargs(const char *name,
>> +DeviceState *sysbus_try_create_varargs(const char *name, CompatSet set,
>>                                         hwaddr addr, ...)
>>  {
>>      DeviceState *dev;
>> @@ -185,6 +185,9 @@ DeviceState *sysbus_try_create_varargs(const char *name,
>>      if (!dev) {
>>          return NULL;
>>      }
>> +    if (set) {
>> +        set(dev);
>> +    }
>>      s = SYS_BUS_DEVICE(dev);
>>      qdev_init_nofail(dev);
>>      if (addr != (hwaddr)-1) {
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index e8bc8ce..09c10ac 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1247,7 +1247,7 @@ 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);
>> +        hpet = sysbus_try_create_simple("hpet", NULL, HPET_BASE, NULL);
>>
>>          if (hpet) {
>>              for (i = 0; i < GSI_NUM_PINS; i++) {
>> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
>> index bb50a87..47337f2 100644
>> --- a/include/hw/sysbus.h
>> +++ b/include/hw/sysbus.h
>> @@ -58,6 +58,8 @@ struct SysBusDevice {
>>      pio_addr_t pio[QDEV_MAX_PIO];
>>  };
>>
>> +typedef void (*CompatSet)(DeviceState *dev);
>> +
>>  void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory);
>>  MemoryRegion *sysbus_mmio_get_region(SysBusDevice *dev, int n);
>>  void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p);
>> @@ -77,7 +79,7 @@ MemoryRegion *sysbus_address_space(SysBusDevice *dev);
>>  /* Legacy helper function for creating devices.  */
>>  DeviceState *sysbus_create_varargs(const char *name,
>>                                   hwaddr addr, ...);
>> -DeviceState *sysbus_try_create_varargs(const char *name,
>> +DeviceState *sysbus_try_create_varargs(const char *name, CompatSet set,
>>                                         hwaddr addr, ...);
>>  static inline DeviceState *sysbus_create_simple(const char *name,
>>                                                hwaddr addr,
>> @@ -86,11 +88,11 @@ static inline DeviceState *sysbus_create_simple(const char *name,
>>      return sysbus_create_varargs(name, addr, irq, NULL);
>>  }
>>
>> -static inline DeviceState *sysbus_try_create_simple(const char *name,
>> +static inline DeviceState *sysbus_try_create_simple(const char *name, CompatSet set,
>>                                                      hwaddr addr,
>>                                                      qemu_irq irq)
>>  {
>> -    return sysbus_try_create_varargs(name, addr, irq, NULL);
>> +    return sysbus_try_create_varargs(name, set, addr, irq, NULL);
>>  }
>>
>>  #endif /* !HW_SYSBUS_H */
>>
>

Re: [Qemu-devel] [PATCH v3 2/3] qdev: interface for SysBusDevice to change property on requirement

[liu ping fan]

On Fri, Aug 30, 2013 at 8:32 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 30.08.2013 10:17, schrieb Paolo Bonzini:
>> Il 30/08/2013 09:53, Liu Ping Fan ha scritto:
>>> qdev's property can not be set after realized, but there is a
>>> requirement of adjusting device's behavior on different mother
>>> boards.  So introducing a callback in sysbus_try_create_simple()
>>> to adjust device's property on board's demand.
>>>
>>> (This patch is needed by the later one which changes hpet's intcap
>>> property)
>>
>> I don't think it is useful to add a new mechanism since there is an
>> existing mechanism to set properties for compatibility (which I pointed
>> you to earlier).  It is also incorrect because this will have an effect
>> on all PC boards including pc-q35-1.7 and newer.
>>
>> You need to create a 1.7 machine like commit 45053fd (pc: Create
>> pc-*-1.6 machine-types, 2013-05-27).
>
> Stefan already has than in his net tree and just needs to (rebase and)
> flush his queue!
>
Thanks, will do like it.

Regards,
Pingfan