[v3,2/3] qdev: interface for SysBusDevice to change property on requirement

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

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

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

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

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

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