qemu-devel
[Top][All Lists]
Advanced

[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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]