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

[Michael S. Tsirkin]

On Fri, Apr 11, 2014 at 11:14:00AM +0200, Igor Mammedov wrote:
> On Mon, 7 Apr 2014 18:14:51 +0300
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > On Mon, Apr 07, 2014 at 04:32:16PM +0200, Igor Mammedov wrote:
> > > 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?
> > > 
> > > How about adding link<acpi_device>  to PCMachine when it's created
> > > and use it instead of piix4_pm_find()/ich9_lpc_find() everywhere?
> > > that would allow to remove above searches in QOM tree and simplify
> > > code including acpi-build.c
> > 
> > So each acpi implementation registers a link at a pre-defined path?
> > I'm fine with this, need an ack from afaerber though.
> It could be a just plain pointer since it points to system device which
> won't disappear suddenly (i.e. it's not hot-unplugable).

It would be nice to support removing them if we want to allow guests to
disable ACPI and switch to native hotplug.

> > 
> > > > >      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
> > 
> 
> 
> -- 
> Regards,
>   Igor