From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44517) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vkvwm-0004oD-3g for qemu-devel@nongnu.org; Mon, 25 Nov 2013 08:11:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vkvwd-0000vZ-LY for qemu-devel@nongnu.org; Mon, 25 Nov 2013 08:11:28 -0500 Received: from mail-qe0-x22d.google.com ([2607:f8b0:400d:c02::22d]:64673) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vkvwd-0000vR-GG for qemu-devel@nongnu.org; Mon, 25 Nov 2013 08:11:19 -0500 Received: by mail-qe0-f45.google.com with SMTP id 6so3793827qea.32 for ; Mon, 25 Nov 2013 05:11:19 -0800 (PST) Sender: Paolo Bonzini Message-ID: <52934C6F.7020805@redhat.com> Date: Mon, 25 Nov 2013 14:11:11 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1385001528-12003-1-git-send-email-imammedo@redhat.com> <1385001528-12003-3-git-send-email-imammedo@redhat.com> <52934759.3020204@redhat.com> In-Reply-To: <52934759.3020204@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 02/27] rename pci_hotplug_fn to hotplug_fn and make it available for other devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: peter.maydell@linaro.org, aliguori@amazon.com, mst@redhat.com, qemu-devel@nongnu.org, hutao@cn.fujitsu.com, stefanb@linux.vnet.ibm.com, mjt@tls.msk.ru, mdroth@linux.vnet.ibm.com, armbru@redhat.com, vasilis.liaskovitis@profitbricks.com, quintela@redhat.com, kraxel@redhat.com, stefanha@redhat.com, Igor Mammedov , marcel.a@redhat.com, lcapitulino@redhat.com, chegu_vinod@hp.com, afaerber@suse.de Il 25/11/2013 13:49, Paolo Bonzini ha scritto: > Il 21/11/2013 03:38, Igor Mammedov ha scritto: >> +typedef enum { >> + HOTPLUG_DISABLED, >> + HOTPLUG_ENABLED, >> + COLDPLUG_ENABLED, >> +} HotplugState; >> + >> +typedef int (*hotplug_fn)(DeviceState *hotplug_dev, DeviceState *dev, >> + HotplugState state); > > I don't think this is a particularly useful interface, because it > reinvents a lot of what already exists in qdev/qbus. > > The cold/hotplug_enabled differentiation is not particularly useful > when dev->hotplugged already exists, and the interface leaves one to > wonder why COLDPLUG_ENABLED doesn't have a matching COLDPLUG_DISABLED > (until one looks at the code). > > Take for example this: > > static void enable_device(PIIX4PMState *s, int slot) > { > s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS; > s->pci0_slot_device_present |= (1U << slot); > } > > static void disable_device(PIIX4PMState *s, int slot) > { > s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS; > s->pci0_status.down |= (1U << slot); > } > > static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, > PCIHotplugState state) > { > int slot = PCI_SLOT(dev->devfn); > PIIX4PMState *s = PIIX4_PM(qdev); > > /* Don't send event when device is enabled during qemu machine creation: > * it is present on boot, no hotplug event is necessary. We do send an > * event when the device is disabled later. */ > if (state == PCI_COLDPLUG_ENABLED) { > s->pci0_slot_device_present |= (1U << slot); > return 0; > } > > if (state == PCI_HOTPLUG_ENABLED) { > enable_device(s, slot); > } else { > disable_device(s, slot); > } > > pm_update_sci(s); > > return 0; > } > > > In my opinion, it is much clearer this way---separating enable/disabled > rather than hotplug/coldplug: > > static void piix4_pci_send_event(PIIX4PMState *s) > { > s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS; > pm_update_sci(s); > } > > static int piix4_device_hotplug(DeviceState *qdev, DeviceState *dev) > { > PCIDevice *pci = PCI_DEVICE(dev); > int slot = PCI_SLOT(pci->devfn); > PIIX4PMState *s = PIIX4_PM(qdev); > > s->pci0_slot_device_present |= (1U << slot); > /* Don't send event when device is enabled during qemu machine creation: > * it is present on boot, no hotplug event is necessary. We do send an > * event when the device is disabled later. */ > if (dev->hotplugged) { > piix4_pci_send_event(s); > } > return 0; > } > > static int piix4_device_hot_unplug(DeviceState *qdev, DeviceState *dev) > { > PCIDevice *pci = PCI_DEVICE(dev); > int slot = PCI_SLOT(pci->devfn); > PIIX4PMState *s = PIIX4_PM(qdev); > > s->pci0_status.down |= (1U << slot); > piix4_pci_send_event(s); > return 0; > } > > This is of course just an example, I'm not asking you to reimplement hotplug > for all buses in QEMU. However, my point is that this shouldn't be a "core" > enum and interface. If you want to have a core interface in BusClass, I'd > rather add ->hotplug and ->hot_unplug to BusClass, similar to how the SCSI > bus does it (note: I only maintain it, I didn't write that code) and so > that BusClass's methods match ->init/->exit on the device side. I reviewed the user now. I understand why you want to reuse the same idiom. Perhaps you can move this to include/hw/hotplug.h instead? I would still prefer DimmBus or BusClass to use the more common hotplug/hot_unplug interface, but ACPIHotpluggableDimmBus can map it to an hotplug_fn. Paolo