subject:"\[Qemu\-devel\] \[PATCH for\-2.13 2\/4\] platform\-bus\-device\: use device plug callback instead of machine

Re: [Qemu-devel] [PATCH for-2.13 2/4] platform-bus-device: use device plug callback instead of machine_done notifier

2018-04-16 Thread David Gibson

On Mon, Apr 16, 2018 at 10:19:14AM +0200, Igor Mammedov wrote:
> On Mon, 16 Apr 2018 12:43:55 +1000
> David Gibson  wrote:
> 
> > On Thu, Apr 12, 2018 at 06:40:19PM +0200, Igor Mammedov wrote:
> > > platform-bus were using machine_done notifier to get and map
> > > (assign irq/mmio resources) dynamically added sysbus devices
> > > after all '-device' options had been processed.
> > > That however creates non obvious dependencies on ordering of
> > > machine_done notifiers and requires carefull line juggling
> > > to keep it working. For example see comment above
> > > create_platform_bus() and 'straitforward' arm_load_kernel()
> > > had to converted to machine_done notifier and that lead to
> > > yet another machine_done notifier to keep it working
> > > arm_register_platform_bus_fdt_creator().
> > > 
> > > Instead of hiding resource assignment in platform-bus-device
> > > to magically initialize sysbus devices, use device plug
> > > callback and assign resources explicitly at board level
> > > at the moment each -device option is being processed.
> > > 
> > > That adds a bunch of machine declaration boiler plate to
> > > e500plat board, similar to ARM/x86 but gets rid of hidden
> > > machine_done notifier and would allow to remove the dependent
> > > notifiers in ARM code simplifying it and making code flow
> > > easier to follow.
> > > 
> > > Signed-off-by: Igor Mammedov 
> > > ---
> > > CC: ag...@suse.de
> > > CC: da...@gibson.dropbear.id.au
> > > CC: qemu-...@nongnu.org
> > > ---
> > >  hw/ppc/e500.h |  3 +++
> > >  include/hw/arm/virt.h |  3 +++
> > >  include/hw/platform-bus.h |  4 ++--
> > >  hw/arm/sysbus-fdt.c   |  3 ---
> > >  hw/arm/virt.c | 36 
> > >  hw/core/platform-bus.c| 29 ---
> > >  hw/ppc/e500.c | 37 +
> > >  hw/ppc/e500plat.c | 60 
> > > +--
> > >  8 files changed, 129 insertions(+), 46 deletions(-)
> > > 
> > > diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> > > index 70ba1d8..d0f8ddd 100644
> > > --- a/hw/ppc/e500.h
> > > +++ b/hw/ppc/e500.h
> > > @@ -2,6 +2,7 @@
> > >  #define PPCE500_H
> > >  
> > >  #include "hw/boards.h"
> > > +#include "hw/sysbus.h"
> > >  
> > >  typedef struct PPCE500Params {
> > >  int pci_first_slot;
> > > @@ -26,6 +27,8 @@ typedef struct PPCE500Params {
> > >  
> > >  void ppce500_init(MachineState *machine, PPCE500Params *params);
> > >  
> > > +void ppce500_plug_dynamic_sysbus_device(SysBusDevice *sbdev);
> > > +
> > >  hwaddr booke206_page_size_to_tlb(uint64_t size);
> > >  
> > >  #endif
> > > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > > index ba0c1a4..5535760 100644
> > > --- a/include/hw/arm/virt.h
> > > +++ b/include/hw/arm/virt.h
> > > @@ -86,11 +86,14 @@ typedef struct {
> > >  bool no_pmu;
> > >  bool claim_edge_triggered_timers;
> > >  bool smbios_old_sys_ver;
> > > +HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> > > +   DeviceState *dev);
> > >  } VirtMachineClass;
> > >  
> > >  typedef struct {
> > >  MachineState parent;
> > >  Notifier machine_done;
> > > +DeviceState *platform_bus_dev;
> > >  FWCfgState *fw_cfg;
> > >  bool secure;
> > >  bool highmem;
> > > diff --git a/include/hw/platform-bus.h b/include/hw/platform-bus.h
> > > index a00775c..19e20c5 100644
> > > --- a/include/hw/platform-bus.h
> > > +++ b/include/hw/platform-bus.h
> > > @@ -37,8 +37,6 @@ typedef struct PlatformBusDevice PlatformBusDevice;
> > >  struct PlatformBusDevice {
> > >  /*< private >*/
> > >  SysBusDevice parent_obj;
> > > -Notifier notifier;
> > > -bool done_gathering;
> > >  
> > >  /*< public >*/
> > >  uint32_t mmio_size;
> > > @@ -54,4 +52,6 @@ int platform_bus_get_irqn(PlatformBusDevice 
> > > *platform_bus, SysBusDevice *sbdev,
> > >  hwaddr platform_bus_get_mmio_addr(PlatformBusDevice *pbus, SysBusDevice 
> > > *sbdev,
> > >int n);
> > >  
> > > +void platform_bus_link_device(PlatformBusDevice *pbus, SysBusDevice 
> > > *sbdev);
> > > +
> > >  #endif /* HW_PLATFORM_BUS_H */
> > > diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> > > index d68e3dc..80ff70e 100644
> > > --- a/hw/arm/sysbus-fdt.c
> > > +++ b/hw/arm/sysbus-fdt.c
> > > @@ -506,9 +506,6 @@ static void 
> > > add_all_platform_bus_fdt_nodes(ARMPlatformBusFDTParams *fdt_params)
> > >  dev = qdev_find_recursive(sysbus_get_default(), 
> > > TYPE_PLATFORM_BUS_DEVICE);
> > >  pbus = PLATFORM_BUS_DEVICE(dev);
> > >  
> > > -/* We can only create dt nodes for dynamic devices when they're 
> > > ready */
> > > -assert(pbus->done_gathering);
> > > -
> > >  PlatformBusFDTData data = {
> > >  .fdt = fdt,
> > >  .irq_start = irq_start,
> > > diff --git a/hw/arm/virt.c

Re: [Qemu-devel] [PATCH for-2.13 2/4] platform-bus-device: use device plug callback instead of machine_done notifier

2018-04-16 Thread Peter Maydell

On 12 April 2018 at 17:40, Igor Mammedov  wrote:
> platform-bus were using machine_done notifier to get and map
> (assign irq/mmio resources) dynamically added sysbus devices
> after all '-device' options had been processed.
> That however creates non obvious dependencies on ordering of
> machine_done notifiers and requires carefull line juggling
> to keep it working. For example see comment above
> create_platform_bus() and 'straitforward' arm_load_kernel()
> had to converted to machine_done notifier and that lead to
> yet another machine_done notifier to keep it working
> arm_register_platform_bus_fdt_creator().
>
> Instead of hiding resource assignment in platform-bus-device
> to magically initialize sysbus devices, use device plug
> callback and assign resources explicitly at board level
> at the moment each -device option is being processed.
>
> That adds a bunch of machine declaration boiler plate to
> e500plat board, similar to ARM/x86 but gets rid of hidden
> machine_done notifier and would allow to remove the dependent
> notifiers in ARM code simplifying it and making code flow
> easier to follow.
>
> Signed-off-by: Igor Mammedov 

> +static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
> +DeviceState *dev, Error **errp)
> +{
> +VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> +
> +if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
> +if (vms->platform_bus_dev) {
> +
> platform_bus_link_device(PLATFORM_BUS_DEVICE(vms->platform_bus_dev),
> + SYS_BUS_DEVICE(dev));
> +}
> +}
> +}
> +
> +static HotplugHandler *virt_machine_get_hotpug_handler(MachineState *machine,
> +   DeviceState *dev)

Nit: typo in function name, should be "hotplug".

Will let others review the meat of the patch.

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.13 2/4] platform-bus-device: use device plug callback instead of machine_done notifier

2018-04-16 Thread Igor Mammedov

On Mon, 16 Apr 2018 12:43:55 +1000
David Gibson  wrote:

> On Thu, Apr 12, 2018 at 06:40:19PM +0200, Igor Mammedov wrote:
> > platform-bus were using machine_done notifier to get and map
> > (assign irq/mmio resources) dynamically added sysbus devices
> > after all '-device' options had been processed.
> > That however creates non obvious dependencies on ordering of
> > machine_done notifiers and requires carefull line juggling
> > to keep it working. For example see comment above
> > create_platform_bus() and 'straitforward' arm_load_kernel()
> > had to converted to machine_done notifier and that lead to
> > yet another machine_done notifier to keep it working
> > arm_register_platform_bus_fdt_creator().
> > 
> > Instead of hiding resource assignment in platform-bus-device
> > to magically initialize sysbus devices, use device plug
> > callback and assign resources explicitly at board level
> > at the moment each -device option is being processed.
> > 
> > That adds a bunch of machine declaration boiler plate to
> > e500plat board, similar to ARM/x86 but gets rid of hidden
> > machine_done notifier and would allow to remove the dependent
> > notifiers in ARM code simplifying it and making code flow
> > easier to follow.
> > 
> > Signed-off-by: Igor Mammedov 
> > ---
> > CC: ag...@suse.de
> > CC: da...@gibson.dropbear.id.au
> > CC: qemu-...@nongnu.org
> > ---
> >  hw/ppc/e500.h |  3 +++
> >  include/hw/arm/virt.h |  3 +++
> >  include/hw/platform-bus.h |  4 ++--
> >  hw/arm/sysbus-fdt.c   |  3 ---
> >  hw/arm/virt.c | 36 
> >  hw/core/platform-bus.c| 29 ---
> >  hw/ppc/e500.c | 37 +
> >  hw/ppc/e500plat.c | 60 
> > +--
> >  8 files changed, 129 insertions(+), 46 deletions(-)
> > 
> > diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> > index 70ba1d8..d0f8ddd 100644
> > --- a/hw/ppc/e500.h
> > +++ b/hw/ppc/e500.h
> > @@ -2,6 +2,7 @@
> >  #define PPCE500_H
> >  
> >  #include "hw/boards.h"
> > +#include "hw/sysbus.h"
> >  
> >  typedef struct PPCE500Params {
> >  int pci_first_slot;
> > @@ -26,6 +27,8 @@ typedef struct PPCE500Params {
> >  
> >  void ppce500_init(MachineState *machine, PPCE500Params *params);
> >  
> > +void ppce500_plug_dynamic_sysbus_device(SysBusDevice *sbdev);
> > +
> >  hwaddr booke206_page_size_to_tlb(uint64_t size);
> >  
> >  #endif
> > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > index ba0c1a4..5535760 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -86,11 +86,14 @@ typedef struct {
> >  bool no_pmu;
> >  bool claim_edge_triggered_timers;
> >  bool smbios_old_sys_ver;
> > +HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> > +   DeviceState *dev);
> >  } VirtMachineClass;
> >  
> >  typedef struct {
> >  MachineState parent;
> >  Notifier machine_done;
> > +DeviceState *platform_bus_dev;
> >  FWCfgState *fw_cfg;
> >  bool secure;
> >  bool highmem;
> > diff --git a/include/hw/platform-bus.h b/include/hw/platform-bus.h
> > index a00775c..19e20c5 100644
> > --- a/include/hw/platform-bus.h
> > +++ b/include/hw/platform-bus.h
> > @@ -37,8 +37,6 @@ typedef struct PlatformBusDevice PlatformBusDevice;
> >  struct PlatformBusDevice {
> >  /*< private >*/
> >  SysBusDevice parent_obj;
> > -Notifier notifier;
> > -bool done_gathering;
> >  
> >  /*< public >*/
> >  uint32_t mmio_size;
> > @@ -54,4 +52,6 @@ int platform_bus_get_irqn(PlatformBusDevice 
> > *platform_bus, SysBusDevice *sbdev,
> >  hwaddr platform_bus_get_mmio_addr(PlatformBusDevice *pbus, SysBusDevice 
> > *sbdev,
> >int n);
> >  
> > +void platform_bus_link_device(PlatformBusDevice *pbus, SysBusDevice 
> > *sbdev);
> > +
> >  #endif /* HW_PLATFORM_BUS_H */
> > diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> > index d68e3dc..80ff70e 100644
> > --- a/hw/arm/sysbus-fdt.c
> > +++ b/hw/arm/sysbus-fdt.c
> > @@ -506,9 +506,6 @@ static void 
> > add_all_platform_bus_fdt_nodes(ARMPlatformBusFDTParams *fdt_params)
> >  dev = qdev_find_recursive(sysbus_get_default(), 
> > TYPE_PLATFORM_BUS_DEVICE);
> >  pbus = PLATFORM_BUS_DEVICE(dev);
> >  
> > -/* We can only create dt nodes for dynamic devices when they're ready 
> > */
> > -assert(pbus->done_gathering);
> > -
> >  PlatformBusFDTData data = {
> >  .fdt = fdt,
> >  .irq_start = irq_start,
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 94dcb12..2e10d8b 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1087,6 +1087,7 @@ static void create_platform_bus(VirtMachineState 
> > *vms, qemu_irq *pic)
> >  qdev_prop_set_uint32(dev, "mmio_size",
> >  platform_bus_params.platform_bus_size);
> >

Re: [Qemu-devel] [PATCH for-2.13 2/4] platform-bus-device: use device plug callback instead of machine_done notifier

2018-04-16 Thread Igor Mammedov

On Fri, 13 Apr 2018 20:00:18 +0200
Auger Eric  wrote:

> Hi Igor,
> 
> On 12/04/18 18:40, Igor Mammedov wrote:
> > platform-bus were using machine_done notifier to get and map
> > (assign irq/mmio resources) dynamically added sysbus devices
> > after all '-device' options had been processed.
> > That however creates non obvious dependencies on ordering of
> > machine_done notifiers and requires carefull line juggling
> > to keep it working. For example see comment above
> > create_platform_bus() and 'straitforward' arm_load_kernel()
> > had to converted to machine_done notifier and that lead to
> > yet another machine_done notifier to keep it working
> > arm_register_platform_bus_fdt_creator().
> > 
> > Instead of hiding resource assignment in platform-bus-device
> > to magically initialize sysbus devices, use device plug
> > callback and assign resources explicitly at board level
> > at the moment each -device option is being processed.
> > 
> > That adds a bunch of machine declaration boiler plate to
> > e500plat board, similar to ARM/x86 but gets rid of hidden
> > machine_done notifier and would allow to remove the dependent
> > notifiers in ARM code simplifying it and making code flow
> > easier to follow.
> > 
> > Signed-off-by: Igor Mammedov 
> > ---
> > CC: ag...@suse.de
> > CC: da...@gibson.dropbear.id.au
> > CC: qemu-...@nongnu.org
> > ---
> >  hw/ppc/e500.h |  3 +++
> >  include/hw/arm/virt.h |  3 +++
> >  include/hw/platform-bus.h |  4 ++--
> >  hw/arm/sysbus-fdt.c   |  3 ---
> >  hw/arm/virt.c | 36 
> >  hw/core/platform-bus.c| 29 ---
> >  hw/ppc/e500.c | 37 +
> >  hw/ppc/e500plat.c | 60 
> > +--
> >  8 files changed, 129 insertions(+), 46 deletions(-)
> > 
> > diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> > index 70ba1d8..d0f8ddd 100644
> > --- a/hw/ppc/e500.h
> > +++ b/hw/ppc/e500.h
> > @@ -2,6 +2,7 @@
> >  #define PPCE500_H
> >  
> >  #include "hw/boards.h"
> > +#include "hw/sysbus.h"
> >  
> >  typedef struct PPCE500Params {
> >  int pci_first_slot;
> > @@ -26,6 +27,8 @@ typedef struct PPCE500Params {
> >  
> >  void ppce500_init(MachineState *machine, PPCE500Params *params);
> >  
> > +void ppce500_plug_dynamic_sysbus_device(SysBusDevice *sbdev);
> > +
> >  hwaddr booke206_page_size_to_tlb(uint64_t size);
> >  
> >  #endif
> > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > index ba0c1a4..5535760 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -86,11 +86,14 @@ typedef struct {
> >  bool no_pmu;
> >  bool claim_edge_triggered_timers;
> >  bool smbios_old_sys_ver;
> > +HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> > +   DeviceState *dev);
> >  } VirtMachineClass;
> >  
> >  typedef struct {
> >  MachineState parent;
> >  Notifier machine_done;
> > +DeviceState *platform_bus_dev;
> >  FWCfgState *fw_cfg;
> >  bool secure;
> >  bool highmem;
> > diff --git a/include/hw/platform-bus.h b/include/hw/platform-bus.h
> > index a00775c..19e20c5 100644
> > --- a/include/hw/platform-bus.h
> > +++ b/include/hw/platform-bus.h
> > @@ -37,8 +37,6 @@ typedef struct PlatformBusDevice PlatformBusDevice;
> >  struct PlatformBusDevice {
> >  /*< private >*/
> >  SysBusDevice parent_obj;
> > -Notifier notifier;
> > -bool done_gathering;
> >  
> >  /*< public >*/
> >  uint32_t mmio_size;
> > @@ -54,4 +52,6 @@ int platform_bus_get_irqn(PlatformBusDevice 
> > *platform_bus, SysBusDevice *sbdev,
> >  hwaddr platform_bus_get_mmio_addr(PlatformBusDevice *pbus, SysBusDevice 
> > *sbdev,
> >int n);
> >  
> > +void platform_bus_link_device(PlatformBusDevice *pbus, SysBusDevice 
> > *sbdev);
> > +
> >  #endif /* HW_PLATFORM_BUS_H */
> > diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> > index d68e3dc..80ff70e 100644
> > --- a/hw/arm/sysbus-fdt.c
> > +++ b/hw/arm/sysbus-fdt.c
> > @@ -506,9 +506,6 @@ static void 
> > add_all_platform_bus_fdt_nodes(ARMPlatformBusFDTParams *fdt_params)
> >  dev = qdev_find_recursive(sysbus_get_default(), 
> > TYPE_PLATFORM_BUS_DEVICE);
> >  pbus = PLATFORM_BUS_DEVICE(dev);
> >  
> > -/* We can only create dt nodes for dynamic devices when they're ready 
> > */
> > -assert(pbus->done_gathering);
> > -
> >  PlatformBusFDTData data = {
> >  .fdt = fdt,
> >  .irq_start = irq_start,
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 94dcb12..2e10d8b 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1087,6 +1087,7 @@ static void create_platform_bus(VirtMachineState 
> > *vms, qemu_irq *pic)
> >  qdev_prop_set_uint32(dev, "mmio_size",
> >  platform_bus_params.platform_bus_size);
> >

Re: [Qemu-devel] [PATCH for-2.13 2/4] platform-bus-device: use device plug callback instead of machine_done notifier

2018-04-15 Thread David Gibson

On Thu, Apr 12, 2018 at 06:40:19PM +0200, Igor Mammedov wrote:
> platform-bus were using machine_done notifier to get and map
> (assign irq/mmio resources) dynamically added sysbus devices
> after all '-device' options had been processed.
> That however creates non obvious dependencies on ordering of
> machine_done notifiers and requires carefull line juggling
> to keep it working. For example see comment above
> create_platform_bus() and 'straitforward' arm_load_kernel()
> had to converted to machine_done notifier and that lead to
> yet another machine_done notifier to keep it working
> arm_register_platform_bus_fdt_creator().
> 
> Instead of hiding resource assignment in platform-bus-device
> to magically initialize sysbus devices, use device plug
> callback and assign resources explicitly at board level
> at the moment each -device option is being processed.
> 
> That adds a bunch of machine declaration boiler plate to
> e500plat board, similar to ARM/x86 but gets rid of hidden
> machine_done notifier and would allow to remove the dependent
> notifiers in ARM code simplifying it and making code flow
> easier to follow.
> 
> Signed-off-by: Igor Mammedov 
> ---
> CC: ag...@suse.de
> CC: da...@gibson.dropbear.id.au
> CC: qemu-...@nongnu.org
> ---
>  hw/ppc/e500.h |  3 +++
>  include/hw/arm/virt.h |  3 +++
>  include/hw/platform-bus.h |  4 ++--
>  hw/arm/sysbus-fdt.c   |  3 ---
>  hw/arm/virt.c | 36 
>  hw/core/platform-bus.c| 29 ---
>  hw/ppc/e500.c | 37 +
>  hw/ppc/e500plat.c | 60 
> +--
>  8 files changed, 129 insertions(+), 46 deletions(-)
> 
> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> index 70ba1d8..d0f8ddd 100644
> --- a/hw/ppc/e500.h
> +++ b/hw/ppc/e500.h
> @@ -2,6 +2,7 @@
>  #define PPCE500_H
>  
>  #include "hw/boards.h"
> +#include "hw/sysbus.h"
>  
>  typedef struct PPCE500Params {
>  int pci_first_slot;
> @@ -26,6 +27,8 @@ typedef struct PPCE500Params {
>  
>  void ppce500_init(MachineState *machine, PPCE500Params *params);
>  
> +void ppce500_plug_dynamic_sysbus_device(SysBusDevice *sbdev);
> +
>  hwaddr booke206_page_size_to_tlb(uint64_t size);
>  
>  #endif
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index ba0c1a4..5535760 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -86,11 +86,14 @@ typedef struct {
>  bool no_pmu;
>  bool claim_edge_triggered_timers;
>  bool smbios_old_sys_ver;
> +HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> +   DeviceState *dev);
>  } VirtMachineClass;
>  
>  typedef struct {
>  MachineState parent;
>  Notifier machine_done;
> +DeviceState *platform_bus_dev;
>  FWCfgState *fw_cfg;
>  bool secure;
>  bool highmem;
> diff --git a/include/hw/platform-bus.h b/include/hw/platform-bus.h
> index a00775c..19e20c5 100644
> --- a/include/hw/platform-bus.h
> +++ b/include/hw/platform-bus.h
> @@ -37,8 +37,6 @@ typedef struct PlatformBusDevice PlatformBusDevice;
>  struct PlatformBusDevice {
>  /*< private >*/
>  SysBusDevice parent_obj;
> -Notifier notifier;
> -bool done_gathering;
>  
>  /*< public >*/
>  uint32_t mmio_size;
> @@ -54,4 +52,6 @@ int platform_bus_get_irqn(PlatformBusDevice *platform_bus, 
> SysBusDevice *sbdev,
>  hwaddr platform_bus_get_mmio_addr(PlatformBusDevice *pbus, SysBusDevice 
> *sbdev,
>int n);
>  
> +void platform_bus_link_device(PlatformBusDevice *pbus, SysBusDevice *sbdev);
> +
>  #endif /* HW_PLATFORM_BUS_H */
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index d68e3dc..80ff70e 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -506,9 +506,6 @@ static void 
> add_all_platform_bus_fdt_nodes(ARMPlatformBusFDTParams *fdt_params)
>  dev = qdev_find_recursive(sysbus_get_default(), 
> TYPE_PLATFORM_BUS_DEVICE);
>  pbus = PLATFORM_BUS_DEVICE(dev);
>  
> -/* We can only create dt nodes for dynamic devices when they're ready */
> -assert(pbus->done_gathering);
> -
>  PlatformBusFDTData data = {
>  .fdt = fdt,
>  .irq_start = irq_start,
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 94dcb12..2e10d8b 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1087,6 +1087,7 @@ static void create_platform_bus(VirtMachineState *vms, 
> qemu_irq *pic)
>  qdev_prop_set_uint32(dev, "mmio_size",
>  platform_bus_params.platform_bus_size);
>  qdev_init_nofail(dev);
> +vms->platform_bus_dev = dev;
>  s = SYS_BUS_DEVICE(dev);
>  
>  for (i = 0; i < platform_bus_params.platform_bus_num_irqs; i++) {
> @@ -1536,9 +1537,37 @@ static const CPUArchIdList 
> *virt_possible_cpu_arch_ids(MachineState *ms)
>  return ms->possible_cpus;
>  }
>  
> +static void

Re: [Qemu-devel] [PATCH for-2.13 2/4] platform-bus-device: use device plug callback instead of machine_done notifier

2018-04-13 Thread Auger Eric

Hi Igor,

On 12/04/18 18:40, Igor Mammedov wrote:
> platform-bus were using machine_done notifier to get and map
> (assign irq/mmio resources) dynamically added sysbus devices
> after all '-device' options had been processed.
> That however creates non obvious dependencies on ordering of
> machine_done notifiers and requires carefull line juggling
> to keep it working. For example see comment above
> create_platform_bus() and 'straitforward' arm_load_kernel()
> had to converted to machine_done notifier and that lead to
> yet another machine_done notifier to keep it working
> arm_register_platform_bus_fdt_creator().
> 
> Instead of hiding resource assignment in platform-bus-device
> to magically initialize sysbus devices, use device plug
> callback and assign resources explicitly at board level
> at the moment each -device option is being processed.
> 
> That adds a bunch of machine declaration boiler plate to
> e500plat board, similar to ARM/x86 but gets rid of hidden
> machine_done notifier and would allow to remove the dependent
> notifiers in ARM code simplifying it and making code flow
> easier to follow.
> 
> Signed-off-by: Igor Mammedov 
> ---
> CC: ag...@suse.de
> CC: da...@gibson.dropbear.id.au
> CC: qemu-...@nongnu.org
> ---
>  hw/ppc/e500.h |  3 +++
>  include/hw/arm/virt.h |  3 +++
>  include/hw/platform-bus.h |  4 ++--
>  hw/arm/sysbus-fdt.c   |  3 ---
>  hw/arm/virt.c | 36 
>  hw/core/platform-bus.c| 29 ---
>  hw/ppc/e500.c | 37 +
>  hw/ppc/e500plat.c | 60 
> +--
>  8 files changed, 129 insertions(+), 46 deletions(-)
> 
> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
> index 70ba1d8..d0f8ddd 100644
> --- a/hw/ppc/e500.h
> +++ b/hw/ppc/e500.h
> @@ -2,6 +2,7 @@
>  #define PPCE500_H
>  
>  #include "hw/boards.h"
> +#include "hw/sysbus.h"
>  
>  typedef struct PPCE500Params {
>  int pci_first_slot;
> @@ -26,6 +27,8 @@ typedef struct PPCE500Params {
>  
>  void ppce500_init(MachineState *machine, PPCE500Params *params);
>  
> +void ppce500_plug_dynamic_sysbus_device(SysBusDevice *sbdev);
> +
>  hwaddr booke206_page_size_to_tlb(uint64_t size);
>  
>  #endif
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index ba0c1a4..5535760 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -86,11 +86,14 @@ typedef struct {
>  bool no_pmu;
>  bool claim_edge_triggered_timers;
>  bool smbios_old_sys_ver;
> +HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
> +   DeviceState *dev);
>  } VirtMachineClass;
>  
>  typedef struct {
>  MachineState parent;
>  Notifier machine_done;
> +DeviceState *platform_bus_dev;
>  FWCfgState *fw_cfg;
>  bool secure;
>  bool highmem;
> diff --git a/include/hw/platform-bus.h b/include/hw/platform-bus.h
> index a00775c..19e20c5 100644
> --- a/include/hw/platform-bus.h
> +++ b/include/hw/platform-bus.h
> @@ -37,8 +37,6 @@ typedef struct PlatformBusDevice PlatformBusDevice;
>  struct PlatformBusDevice {
>  /*< private >*/
>  SysBusDevice parent_obj;
> -Notifier notifier;
> -bool done_gathering;
>  
>  /*< public >*/
>  uint32_t mmio_size;
> @@ -54,4 +52,6 @@ int platform_bus_get_irqn(PlatformBusDevice *platform_bus, 
> SysBusDevice *sbdev,
>  hwaddr platform_bus_get_mmio_addr(PlatformBusDevice *pbus, SysBusDevice 
> *sbdev,
>int n);
>  
> +void platform_bus_link_device(PlatformBusDevice *pbus, SysBusDevice *sbdev);
> +
>  #endif /* HW_PLATFORM_BUS_H */
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index d68e3dc..80ff70e 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -506,9 +506,6 @@ static void 
> add_all_platform_bus_fdt_nodes(ARMPlatformBusFDTParams *fdt_params)
>  dev = qdev_find_recursive(sysbus_get_default(), 
> TYPE_PLATFORM_BUS_DEVICE);
>  pbus = PLATFORM_BUS_DEVICE(dev);
>  
> -/* We can only create dt nodes for dynamic devices when they're ready */
> -assert(pbus->done_gathering);
> -
>  PlatformBusFDTData data = {
>  .fdt = fdt,
>  .irq_start = irq_start,
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 94dcb12..2e10d8b 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1087,6 +1087,7 @@ static void create_platform_bus(VirtMachineState *vms, 
> qemu_irq *pic)
>  qdev_prop_set_uint32(dev, "mmio_size",
>  platform_bus_params.platform_bus_size);
>  qdev_init_nofail(dev);
> +vms->platform_bus_dev = dev;
>  s = SYS_BUS_DEVICE(dev);
>  
>  for (i = 0; i < platform_bus_params.platform_bus_num_irqs; i++) {
> @@ -1536,9 +1537,37 @@ static const CPUArchIdList 
> *virt_possible_cpu_arch_ids(MachineState *ms)
>  return ms->possible_cpus;
>  }
>  
> +static void

[Qemu-devel] [PATCH for-2.13 2/4] platform-bus-device: use device plug callback instead of machine_done notifier

2018-04-12 Thread Igor Mammedov

platform-bus were using machine_done notifier to get and map
(assign irq/mmio resources) dynamically added sysbus devices
after all '-device' options had been processed.
That however creates non obvious dependencies on ordering of
machine_done notifiers and requires carefull line juggling
to keep it working. For example see comment above
create_platform_bus() and 'straitforward' arm_load_kernel()
had to converted to machine_done notifier and that lead to
yet another machine_done notifier to keep it working
arm_register_platform_bus_fdt_creator().

Instead of hiding resource assignment in platform-bus-device
to magically initialize sysbus devices, use device plug
callback and assign resources explicitly at board level
at the moment each -device option is being processed.

That adds a bunch of machine declaration boiler plate to
e500plat board, similar to ARM/x86 but gets rid of hidden
machine_done notifier and would allow to remove the dependent
notifiers in ARM code simplifying it and making code flow
easier to follow.

Signed-off-by: Igor Mammedov 
---
CC: ag...@suse.de
CC: da...@gibson.dropbear.id.au
CC: qemu-...@nongnu.org
---
 hw/ppc/e500.h |  3 +++
 include/hw/arm/virt.h |  3 +++
 include/hw/platform-bus.h |  4 ++--
 hw/arm/sysbus-fdt.c   |  3 ---
 hw/arm/virt.c | 36 
 hw/core/platform-bus.c| 29 ---
 hw/ppc/e500.c | 37 +
 hw/ppc/e500plat.c | 60 +--
 8 files changed, 129 insertions(+), 46 deletions(-)

diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h
index 70ba1d8..d0f8ddd 100644
--- a/hw/ppc/e500.h
+++ b/hw/ppc/e500.h
@@ -2,6 +2,7 @@
 #define PPCE500_H
 
 #include "hw/boards.h"
+#include "hw/sysbus.h"
 
 typedef struct PPCE500Params {
 int pci_first_slot;
@@ -26,6 +27,8 @@ typedef struct PPCE500Params {
 
 void ppce500_init(MachineState *machine, PPCE500Params *params);
 
+void ppce500_plug_dynamic_sysbus_device(SysBusDevice *sbdev);
+
 hwaddr booke206_page_size_to_tlb(uint64_t size);
 
 #endif
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index ba0c1a4..5535760 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -86,11 +86,14 @@ typedef struct {
 bool no_pmu;
 bool claim_edge_triggered_timers;
 bool smbios_old_sys_ver;
+HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
+   DeviceState *dev);
 } VirtMachineClass;
 
 typedef struct {
 MachineState parent;
 Notifier machine_done;
+DeviceState *platform_bus_dev;
 FWCfgState *fw_cfg;
 bool secure;
 bool highmem;
diff --git a/include/hw/platform-bus.h b/include/hw/platform-bus.h
index a00775c..19e20c5 100644
--- a/include/hw/platform-bus.h
+++ b/include/hw/platform-bus.h
@@ -37,8 +37,6 @@ typedef struct PlatformBusDevice PlatformBusDevice;
 struct PlatformBusDevice {
 /*< private >*/
 SysBusDevice parent_obj;
-Notifier notifier;
-bool done_gathering;
 
 /*< public >*/
 uint32_t mmio_size;
@@ -54,4 +52,6 @@ int platform_bus_get_irqn(PlatformBusDevice *platform_bus, 
SysBusDevice *sbdev,
 hwaddr platform_bus_get_mmio_addr(PlatformBusDevice *pbus, SysBusDevice *sbdev,
   int n);
 
+void platform_bus_link_device(PlatformBusDevice *pbus, SysBusDevice *sbdev);
+
 #endif /* HW_PLATFORM_BUS_H */
diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index d68e3dc..80ff70e 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -506,9 +506,6 @@ static void 
add_all_platform_bus_fdt_nodes(ARMPlatformBusFDTParams *fdt_params)
 dev = qdev_find_recursive(sysbus_get_default(), TYPE_PLATFORM_BUS_DEVICE);
 pbus = PLATFORM_BUS_DEVICE(dev);
 
-/* We can only create dt nodes for dynamic devices when they're ready */
-assert(pbus->done_gathering);
-
 PlatformBusFDTData data = {
 .fdt = fdt,
 .irq_start = irq_start,
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 94dcb12..2e10d8b 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1087,6 +1087,7 @@ static void create_platform_bus(VirtMachineState *vms, 
qemu_irq *pic)
 qdev_prop_set_uint32(dev, "mmio_size",
 platform_bus_params.platform_bus_size);
 qdev_init_nofail(dev);
+vms->platform_bus_dev = dev;
 s = SYS_BUS_DEVICE(dev);
 
 for (i = 0; i < platform_bus_params.platform_bus_num_irqs; i++) {
@@ -1536,9 +1537,37 @@ static const CPUArchIdList 
*virt_possible_cpu_arch_ids(MachineState *ms)
 return ms->possible_cpus;
 }
 
+static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
+DeviceState *dev, Error **errp)
+{
+VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+
+if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
+if (vms->platform_bus_dev) {
+

Re: [Qemu-devel] [PATCH for-2.13 2/4] platform-bus-device: use device plug callback instead of machine_done notifier

Re: [Qemu-devel] [PATCH for-2.13 2/4] platform-bus-device: use device plug callback instead of machine_done notifier

Re: [Qemu-devel] [PATCH for-2.13 2/4] platform-bus-device: use device plug callback instead of machine_done notifier

Re: [Qemu-devel] [PATCH for-2.13 2/4] platform-bus-device: use device plug callback instead of machine_done notifier

Re: [Qemu-devel] [PATCH for-2.13 2/4] platform-bus-device: use device plug callback instead of machine_done notifier

Re: [Qemu-devel] [PATCH for-2.13 2/4] platform-bus-device: use device plug callback instead of machine_done notifier

[Qemu-devel] [PATCH for-2.13 2/4] platform-bus-device: use device plug callback instead of machine_done notifier

7 matches

Site Navigation

Mail list logo

Footer information