Re: [Qemu-devel] [PATCH 28/35] pc: propagate memory hotplug event to ACPI device

[Igor Mammedov]

On Mon, 7 Apr 2014 13:23:54 +0300
"Michael S. Tsirkin" <address@hidden> wrote:

> On Fri, Apr 04, 2014 at 03:36:53PM +0200, Igor Mammedov wrote:
> > Notify PIIX4_PM/ICH9LPC device about hotplug event,
> > so that it would send SCI to guest notifying about
> > newly added memory.
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> >  hw/i386/pc.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 734c6ee..ee5cf88 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -60,6 +60,8 @@
> >  #include "acpi-build.h"
> >  #include "hw/mem/dimm.h"
> >  #include "trace.h"
> > +#include "hw/acpi/piix4.h"
> > +#include "hw/i386/ich9.h"
> >  
> >  /* debug PC/ISA interrupts */
> >  //#define DEBUG_IRQ
> > @@ -1484,6 +1486,8 @@ void qemu_register_pc_machine(QEMUMachine *m)
> >  static void pc_dimm_plug(HotplugHandler *hotplug_dev,
> >                           DeviceState *dev, Error **errp)
> >  {
> > +    Object *acpi_dev;
> > +    HotplugHandlerClass *hhc;
> >      Error *local_err = NULL;
> >      PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> >      DimmDevice *dimm = DIMM(dev);
> > @@ -1517,10 +1521,19 @@ static void pc_dimm_plug(HotplugHandler 
> > *hotplug_dev,
> >      }
> >      trace_mhp_pc_dimm_assigned_slot(dimm->slot);
> >  
> > +    acpi_dev = (acpi_dev = piix4_pm_find()) ? acpi_dev : ich9_lpc_find();
> > +    if (!acpi_dev) {
> > +        error_setg(&local_err,
> > +                   "memory hotplug is not enabled: missing acpi device");
> > +        return;
> > +    }
> > +
> > +    hhc = HOTPLUG_HANDLER_GET_CLASS(acpi_dev);
> 
> How about simply looking for a hotplug handler type device instead?
> We aren't likely to have many of these, are we?
There is at least 2 hotplug handlers that handle event for DIMM device,
this one in PCMachine and in acpi device.

Having explicit wiring where main handler forwards partially handled event
to another known in advance handler would be more simple and robust approach.
I think that's how real hardware works, i.e. when memory is hotplugged
it doesn't triggers signals to CPU or SHCP hotplug circuits.

Doing broadcast here would be overkill.

> 
> >      memory_region_add_subregion(&pcms->hotplug_memory,
> >                                  addr - pcms->hotplug_memory_base,
> >                                  mr);
> >      vmstate_register_ram(mr, dev);
> > +    hhc->plug(HOTPLUG_HANDLER(acpi_dev), dev, &local_err);
> >  
> >  out:
> >      error_propagate(errp, local_err);
> > -- 
> > 1.9.0