From: Marcel Apfelbaum <marcel@redhat.com>
To: Laszlo Ersek <lersek@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>
Cc: qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v6 3/7] hw/pci-bridge: introduce "hotplug" property
Date: Wed, 17 Jun 2015 22:54:50 +0300 [thread overview]
Message-ID: <5581D08A.90701@redhat.com> (raw)
In-Reply-To: <558180F0.7040704@redhat.com>
On 06/17/2015 05:15 PM, Laszlo Ersek wrote:
> On 06/17/15 16:02, Michael S. Tsirkin wrote:
>> On Wed, Jun 17, 2015 at 03:55:32PM +0200, Laszlo Ersek wrote:
>>> On 06/17/15 15:42, Michael S. Tsirkin wrote:
>>>> On Wed, Jun 17, 2015 at 02:45:01PM +0200, Laszlo Ersek wrote:
>>>>> In the PCI expander bridge, we will want to disable those features of
>>>>> pci-bridge that relate to SHPC (standard hotplug controller):
>>>>>
>>>>> - SHPC bar and underlying MemoryRegion
>>>>> - interrupt (INTx or MSI)
>>>>> - effective hotplug callbacks
>>>>> - other SHPC hooks (initialization, cleanup, migration etc)
>>>>>
>>>>> Introduce a new feature bit in the PCIBridgeDev.flags field, and turn off
>>>>> the above if the bit is explicitly cleared.
>>>>>
>>>>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>>> Cc: Marcel Apfelbaum <marcel@redhat.com>
>>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>>
>>>>
>>>> I think I would prefer naming the property "shpc".
>>>> In particular ACPI hotplug still works fine.
>>>
>>> Will do.
>>>
>>> Are you okay with the flag's name, PCI_BRIDGE_DEV_F_HOTPLUG? If not,
>>> what would be your preference?
>>
>> PCI_BRIDGE_DEV_F_SHPC_REQ
>>
>> Also add macro for "shpc" property string to avoid duplication.
>> Also clear msi explicitly - no need for that to depend on shpc.
>>
>>> Should I also rename the helper function hotplug_enabled() to
>>> shpc_enabled()?
>>
>> It should be tested only once - before shpc is initialized.
>> All other users should test QEMU_PCI_CAP_SHPC - if you like,
>> using a helper shpc_present.
>
> I'll make one attempt (in v7) to get this right. If I miss it, I'll ask
> Marcel to please take over the series.
No problem here,
I'll take it from V8 if necessary, but I think it will be just fine. :)
Thanks,
Marcel
>
> Thanks
> Laszlo
>
>>
>>>
>>> Thanks
>>> Laszlo
>>>
>>>>
>>>>
>>>>> ---
>>>>>
>>>>> Notes:
>>>>> v6:
>>>>> - new in v6 (replaces "hw/pci-bridge: create interrupt-less,
>>>>> hotplug-less bridge for PXB" from v5) [Michael]
>>>>> - I tested migration-to-file and migration-from-file, with a single PXB
>>>>> behind which a virtio-scsi HBA lived
>>>>> - No idea how to test the hotplug callbacks. I tried the device_add and
>>>>> device_del HMP commands with a virtio-scsi HBA behind the PXB, and the
>>>>> monitor reports no error at all; the callbacks don't seem to be
>>>>> reached.
>>>>> - Retested with windows 2012; it works
>>>>>
>>>>> hw/pci-bridge/pci_bridge_dev.c | 90 ++++++++++++++++++++++++++++++++++++------
>>>>> 1 file changed, 78 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
>>>>> index d697c7b..a0a1ce9 100644
>>>>> --- a/hw/pci-bridge/pci_bridge_dev.c
>>>>> +++ b/hw/pci-bridge/pci_bridge_dev.c
>>>>> @@ -40,10 +40,16 @@ struct PCIBridgeDev {
>>>>> MemoryRegion bar;
>>>>> uint8_t chassis_nr;
>>>>> #define PCI_BRIDGE_DEV_F_MSI_REQ 0
>>>>> +#define PCI_BRIDGE_DEV_F_HOTPLUG 1
>>>>> uint32_t flags;
>>>>> };
>>>>> typedef struct PCIBridgeDev PCIBridgeDev;
>>>>>
>>>>> +static inline bool hotplug_enabled(const PCIBridgeDev *bridge_dev)
>>>>> +{
>>>>> + return !!(bridge_dev->flags & (1u << PCI_BRIDGE_DEV_F_HOTPLUG));
>>>>> +}
>>>>> +
>>>>> static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>>> {
>>>>> PCIBridge *br = PCI_BRIDGE(dev);
>>>>> @@ -54,16 +60,26 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>>> if (err) {
>>>>> goto bridge_error;
>>>>> }
>>>>> - dev->config[PCI_INTERRUPT_PIN] = 0x1;
>>>>> - memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar", shpc_bar_size(dev));
>>>>> - err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
>>>>> - if (err) {
>>>>> - goto shpc_error;
>>>>> +
>>>>> + if (hotplug_enabled(bridge_dev)) {
>>>>> + dev->config[PCI_INTERRUPT_PIN] = 0x1;
>>>>> + memory_region_init(&bridge_dev->bar, OBJECT(dev), "shpc-bar",
>>>>> + shpc_bar_size(dev));
>>>>> + err = shpc_init(dev, &br->sec_bus, &bridge_dev->bar, 0);
>>>>> + if (err) {
>>>>> + goto shpc_error;
>>>>> + }
>>>>> }
>>>>> +
>>>>> err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0);
>>>>> if (err) {
>>>>> goto slotid_error;
>>>>> }
>>>>> +
>>>>> + if (!hotplug_enabled(bridge_dev)) {
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> if ((bridge_dev->flags & (1 << PCI_BRIDGE_DEV_F_MSI_REQ)) &&
>>>>> msi_supported) {
>>>>> err = msi_init(dev, 0, 1, true, true);
>>>>> @@ -79,7 +95,9 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
>>>>> msi_error:
>>>>> slotid_cap_cleanup(dev);
>>>>> slotid_error:
>>>>> - shpc_cleanup(dev, &bridge_dev->bar);
>>>>> + if (hotplug_enabled(bridge_dev)) {
>>>>> + shpc_cleanup(dev, &bridge_dev->bar);
>>>>> + }
>>>>> shpc_error:
>>>>> pci_bridge_exitfn(dev);
>>>>> bridge_error:
>>>>> @@ -89,53 +107,101 @@ bridge_error:
>>>>> static void pci_bridge_dev_exitfn(PCIDevice *dev)
>>>>> {
>>>>> PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
>>>>> +
>>>>> if (msi_present(dev)) {
>>>>> msi_uninit(dev);
>>>>> }
>>>>> slotid_cap_cleanup(dev);
>>>>> - shpc_cleanup(dev, &bridge_dev->bar);
>>>>> + if (hotplug_enabled(bridge_dev)) {
>>>>> + shpc_cleanup(dev, &bridge_dev->bar);
>>>>> + }
>>>>> pci_bridge_exitfn(dev);
>>>>> }
>>>>>
>>>>> static void pci_bridge_dev_instance_finalize(Object *obj)
>>>>> {
>>>>> + /* this function is idempotent and handles (PCIDevice.shpc == NULL) */
>>>>> shpc_free(PCI_DEVICE(obj));
>>>>> }
>>>>>
>>>>> static void pci_bridge_dev_write_config(PCIDevice *d,
>>>>> uint32_t address, uint32_t val, int len)
>>>>> {
>>>>> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(d);
>>>>> +
>>>>> pci_bridge_write_config(d, address, val, len);
>>>>> if (msi_present(d)) {
>>>>> msi_write_config(d, address, val, len);
>>>>> }
>>>>> - shpc_cap_write_config(d, address, val, len);
>>>>> + if (hotplug_enabled(bridge_dev)) {
>>>>> + shpc_cap_write_config(d, address, val, len);
>>>>> + }
>>>>> }
>>>>>
>>>>> static void qdev_pci_bridge_dev_reset(DeviceState *qdev)
>>>>> {
>>>>> PCIDevice *dev = PCI_DEVICE(qdev);
>>>>> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
>>>>>
>>>>> pci_bridge_reset(qdev);
>>>>> - shpc_reset(dev);
>>>>> + if (hotplug_enabled(bridge_dev)) {
>>>>> + shpc_reset(dev);
>>>>> + }
>>>>> }
>>>>>
>>>>> static Property pci_bridge_dev_properties[] = {
>>>>> /* Note: 0 is not a legal chassis number. */
>>>>> DEFINE_PROP_UINT8("chassis_nr", PCIBridgeDev, chassis_nr, 0),
>>>>> DEFINE_PROP_BIT("msi", PCIBridgeDev, flags, PCI_BRIDGE_DEV_F_MSI_REQ, true),
>>>>> + DEFINE_PROP_BIT("hotplug", PCIBridgeDev, flags, PCI_BRIDGE_DEV_F_HOTPLUG,
>>>>> + true),
>>>>> DEFINE_PROP_END_OF_LIST(),
>>>>> };
>>>>>
>>>>> +static bool pci_bridge_dev_shpc_exists(void *opaque, int version_id)
>>>>> +{
>>>>> + PCIDevice *dev = opaque;
>>>>> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev);
>>>>> +
>>>>> + return hotplug_enabled(bridge_dev);
>>>>> +}
>>>>> +
>>>>> static const VMStateDescription pci_bridge_dev_vmstate = {
>>>>> .name = "pci_bridge",
>>>>> .fields = (VMStateField[]) {
>>>>> VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
>>>>> - SHPC_VMSTATE(shpc, PCIDevice, NULL),
>>>>> + SHPC_VMSTATE(shpc, PCIDevice, pci_bridge_dev_shpc_exists),
>>>>> VMSTATE_END_OF_LIST()
>>>>> }
>>>>> };
>>>>>
>>>>> +static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev,
>>>>> + DeviceState *dev, Error **errp)
>>>>> +{
>>>>> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(hotplug_dev);
>>>>> +
>>>>> + if (!hotplug_enabled(bridge_dev)) {
>>>>> + error_setg(errp, "standard hotplug controller has been disabled for "
>>>>> + "this %s", TYPE_PCI_BRIDGE_DEV);
>>>>> + return;
>>>>> + }
>>>>> + shpc_device_hotplug_cb(hotplug_dev, dev, errp);
>>>>> +}
>>>>> +
>>>>> +static void pci_bridge_dev_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
>>>>> + DeviceState *dev,
>>>>> + Error **errp)
>>>>> +{
>>>>> + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(hotplug_dev);
>>>>> +
>>>>> + if (!hotplug_enabled(bridge_dev)) {
>>>>> + error_setg(errp, "standard hotplug controller has been disabled for "
>>>>> + "this %s", TYPE_PCI_BRIDGE_DEV);
>>>>> + return;
>>>>> + }
>>>>> + shpc_device_hot_unplug_request_cb(hotplug_dev, dev, errp);
>>>>> +}
>>>>> +
>>>>> static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
>>>>> {
>>>>> DeviceClass *dc = DEVICE_CLASS(klass);
>>>>> @@ -154,8 +220,8 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
>>>>> dc->props = pci_bridge_dev_properties;
>>>>> dc->vmsd = &pci_bridge_dev_vmstate;
>>>>> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>>>>> - hc->plug = shpc_device_hotplug_cb;
>>>>> - hc->unplug_request = shpc_device_hot_unplug_request_cb;
>>>>> + hc->plug = pci_bridge_dev_hotplug_cb;
>>>>> + hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb;
>>>>> }
>>>>>
>>>>> static const TypeInfo pci_bridge_dev_info = {
>>>>> --
>>>>> 1.8.3.1
>>>>>
>
next prev parent reply other threads:[~2015-06-17 19:54 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-17 12:44 [Qemu-devel] [PATCH v6 0/7] PXB changes Laszlo Ersek
2015-06-17 12:44 ` [Qemu-devel] [PATCH v6 1/7] migration: introduce VMSTATE_BUFFER_UNSAFE_INFO_TEST() Laszlo Ersek
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 2/7] hw/pci-bridge: expose _test parameter in SHPC_VMSTATE() Laszlo Ersek
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 3/7] hw/pci-bridge: introduce "hotplug" property Laszlo Ersek
2015-06-17 13:42 ` Michael S. Tsirkin
2015-06-17 13:55 ` Laszlo Ersek
2015-06-17 14:02 ` Michael S. Tsirkin
2015-06-17 14:15 ` Laszlo Ersek
2015-06-17 19:54 ` Marcel Apfelbaum [this message]
2015-06-18 13:47 ` Paolo Bonzini
2015-06-18 14:44 ` Michael S. Tsirkin
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 4/7] hw/pci-bridge: disable hotplug in PXB Laszlo Ersek
2015-06-17 13:45 ` Michael S. Tsirkin
2015-06-17 19:52 ` Marcel Apfelbaum
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 5/7] hw/core: rebase sysbus_get_fw_dev_path() to g_strdup_printf() Laszlo Ersek
2015-06-17 13:46 ` Michael S. Tsirkin
2015-06-17 13:56 ` Laszlo Ersek
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 6/7] hw/core: explicit OFW unit address callback for SysBusDeviceClass Laszlo Ersek
2015-06-17 12:45 ` [Qemu-devel] [PATCH v6 7/7] hw/pci-bridge: format SeaBIOS-compliant OFW device node for PXB Laszlo Ersek
2015-06-17 13:57 ` Michael S. Tsirkin
2015-06-17 14:00 ` Laszlo Ersek
2015-06-17 14:03 ` Michael S. Tsirkin
2015-06-17 14:18 ` Kevin O'Connor
2015-06-17 14:45 ` Laszlo Ersek
2015-06-17 15:05 ` Michael S. Tsirkin
2015-06-17 18:16 ` Laszlo Ersek
2015-06-17 18:54 ` Michael S. Tsirkin
2015-06-17 19:15 ` Laszlo Ersek
2015-06-17 19:28 ` Kevin O'Connor
2015-06-17 19:32 ` Michael S. Tsirkin
2015-06-17 19:44 ` Laszlo Ersek
2015-06-17 21:50 ` Michael S. Tsirkin
2015-06-18 13:22 ` Laszlo Ersek
2015-06-18 13:40 ` Michael S. Tsirkin
2015-06-18 15:42 ` Laszlo Ersek
2015-06-17 19:09 ` Kevin O'Connor
2015-06-17 19:21 ` Michael S. Tsirkin
2015-06-17 19:35 ` Laszlo Ersek
2015-06-17 21:49 ` Michael S. Tsirkin
2015-06-18 13:18 ` Laszlo Ersek
2015-06-17 19:38 ` Kevin O'Connor
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=5581D08A.90701@redhat.com \
--to=marcel@redhat.com \
--cc=lersek@redhat.com \
--cc=mst@redhat.com \
--cc=qemu-devel@nongnu.org \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.