Re: [Qemu-devel] [PATCH 1/5] hw/ppc: setting spapr_drc_detach_cb in spapr_dr_connector_new

[Laurent Vivier <lvivier@redhat.com>]

On 30/04/2017 19:25, Daniel Henrique Barboza wrote:
> The idea of moving the detach callback functions to the constructor
> of the dr_connector is to set them statically at init time, avoiding
> any post-load hooks to restore it (after a migration, for example).
> 
> Summary of changes:
> 
> - hw/ppc/spapr_drc.c and include/hw/ppc/spapr_drc.h:
>     *  spapr_dr_connector_new() now has an additional parameter,
> spapr_drc_detach_cb *detach_cb
>     *  'spapr_drc_detach_cb *detach_cb' parameter was removed of
> the detach function pointer in sPAPRDRConnectorClass
> 
> - hw/ppc/spapr_pci.c:
>     * the callback 'spapr_phb_remove_pci_device_cb' is now passed
> as a parameter in 'spapr_dr_connector_new' instead of 'drck->detach()'
> 
> - hw/ppc/spapr.c:
>     * 'spapr_create_lmb_dr_connectors' now passes the callback
> 'spapr_lmb_release' to 'spapr_dr_connector_new' instead of 'drck-detach()'
>     * 'spapr_init_cpus' now passes the callback 'spapr_core_release'
> to 'spapr_dr_connector_new' instead of 'drck-detach()'
>     * moved the callback functions up in the code so they can be referenced
> by 'spapr_create_lmb_dr_connectors' and 'spapr_init_cpus'
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c             | 71 +++++++++++++++++++++++-----------------------
>  hw/ppc/spapr_drc.c         | 17 ++++++-----
>  hw/ppc/spapr_pci.c         |  5 ++--
>  include/hw/ppc/spapr_drc.h |  4 +--
>  4 files changed, 49 insertions(+), 48 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 80d12d0..bc11757 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1887,6 +1887,29 @@ static void spapr_drc_reset(void *opaque)
>      }
>  }
>  
> +typedef struct sPAPRDIMMState {
> +    uint32_t nr_lmbs;
> +} sPAPRDIMMState;
> +
> +static void spapr_lmb_release(DeviceState *dev, void *opaque)
> +{
> +    sPAPRDIMMState *ds = (sPAPRDIMMState *)opaque;
> +    HotplugHandler *hotplug_ctrl;
> +
> +    if (--ds->nr_lmbs) {
> +        return;
> +    }
> +
> +    g_free(ds);
> +
> +    /*
> +     * Now that all the LMBs have been removed by the guest, call the
> +     * pc-dimm unplug handler to cleanup up the pc-dimm device.
> +     */
> +    hotplug_ctrl = qdev_get_hotplug_handler(dev);
> +    hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
> +}
> +
>  static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
>  {
>      MachineState *machine = MACHINE(spapr);
> @@ -1900,7 +1923,7 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
>  
>          addr = i * lmb_size + spapr->hotplug_memory.base;
>          drc = spapr_dr_connector_new(OBJECT(spapr), SPAPR_DR_CONNECTOR_TYPE_LMB,
> -                                     addr/lmb_size);
> +                                     (addr / lmb_size), spapr_lmb_release);

You have added useless parenthesis around "addr / lmb_size".

Reviewed-by: Laurent Vivier <lvivier@redhat.com>