* Julia Suvorova (jus...@redhat.com) wrote: > On Mon, Jan 13, 2020 at 3:04 PM Yury Kotov <yury-ko...@yandex-team.ru> wrote: > > > > Devices hot-plug during migration is not allowed and disabled in > > corresponding QMP-commands (device_add, device_del). > > But guest still can unplug device by powering it off > > (Example: echo 0 > /sys/bus/pci/slots/XXX/power). > > You don't want to unplug device due to powering the slot off in the > first place. Instead, you can hide the device (see f3a8505656), and > make it visible again when the slot is powered on. Thus, the guest > will not be able to unplug the device and your problem will disappear. > > Best regards, Julia Suvorova.
I don't really understand how hidden a hidden device is; will it still get it's migration data migrated? - if so what state is it in? What gets migrated? is the behaviour after reset/reboot consistent with what we want in the existing unplug behaviour? Say it's a disk device; will it release the lock on the disk backend? It feels like the semantics need to be tied down a bit. Dave > > Fix it by deferring unplugging until the migration is complete. > > > > Signed-off-by: Yury Kotov <yury-ko...@yandex-team.ru> > > --- > > hw/pci-bridge/gen_pcie_root_port.c | 7 ++++ > > hw/pci-bridge/ioh3420.c | 7 ++++ > > hw/pci-bridge/xio3130_downstream.c | 7 ++++ > > hw/pci/pcie.c | 54 +++++++++++++++++++++++------- > > hw/pci/pcie_port.c | 47 ++++++++++++++++++++++++++ > > include/hw/pci/pcie.h | 1 + > > include/hw/pci/pcie_port.h | 20 +++++++++++ > > 7 files changed, 130 insertions(+), 13 deletions(-) > > > > diff --git a/hw/pci-bridge/gen_pcie_root_port.c > > b/hw/pci-bridge/gen_pcie_root_port.c > > index 9eaefebca8..5b3c202341 100644 > > --- a/hw/pci-bridge/gen_pcie_root_port.c > > +++ b/hw/pci-bridge/gen_pcie_root_port.c > > @@ -100,6 +100,9 @@ static void gen_rp_realize(DeviceState *dev, Error > > **errp) > > } > > } > > > > +static const VMStateDescription vmstate_rp_deffered_unplug = > > + VMSTATE_DEFFERED_UNPLUG("pcie-root-port"); > > + > > static const VMStateDescription vmstate_rp_dev = { > > .name = "pcie-root-port", > > .priority = MIG_PRI_PCI_BUS, > > @@ -114,6 +117,10 @@ static const VMStateDescription vmstate_rp_dev = { > > GenPCIERootPort, > > gen_rp_test_migrate_msix), > > VMSTATE_END_OF_LIST() > > + }, > > + .subsections = (const VMStateDescription * []) { > > + &vmstate_rp_deffered_unplug, > > + NULL > > } > > }; > > > > diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c > > index f1e16135a3..2399a9a87f 100644 > > --- a/hw/pci-bridge/ioh3420.c > > +++ b/hw/pci-bridge/ioh3420.c > > @@ -82,6 +82,9 @@ static void ioh3420_interrupts_uninit(PCIDevice *d) > > msi_uninit(d); > > } > > > > +static const VMStateDescription vmstate_ioh3420_deffered_unplug = > > + VMSTATE_DEFFERED_UNPLUG("ioh-3240-express-root-port"); > > + > > static const VMStateDescription vmstate_ioh3420 = { > > .name = "ioh-3240-express-root-port", > > .priority = MIG_PRI_PCI_BUS, > > @@ -93,6 +96,10 @@ static const VMStateDescription vmstate_ioh3420 = { > > VMSTATE_STRUCT(parent_obj.parent_obj.parent_obj.exp.aer_log, > > PCIESlot, 0, vmstate_pcie_aer_log, PCIEAERLog), > > VMSTATE_END_OF_LIST() > > + }, > > + .subsections = (const VMStateDescription * []) { > > + &vmstate_ioh3420_deffered_unplug, > > + NULL > > } > > }; > > > > diff --git a/hw/pci-bridge/xio3130_downstream.c > > b/hw/pci-bridge/xio3130_downstream.c > > index a9f084b863..a5b4fe08ee 100644 > > --- a/hw/pci-bridge/xio3130_downstream.c > > +++ b/hw/pci-bridge/xio3130_downstream.c > > @@ -139,6 +139,9 @@ static Property xio3130_downstream_props[] = { > > DEFINE_PROP_END_OF_LIST() > > }; > > > > +static const VMStateDescription vmstate_xio3130_downstream_deffered_unplug > > = > > + VMSTATE_DEFFERED_UNPLUG("xio3130-express-downstream-port"); > > + > > static const VMStateDescription vmstate_xio3130_downstream = { > > .name = "xio3130-express-downstream-port", > > .priority = MIG_PRI_PCI_BUS, > > @@ -150,6 +153,10 @@ static const VMStateDescription > > vmstate_xio3130_downstream = { > > VMSTATE_STRUCT(parent_obj.parent_obj.parent_obj.exp.aer_log, > > PCIESlot, 0, vmstate_pcie_aer_log, PCIEAERLog), > > VMSTATE_END_OF_LIST() > > + }, > > + .subsections = (const VMStateDescription * []) { > > + &vmstate_xio3130_downstream_deffered_unplug, > > + NULL > > } > > }; > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > index 08718188bb..29f0e5c05b 100644 > > --- a/hw/pci/pcie.c > > +++ b/hw/pci/pcie.c > > @@ -28,6 +28,8 @@ > > #include "hw/pci/pcie_regs.h" > > #include "hw/pci/pcie_port.h" > > #include "qemu/range.h" > > +#include "sysemu/sysemu.h" > > +#include "migration/misc.h" > > > > //#define DEBUG_PCIE > > #ifdef DEBUG_PCIE > > @@ -575,6 +577,7 @@ void pcie_cap_slot_reset(PCIDevice *dev) > > > > if (dev->cap_present & QEMU_PCIE_SLTCAP_PCP) { > > /* Downstream ports enforce device number 0. */ > > + PCIESlot *slot = PCIE_SLOT(dev); > > bool populated = > > pci_bridge_get_sec_bus(PCI_BRIDGE(dev))->devices[0]; > > uint16_t pic; > > > > @@ -588,6 +591,7 @@ void pcie_cap_slot_reset(PCIDevice *dev) > > > > pic = populated ? PCI_EXP_SLTCTL_PIC_ON : PCI_EXP_SLTCTL_PIC_OFF; > > pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL, pic); > > + slot->unplug_is_deferred = false; > > } > > > > pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > > @@ -608,13 +612,42 @@ void pcie_cap_slot_get(PCIDevice *dev, uint16_t > > *slt_ctl, uint16_t *slt_sta) > > *slt_sta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); > > } > > > > +static void pcie_cap_slot_unplug(PCIDevice *dev) > > +{ > > + uint32_t pos = dev->exp.exp_cap; > > + uint8_t *exp_cap = dev->config + pos; > > + PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev)); > > + > > + pci_for_each_device(sec_bus, pci_bus_num(sec_bus), > > + pcie_unplug_device, NULL); > > + pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > > PCI_EXP_SLTSTA_PDS); > > + if (dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA) { > > + pci_word_test_and_clear_mask(exp_cap + PCI_EXP_LNKSTA, > > + PCI_EXP_LNKSTA_DLLLA); > > + } > > + pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, > > PCI_EXP_SLTSTA_PDC); > > + hotplug_event_notify(dev); > > +} > > + > > +void pcie_cap_slot_deferred_unplug(PCIDevice *dev) > > +{ > > + PCIESlot *slot = PCIE_SLOT(dev); > > + > > + if (migration_is_idle() && slot->unplug_is_deferred) { > > + pcie_cap_slot_unplug(dev); > > + slot->unplug_is_deferred = false; > > + } > > +} > > + > > void pcie_cap_slot_write_config(PCIDevice *dev, > > uint16_t old_slt_ctl, uint16_t old_slt_sta, > > uint32_t addr, uint32_t val, int len) > > { > > + PCIESlot *slot = PCIE_SLOT(dev); > > uint32_t pos = dev->exp.exp_cap; > > uint8_t *exp_cap = dev->config + pos; > > uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); > > + bool may_unplug; > > > > if (ranges_overlap(addr, len, pos + PCI_EXP_SLTSTA, 2)) { > > /* > > @@ -660,22 +693,17 @@ void pcie_cap_slot_write_config(PCIDevice *dev, > > * this is a work around for guests that overwrite > > * control of powered off slots before powering them on. > > */ > > - if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && > > - (val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF && > > + may_unplug = (val & PCI_EXP_SLTCTL_PCC) && > > + (val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF; > > + if (may_unplug && (sltsta & PCI_EXP_SLTSTA_PDS) && > > (!(old_slt_ctl & PCI_EXP_SLTCTL_PCC) || > > (old_slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF)) > > { > > - PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev)); > > - pci_for_each_device(sec_bus, pci_bus_num(sec_bus), > > - pcie_unplug_device, NULL); > > - > > - pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA, > > - PCI_EXP_SLTSTA_PDS); > > - if (dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA) { > > - pci_word_test_and_clear_mask(exp_cap + PCI_EXP_LNKSTA, > > - PCI_EXP_LNKSTA_DLLLA); > > + slot->unplug_is_deferred = !migration_is_idle(); > > + if (!slot->unplug_is_deferred) { > > + pcie_cap_slot_unplug(dev); > > } > > - pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, > > - PCI_EXP_SLTSTA_PDC); > > + } else if (!may_unplug) { > > + slot->unplug_is_deferred = false; > > } > > > > hotplug_event_notify(dev); > > diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c > > index c19a9be592..bd5fbf6827 100644 > > --- a/hw/pci/pcie_port.c > > +++ b/hw/pci/pcie_port.c > > @@ -23,6 +23,9 @@ > > #include "hw/qdev-properties.h" > > #include "qemu/module.h" > > #include "hw/hotplug.h" > > +#include "sysemu/runstate.h" > > +#include "migration/migration.h" > > +#include "migration/misc.h" > > > > void pcie_port_init_reg(PCIDevice *d) > > { > > @@ -150,6 +153,48 @@ static Property pcie_slot_props[] = { > > DEFINE_PROP_END_OF_LIST() > > }; > > > > +bool vmstate_deffered_unplug_needed(void *opaque) > > +{ > > + PCIESlot *slot = opaque; > > + > > + return slot->unplug_is_deferred; > > +} > > + > > +static void pcie_slot_migration_notifier_cb(Notifier *notifier, void *data) > > +{ > > + PCIESlot *slot = container_of(notifier, PCIESlot, migration_notifier); > > + > > + pcie_cap_slot_deferred_unplug(PCI_DEVICE(slot)); > > +} > > + > > +static void pcie_slot_vm_state_change(void *opaque, int running, RunState > > state) > > +{ > > + PCIESlot *slot = opaque; > > + > > + pcie_cap_slot_deferred_unplug(PCI_DEVICE(slot)); > > +} > > + > > +static void pcie_slot_init(Object *obj) > > +{ > > + PCIESlot *slot = PCIE_SLOT(obj); > > + > > + slot->unplug_is_deferred = false; > > + slot->migration_notifier = (Notifier) { > > + .notify = pcie_slot_migration_notifier_cb > > + }; > > + add_migration_state_change_notifier(&slot->migration_notifier); > > + slot->vmstate_change = > > + qemu_add_vm_change_state_handler(pcie_slot_vm_state_change, slot); > > +} > > + > > +static void pcie_slot_finalize(Object *obj) > > +{ > > + PCIESlot *slot = PCIE_SLOT(obj); > > + > > + remove_migration_state_change_notifier(&slot->migration_notifier); > > + qemu_del_vm_change_state_handler(slot->vmstate_change); > > +} > > + > > static void pcie_slot_class_init(ObjectClass *oc, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(oc); > > @@ -166,6 +211,8 @@ static const TypeInfo pcie_slot_type_info = { > > .name = TYPE_PCIE_SLOT, > > .parent = TYPE_PCIE_PORT, > > .instance_size = sizeof(PCIESlot), > > + .instance_init = pcie_slot_init, > > + .instance_finalize = pcie_slot_finalize, > > .abstract = true, > > .class_init = pcie_slot_class_init, > > .interfaces = (InterfaceInfo[]) { > > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h > > index 7064875835..128f26199e 100644 > > --- a/include/hw/pci/pcie.h > > +++ b/include/hw/pci/pcie.h > > @@ -110,6 +110,7 @@ void pcie_cap_slot_get(PCIDevice *dev, uint16_t > > *slt_ctl, uint16_t *slt_sta); > > void pcie_cap_slot_write_config(PCIDevice *dev, > > uint16_t old_slt_ctl, uint16_t old_slt_sta, > > uint32_t addr, uint32_t val, int len); > > +void pcie_cap_slot_deferred_unplug(PCIDevice *dev); > > int pcie_cap_slot_post_load(void *opaque, int version_id); > > void pcie_cap_slot_push_attention_button(PCIDevice *dev); > > > > diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h > > index 7515430087..32e45f0c89 100644 > > --- a/include/hw/pci/pcie_port.h > > +++ b/include/hw/pci/pcie_port.h > > @@ -23,6 +23,9 @@ > > > > #include "hw/pci/pci_bridge.h" > > #include "hw/pci/pci_bus.h" > > +#include "qemu/notify.h" > > +#include "sysemu/sysemu.h" > > +#include "migration/vmstate.h" > > > > #define TYPE_PCIE_PORT "pcie-port" > > #define PCIE_PORT(obj) OBJECT_CHECK(PCIEPort, (obj), TYPE_PCIE_PORT) > > @@ -44,6 +47,10 @@ void pcie_port_init_reg(PCIDevice *d); > > struct PCIESlot { > > /*< private >*/ > > PCIEPort parent_obj; > > + bool unplug_is_deferred; > > + Notifier migration_notifier; > > + VMChangeStateEntry *vmstate_change; > > + > > /*< public >*/ > > > > /* pci express switch port with slot */ > > @@ -58,6 +65,19 @@ struct PCIESlot { > > QLIST_ENTRY(PCIESlot) next; > > }; > > > > +bool vmstate_deffered_unplug_needed(void *opaque); > > + > > +#define VMSTATE_DEFFERED_UNPLUG(parent_section_name) { \ > > + .name = parent_section_name "/deffered-unplug", \ > > + .version_id = 1, \ > > + .minimum_version_id = 1, \ > > + .needed = vmstate_deffered_unplug_needed, \ > > + .fields = (VMStateField[]) { \ > > + VMSTATE_BOOL(unplug_is_deferred, PCIESlot), \ > > + VMSTATE_END_OF_LIST() \ > > + } \ > > +} > > + > > void pcie_chassis_create(uint8_t chassis_number); > > PCIESlot *pcie_chassis_find_slot(uint8_t chassis, uint16_t slot); > > int pcie_chassis_add_slot(struct PCIESlot *slot); > > -- > > 2.24.1 > > > > > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK