Re: [Qemu-devel] [PATCH 28/35] pc: propagate memory hotplug event to ACPI device
On Wed, 16 Apr 2014 15:02:40 +0300
"Michael S. Tsirkin" 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" 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" 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
> > > > > > ---
> > > > > > 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 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
Re: [Qemu-devel] [PATCH 28/35] pc: propagate memory hotplug event to ACPI device
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" 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" 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
> > > > > ---
> > > > > 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 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
Re: [Qemu-devel] [PATCH 28/35] pc: propagate memory hotplug event to ACPI device
On Mon, 14 Apr 2014 13:25:59 -0400 Paolo Bonzini 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 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, 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). > > > -- Regards, Igor
Re: [Qemu-devel] [PATCH 28/35] pc: propagate memory hotplug event to ACPI device
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 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, 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).
Re: [Qemu-devel] [PATCH 28/35] pc: propagate memory hotplug event to ACPI device
On Mon, 7 Apr 2014 18:21:14 +0300
"Michael S. Tsirkin" 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 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
> > > > > ---
> > > > > 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.
>
--
Regards,
Igor
Re: [Qemu-devel] [PATCH 28/35] pc: propagate memory hotplug event to ACPI device
On Mon, 7 Apr 2014 18:14:51 +0300
"Michael S. Tsirkin" 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" 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
> > > > ---
> > > > 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 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
>
--
Regards,
Igor
Re: [Qemu-devel] [PATCH 28/35] pc: propagate memory hotplug event to ACPI device
On Mon, Apr 07, 2014 at 04:26:02PM +0200, Igor Mammedov wrote:
> On Mon, 7 Apr 2014 11:13:01 -0300
> Eduardo Habkost 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
> > > > ---
> > > > 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.
Re: [Qemu-devel] [PATCH 28/35] pc: propagate memory hotplug event to ACPI device
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" 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
> > > ---
> > > 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 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
Re: [Qemu-devel] [PATCH 28/35] pc: propagate memory hotplug event to ACPI device
On Mon, 7 Apr 2014 11:13:01 -0300
Eduardo Habkost 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
> > > ---
> > > 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();
}
Re: [Qemu-devel] [PATCH 28/35] pc: propagate memory hotplug event to ACPI device
On Mon, 7 Apr 2014 13:23:54 +0300
"Michael S. Tsirkin" 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
> > ---
> > 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 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
Re: [Qemu-devel] [PATCH 28/35] pc: propagate memory hotplug event to ACPI device
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
> > ---
> > 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.
--
Eduardo
Re: [Qemu-devel] [PATCH 28/35] pc: propagate memory hotplug event to ACPI device
On Mon, 7 Apr 2014 13:23:54 +0300
"Michael S. Tsirkin" 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
> > ---
> > 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
Re: [Qemu-devel] [PATCH 28/35] pc: propagate memory hotplug event to ACPI device
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
> ---
> 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
Re: [Qemu-devel] [PATCH 28/35] pc: propagate memory hotplug event to ACPI device
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
> ---
> 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.
--
Alexey
[Qemu-devel] [PATCH 28/35] pc: propagate memory hotplug event to ACPI device
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
---
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);
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
Re: [Qemu-devel] [PATCH 28/35] pc: propagate memory hotplug event to ACPI device
On Fri, 04 Apr 2014 16:02:32 +0200
Paolo Bonzini wrote:
> Il 04/04/2014 15:36, Igor Mammedov ha scritto:
> > +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");
>
> errp, not &local_err.
>
> Paolo
>
or better for consistency with th rest of code:
+goto out;
-return;
> > +}
> > +
>
>
--
Regards,
Igor
Re: [Qemu-devel] [PATCH 28/35] pc: propagate memory hotplug event to ACPI device
Il 04/04/2014 15:36, Igor Mammedov ha scritto:
+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");
errp, not &local_err.
Paolo
+return;
+}
+
