[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 02/27] rename pci_hotplug_fn to hotplug_fn and m
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH 02/27] rename pci_hotplug_fn to hotplug_fn and make it available for other devices |
Date: |
Mon, 25 Nov 2013 13:49:29 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130923 Thunderbird/17.0.9 |
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.
Paolo
- Re: [Qemu-devel] [PATCH 04/27] vl: convert -m to qemu_opts_parse(), (continued)
- Re: [Qemu-devel] [PATCH 04/27] vl: convert -m to qemu_opts_parse(), Markus Armbruster, 2013/11/21
- Re: [Qemu-devel] [PATCH 04/27] vl: convert -m to qemu_opts_parse(), Igor Mammedov, 2013/11/26
- Re: [Qemu-devel] [PATCH 04/27] vl: convert -m to qemu_opts_parse(), Markus Armbruster, 2013/11/26
- Re: [Qemu-devel] [PATCH 04/27] vl: convert -m to qemu_opts_parse(), Igor Mammedov, 2013/11/26
- [Qemu-devel] [PATCH 04/28] vl: convert -m to QemuOpts, Igor Mammedov, 2013/11/26
- [Qemu-devel] [PATCH 05/28] vl.c: extend -m option to support options for memory hotplug, Igor Mammedov, 2013/11/26
Re: [Qemu-devel] [PATCH 04/27] vl: convert -m to qemu_opts_parse(), Paolo Bonzini, 2013/11/25
[Qemu-devel] [PATCH 02/27] rename pci_hotplug_fn to hotplug_fn and make it available for other devices, Igor Mammedov, 2013/11/20
- Re: [Qemu-devel] [PATCH 02/27] rename pci_hotplug_fn to hotplug_fn and make it available for other devices,
Paolo Bonzini <=
[Qemu-devel] [PATCH 05/27] qapi: add SIZE type parser to string_input_visitor, Igor Mammedov, 2013/11/20
- Re: [Qemu-devel] [PATCH 05/27] qapi: add SIZE type parser to string_input_visitor, Markus Armbruster, 2013/11/21
- Re: [Qemu-devel] [PATCH 05/27] qapi: add SIZE type parser to string_input_visitor, Igor Mammedov, 2013/11/25
- Re: [Qemu-devel] [PATCH 05/27] qapi: add SIZE type parser to string_input_visitor, Michael S. Tsirkin, 2013/11/25
- Re: [Qemu-devel] [PATCH 05/27] qapi: add SIZE type parser to string_input_visitor, Paolo Bonzini, 2013/11/25
- Re: [Qemu-devel] [PATCH 05/27] qapi: add SIZE type parser to string_input_visitor, Eric Blake, 2013/11/25
- Re: [Qemu-devel] [PATCH 05/27] qapi: add SIZE type parser to string_input_visitor, Paolo Bonzini, 2013/11/25
- Re: [Qemu-devel] [PATCH 05/27] qapi: add SIZE type parser to string_input_visitor, Markus Armbruster, 2013/11/27