| Message ID | 1396618620-27823-29-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/05/2014 12:36 AM, 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 <imammedo@redhat.com> > --- > 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(); wow. just wow.
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 <imammedo@redhat.com> > --- > 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? > 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
On Mon, 7 Apr 2014 13:23:54 +0300 "Michael S. Tsirkin" <mst@redhat.com> 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 <imammedo@redhat.com> > > --- > > 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
On Mon, Apr 07, 2014 at 01:07:53PM +1000, Alexey Kardashevskiy wrote: > On 04/05/2014 12:36 AM, 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 <imammedo@redhat.com> > > --- > > 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(); > > > wow. just wow. I had to read the C99 spec to find out if this was safe. :-) But I believe it is readable, I wouldn't mind keeping it that way.
On Mon, 7 Apr 2014 11:13:01 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Apr 07, 2014 at 01:07:53PM +1000, Alexey Kardashevskiy wrote: > > On 04/05/2014 12:36 AM, 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 <imammedo@redhat.com> > > > --- > > > 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 : ; ; > > > > > > wow. just wow. > > I had to read the C99 spec to find out if this was safe. :-) > > But I believe it is readable, I wouldn't mind keeping it that way. > I'll change it to a less obscure form: acpi_dev = piix4_pm_find(); if (!acpi_dev) { acpi_dev = ich9_lpc_find(); }
On Mon, 7 Apr 2014 13:23:54 +0300 "Michael S. Tsirkin" <mst@redhat.com> 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 <imammedo@redhat.com> > > --- > > 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 > > 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
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" <mst@redhat.com> 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 <imammedo@redhat.com> > > > --- > > > 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. > > > 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
On Mon, Apr 07, 2014 at 04:26:02PM +0200, Igor Mammedov wrote: > On Mon, 7 Apr 2014 11:13:01 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Mon, Apr 07, 2014 at 01:07:53PM +1000, Alexey Kardashevskiy wrote: > > > On 04/05/2014 12:36 AM, 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 <imammedo@redhat.com> > > > > --- > > > > 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 : ; > ; > > > > > > > > > wow. just wow. > > > > I had to read the C99 spec to find out if this was safe. :-) > > > > But I believe it is readable, I wouldn't mind keeping it that way. > > > I'll change it to a less obscure form: > > acpi_dev = piix4_pm_find(); > if (!acpi_dev) { > acpi_dev = ich9_lpc_find(); > } or Object *piix = piix4_pm_find(); Object *lpc = ich9_lpc_find(); assert(!!piix != !!lpc); so we verify it's one of the other.
On Mon, 7 Apr 2014 18:21:14 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Mon, Apr 07, 2014 at 04:26:02PM +0200, Igor Mammedov wrote: > > On Mon, 7 Apr 2014 11:13:01 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Mon, Apr 07, 2014 at 01:07:53PM +1000, Alexey Kardashevskiy wrote: > > > > On 04/05/2014 12:36 AM, 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 <imammedo@redhat.com> > > > > > --- > > > > > 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 : ; > > ; > > > > > > > > > > > > wow. just wow. > > > > > > I had to read the C99 spec to find out if this was safe. :-) > > > > > > But I believe it is readable, I wouldn't mind keeping it that way. > > > > > I'll change it to a less obscure form: > > > > acpi_dev = piix4_pm_find(); > > if (!acpi_dev) { > > acpi_dev = ich9_lpc_find(); > > } > > > or > > Object *piix = piix4_pm_find(); > Object *lpc = ich9_lpc_find(); > assert(!!piix != !!lpc); wouldn't that assert on device_add if qemu was started without ACPI? Exiting with error if acpi_dev == NULL seems safer here. > > > so we verify it's one of the other. >
On Mon, 7 Apr 2014 18:14:51 +0300 "Michael S. Tsirkin" <mst@redhat.com> 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" <mst@redhat.com> 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 <imammedo@redhat.com> > > > > --- > > > > 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). > > > > > 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 >
Il 11/04/2014 05:14, Igor Mammedov ha scritto: >>>> > > > 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? That's a nice solution. (It would be a link<HotplugHandler>, right?) Paolo >> > 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). >
On Mon, 14 Apr 2014 13:25:59 -0400 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 11/04/2014 05:14, Igor Mammedov ha scritto: > >>>> > > > 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? > > That's a nice solution. (It would be a link<HotplugHandler>, right?) Yep, but still named acpi_dev :), so that each board piix4/q35 could setup it's own device in board init code and generic code just use what board has provided. > > Paolo > > > >> > 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). > > >
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" <mst@redhat.com> 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" <mst@redhat.com> 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 <imammedo@redhat.com> > > > > > --- > > > > > 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
On Wed, 16 Apr 2014 15:02:40 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > 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" <mst@redhat.com> 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" <mst@redhat.com> 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 <imammedo@redhat.com> > > > > > > --- > > > > > > 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. Pointer/link to ACPI in PCMachine is nothing more than a convenience, to avoid lookups each time we need to get property from ACPI device. It's not limited to hotplug handler in any way and could/would be used in acpi_build.c as well. I guess that under "allow guests to disable ACPI" you mean switching hotplug handlers on pci bridges, i.e. not really removing/disabling ACPI device itself. About switching ACPI/SHPC hotplug modes, do you have an idea how it could be implemented, starting from guest side and till the switch actually happens? > > > > > > > > > > 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
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); 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);
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 <imammedo@redhat.com> --- hw/i386/pc.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)