qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: "Michael S. Tsirkin" <mst@redhat.com>
To: Igor Mammedov <imammedo@redhat.com>
Cc: peter.maydell@linaro.org, aik@ozlabs.ru, hutao@cn.fujitsu.com,
	mjt@tls.msk.ru, qemu-devel@nongnu.org, lcapitulino@redhat.com,
	kraxel@redhat.com, akong@redhat.com, quintela@redhat.com,
	armbru@redhat.com, aliguori@amazon.com, jan.kiszka@siemens.com,
	lersek@redhat.com, ehabkost@redhat.com, marcel.a@redhat.com,
	stefanha@redhat.com, chegu_vinod@hp.com, rth@twiddle.net,
	kwolf@redhat.com, s.priebe@profihost.ag, mreitz@redhat.com,
	vasilis.liaskovitis@profitbricks.com, pbonzini@redhat.com,
	afaerber@suse.de
Subject: Re: [Qemu-devel] [PATCH 23/35] acpi:piix4: make plug/unlug callbacks generic
Date: Mon, 7 Apr 2014 18:19:06 +0300	[thread overview]
Message-ID: <20140407151906.GA17264@redhat.com> (raw)
In-Reply-To: <20140407132530.GA16541@redhat.com>

On Mon, Apr 07, 2014 at 04:25:30PM +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 07, 2014 at 03:12:11PM +0200, Igor Mammedov wrote:
> > On Mon, 7 Apr 2014 15:07:15 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Mon, Apr 07, 2014 at 02:00:37PM +0200, Igor Mammedov wrote:
> > > > On Mon, 7 Apr 2014 14:32:41 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > On Fri, Apr 04, 2014 at 03:36:48PM +0200, Igor Mammedov wrote:
> > > > > > ... and report error if plugged in device is not supported.
> > > > > > Later generic callbacks will be used by memory hotplug.
> > > > > > 
> > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > 
> > > > > 
> > > > > OK in that case, how about teaching all hotplug callbacks about this?
> > > > > 
> > > > > There are two ATM:
> > > > > shpc_device_hotplug_cb
> > > > > pcie_cap_slot_hotplug_cb
> > > > > 
> > > > > Teach them both to fail gracefully if they get
> > > > > an object that is not a pci device.
> > > > > 
> > > > > Afterwards, simply iterate over all objects of type
> > > > > TYPE_HOTPLUG_HANDLER
> > > > > and look for one that will accept your object.
> > > > Then you would never know if any hotplug handler has actually
> > > > handled event.
> > > 
> > > Why not? Check the error.
> > > If no one accepts your object, return error to user.
> > > 
> > > > I think hotplug handler should return error if unsupported
> > > > device passed in rather than ignore it. It makes catching
> > > > wiring errors easier.
> > > 
> > > Absolutely.
> > > 
> > > > Dropping error so that we could not care which hotplug handler
> > > > should be notified, looks like a wrong direction and makes
> > > > system more fragile.
> > > 
> > > That's not what I was suggesting.
> > > 
> > > > It shouldn't be up to consumer to determine that event should
> > > > be routed to it, but rather by external routing that knows
> > > > what and when should be notified.
> > > 
> > > Yes. So
> > > 
> > > 	for each hotplug handler (&err)
> > > 		handler->plug(device, &err)
> > > 		if (!err)
> > > 			break;
> > here it would break on the first handler that doesn't support device
> > and tells so to caller.
> 
> This is pseudo-code, I really mean !err == no error reported.
> 
> > And there isn't any routing here, it just blindly broadcast to every
> > handler, regardless whether it's right or not.
> 
> Yes - handlers verify what they can support.
> 
> > If broadcast should be ever done than it probably should be a part of
> > DEVICE class and part of
> >   [Qemu-devel] [PATCH 08/35] qdev: hotplug for buss-less devices
> > patch and be generic to all devices.
> 
> Not sure what's suggested here.
> 
> > 
> > Andreas,
> >   since you care about QDEV
> >   do you have an opinion on ^^^ discussion?


Actually, there's one useful case that this would help address.
At the moment ACPI code has to poke at SHPC bridges and
modify their hotplug callbacks in order to get notified
of hotplug events.

This means we can't cleanly implement an option for guest to
disable ACPI and switch to native if supported,
like the ACPI spec allows.

If we change hotplug code to walk the tree top down
and invoke all callbacks, then it will be fixed
in a cleaner way: bridges would just do:

	if (dev->bus != self) {
		set_error
		return;
	}

and suddently pci host can trap callbacks and redirect
to acpi if it wants to.


> > > 
> > > 
> > > 	if (err)
> > > 		hotplug failed - destroy device
> > > 
> > > 
> > > > > 
> > > > > 
> > > > > > ---
> > > > > >  hw/acpi/piix4.c | 31 ++++++++++++++++++++++---------
> > > > > >  1 file changed, 22 insertions(+), 9 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > > > > index 67dc075..4341f82 100644
> > > > > > --- a/hw/acpi/piix4.c
> > > > > > +++ b/hw/acpi/piix4.c
> > > > > > @@ -310,19 +310,32 @@ static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
> > > > > >      acpi_pm1_evt_power_down(&s->ar);
> > > > > >  }
> > > > > >  
> > > > > > -static void piix4_pci_device_plug_cb(HotplugHandler *hotplug_dev,
> > > > > > -                                     DeviceState *dev, Error **errp)
> > > > > > +static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
> > > > > > +                                 DeviceState *dev, Error **errp)
> > > > > >  {
> > > > > >      PIIX4PMState *s = PIIX4_PM(hotplug_dev);
> > > > > > -    acpi_pcihp_device_plug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, dev, errp);
> > > > > > +
> > > > > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > > > > > +        acpi_pcihp_device_plug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, dev,
> > > > > > +                                  errp);
> > > > > > +    } else {
> > > > > > +        error_setg(errp, "acpi: device plug request for not supported device"
> > > > > > +                   " type: %s", object_get_typename(OBJECT(dev)));
> > > > > > +    }
> > > > > >  }
> > > > > >  
> > > > > > -static void piix4_pci_device_unplug_cb(HotplugHandler *hotplug_dev,
> > > > > > -                                       DeviceState *dev, Error **errp)
> > > > > > +static void piix4_device_unplug_cb(HotplugHandler *hotplug_dev,
> > > > > > +                                   DeviceState *dev, Error **errp)
> > > > > >  {
> > > > > >      PIIX4PMState *s = PIIX4_PM(hotplug_dev);
> > > > > > -    acpi_pcihp_device_unplug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, dev,
> > > > > > -                                errp);
> > > > > > +
> > > > > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > > > > > +        acpi_pcihp_device_unplug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, dev,
> > > > > > +                                    errp);
> > > > > > +    } else {
> > > > > > +        error_setg(errp, "acpi: device unplug request for not supported device"
> > > > > > +                   " type: %s", object_get_typename(OBJECT(dev)));
> > > > > > +    }
> > > > > >  }
> > > > > >  
> > > > > >  static void piix4_update_bus_hotplug(PCIBus *pci_bus, void *opaque)
> > > > > > @@ -553,8 +566,8 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
> > > > > >       */
> > > > > >      dc->cannot_instantiate_with_device_add_yet = true;
> > > > > >      dc->hotpluggable = false;
> > > > > > -    hc->plug = piix4_pci_device_plug_cb;
> > > > > > -    hc->unplug = piix4_pci_device_unplug_cb;
> > > > > > +    hc->plug = piix4_device_plug_cb;
> > > > > > +    hc->unplug = piix4_device_unplug_cb;
> > > > > >  }
> > > > > >  
> > > > > >  static const TypeInfo piix4_pm_info = {
> > > > > > -- 
> > > > > > 1.9.0

  parent reply	other threads:[~2014-04-07 15:19 UTC|newest]

Thread overview: 92+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-04-04 13:36 [Qemu-devel] [PATCH 00/35] pc: ACPI memory hotplug Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 01/35] qemu-option: introduce qemu_find_opts_singleton Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 02/35] vl: convert -m to QemuOpts Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 03/35] object_add: allow completion handler to get canonical path Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 04/35] add memdev backend infrastructure Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 05/35] vl.c: extend -m option to support options for memory hotplug Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 06/35] add pc-{i440fx,q35}-2.1 machine types Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 07/35] pc: create custom generic PC machine type Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 08/35] qdev: hotplug for buss-less devices Igor Mammedov
2014-04-07  2:26   ` Alexey Kardashevskiy
2014-04-07  6:51     ` Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 09/35] qdev: expose DeviceState.hotplugged field as a property Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 10/35] dimm: implement dimm device abstraction Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 11/35] memory: add memory_region_is_mapped() API Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 12/35] dimm: do not allow to set already busy memdev Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 13/35] pc: initialize memory hotplug address space Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 14/35] pc: exit QEMU if slots > 256 Igor Mammedov
2014-04-04 17:14   ` Eduardo Habkost
2014-04-07  6:55     ` Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 15/35] pc: add 'etc/reserved-memory-end' fw_cfg interface for SeaBIOS Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 16/35] pc: add memory hotplug handler to PC_MACHINE Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 17/35] dimm: add busy address check and address auto-allocation Igor Mammedov
2014-05-07  9:58   ` Tang Chen
2014-04-04 13:36 ` [Qemu-devel] [PATCH 18/35] dimm: add busy slot check and slot auto-allocation Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 19/35] acpi: rename cpu_hotplug_defs.h to acpi_defs.h Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 20/35] acpi: memory hotplug ACPI hardware implementation Igor Mammedov
2014-05-05 12:20   ` Vasilis Liaskovitis
2014-05-06  7:13     ` Igor Mammedov
2014-05-06 12:58       ` Vasilis Liaskovitis
2014-04-04 13:36 ` [Qemu-devel] [PATCH 21/35] trace: add acpi memory hotplug IO region events Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 22/35] trace: add DIMM slot & address allocation for target-i386 Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 23/35] acpi:piix4: make plug/unlug callbacks generic Igor Mammedov
2014-04-07 11:32   ` Michael S. Tsirkin
2014-04-07 12:00     ` Igor Mammedov
2014-04-07 12:07       ` Michael S. Tsirkin
2014-04-07 13:12         ` Igor Mammedov
2014-04-07 13:25           ` Michael S. Tsirkin
2014-04-07 14:22             ` Igor Mammedov
2014-04-07 15:36               ` Michael S. Tsirkin
2014-04-11  9:41                 ` Igor Mammedov
2014-04-07 15:19             ` Michael S. Tsirkin [this message]
2014-04-12  1:40               ` Paolo Bonzini
2014-04-04 13:36 ` [Qemu-devel] [PATCH 24/35] acpi:piix4: add memory hotplug handling Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 25/35] pc: ich9 lpc: make it work with global/compat properties Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 26/35] acpi:ich9: add memory hotplug handling Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 27/35] pc: migrate piix4 & ich9 MemHotplugState Igor Mammedov
2014-04-04 14:16   ` Paolo Bonzini
2014-04-04 14:37     ` Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 28/35] pc: propagate memory hotplug event to ACPI device Igor Mammedov
2014-04-04 14:02   ` Paolo Bonzini
2014-04-04 14:29     ` Igor Mammedov
2014-04-07  3:07   ` Alexey Kardashevskiy
2014-04-07 14:13     ` Eduardo Habkost
2014-04-07 14:26       ` Igor Mammedov
2014-04-07 15:21         ` Michael S. Tsirkin
2014-04-11  9:13           ` Igor Mammedov
2014-04-07 10:23   ` Michael S. Tsirkin
2014-04-07 13:21     ` Igor Mammedov
2014-04-07 14:32     ` Igor Mammedov
2014-04-07 15:14       ` Michael S. Tsirkin
2014-04-11  9:14         ` Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 29/35] pc: ACPI BIOS: punch holes in PCI0._CRS for memory hotplug IO region Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 30/35] pc: ACPI BIOS: name CPU hotplug ACPI0004 device Igor Mammedov
2014-04-06  9:18   ` Michael S. Tsirkin
2014-04-07  7:13     ` Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 31/35] pc: ACPI BIOS: implement memory hotplug interface Igor Mammedov
2014-04-06  9:13   ` Michael S. Tsirkin
2014-04-07  7:23     ` Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 32/35] pc: ACPI BIOS: use enum for defining memory affinity flags Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 33/35] pc: ACPI BIOS: reserve SRAT entry for hotplug mem hole Igor Mammedov
     [not found]   ` <20140414072501.GC10931@G08FNSTD100614.fnst.cn.fujitsu.com>
     [not found]     ` <20140414184442.6ff3e626@nial.usersys.redhat.com>
2014-05-05 15:59       ` Vasilis Liaskovitis
2014-05-06  1:52         ` Hu Tao
2014-05-06 13:00           ` Vasilis Liaskovitis
     [not found]             ` <CAM4NYE8WjH-AhEAv9h8Z14+g3XutfEDM8UYFHtDUF7iR4jAOUg@mail.gmail.com>
     [not found]               ` <20140528100722.01b59a48@nial.usersys.redhat.com>
     [not found]                 ` <20140528122312.GA4730@dhcp-192-168-178-175.profitbricks.localdomain>
     [not found]                   ` <20140528152642.108cb193@nial.usersys.redhat.com>
     [not found]                     ` <20140528163813.GB28017@dhcp-192-168-178-175.profitbricks.localdomain>
     [not found]                       ` <20140529111237.7d775371@nial.usersys.redhat.com>
2014-06-02 14:29                         ` Vasilis Liaskovitis
2014-06-02 14:54                           ` Igor Mammedov
2014-04-04 13:36 ` [Qemu-devel] [PATCH 34/35] pc: ACPI BIOS: make GPE.3 handle memory hotplug event on PIIX and Q35 machines Igor Mammedov
2014-05-05 16:25   ` Eric Blake
2014-05-05 16:28     ` Paolo Bonzini
2014-05-06 10:05       ` Laszlo Ersek
2014-05-06  7:16     ` Igor Mammedov
2014-04-04 13:37 ` [Qemu-devel] [PATCH 35/35] pc: ACPI BIOS: update pregenerated ACPI table blobs Igor Mammedov
     [not found] ` <533EBCB9.3040001@redhat.com>
2014-04-04 14:24   ` [Qemu-devel] [PATCH 00/35] pc: ACPI memory hotplug Igor Mammedov
2014-04-04 15:19     ` Paolo Bonzini
2014-04-04 15:37       ` Igor Mammedov
2014-04-04 16:57 ` Dr. David Alan Gilbert
2014-04-07  7:32   ` Igor Mammedov
2014-05-07  9:15 ` Stefan Priebe - Profihost AG
2014-05-19 21:24   ` [Qemu-devel] [PATCH] vl.c: daemonize before guest memory allocation Igor Mammedov
2014-05-19 21:28     ` Eric Blake
2014-05-19 21:35       ` Igor Mammedov
2014-08-25 13:28   ` [Qemu-devel] [PATCH 00/35] pc: ACPI memory hotplug Anshul Makkar
2014-08-25 13:35     ` Paolo Bonzini

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140407151906.GA17264@redhat.com \
    --to=mst@redhat.com \
    --cc=afaerber@suse.de \
    --cc=aik@ozlabs.ru \
    --cc=akong@redhat.com \
    --cc=aliguori@amazon.com \
    --cc=armbru@redhat.com \
    --cc=chegu_vinod@hp.com \
    --cc=ehabkost@redhat.com \
    --cc=hutao@cn.fujitsu.com \
    --cc=imammedo@redhat.com \
    --cc=jan.kiszka@siemens.com \
    --cc=kraxel@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=lcapitulino@redhat.com \
    --cc=lersek@redhat.com \
    --cc=marcel.a@redhat.com \
    --cc=mjt@tls.msk.ru \
    --cc=mreitz@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-devel@nongnu.org \
    --cc=quintela@redhat.com \
    --cc=rth@twiddle.net \
    --cc=s.priebe@profihost.ag \
    --cc=stefanha@redhat.com \
    --cc=vasilis.liaskovitis@profitbricks.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).