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 <address@hidden>
> ---
>  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 */
>