On Tue, Feb 06, 2018 at 03:39:33PM +0800, Peter Xu wrote:
> In the past, we prioritized IOMMU migration so that we have such a
> priority order:
>
> IOMMU > PCI Devices
>
> When migrating a guest with both vIOMMU and a pcie-root-port, we'll
> always migrate vIOMMU first, since pci buses will be seen to have the
> same priority of general PCI devices.
>
> That's problematic.
>
> The thing is that PCI bus number information is stored in the root port,
> and that is needed by vIOMMU during post_load(), e.g., to figure out
> context entry for a device. If we don't have correct bus numbers for
> devices, we won't be able to recover device state of the DMAR memory
> regions, and things will be messed up.
>
> So let's boost the PCIe root ports to be even with higher priority:
>
> PCIe Root Port > IOMMU > PCI Devices
>
> A smoke test shows that this patch fixes bug 1538953.
>
> Also, apply this rule to all the PCI bus/bridge devices: ioh3420,
> xio3130_downstream, xio3130_upstream, pcie_pci_bridge, pci-pci bridge,
> i82801b11.
>
> I noted that we set pcie_pci_bridge_dev_vmstate twice. Clean that up
> together.
>
> CC: Alex Williamson <alex.william...@redhat.com>
> CC: Marcel Apfelbaum <mar...@redhat.com>
> CC: Michael S. Tsirkin <m...@redhat.com>
> CC: Dr. David Alan Gilbert <dgilb...@redhat.com>
> CC: Juan Quintela <quint...@redhat.com>
> CC: Laurent Vivier <lviv...@redhat.com>
> Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1538953
> Reported-by: Maxime Coquelin <maxime.coque...@redhat.com>
> Signed-off-by: Peter Xu <pet...@redhat.com>
dgilbert, could you pls confirm this looks like a sane approach to you?
> ---
> v2:
> - add more devices that Marcel mentioned
> - rename to MIG_PRI_PCI_BUS
> - remove one useless line in existing code
> ---
> hw/pci-bridge/gen_pcie_root_port.c | 1 +
> hw/pci-bridge/i82801b11.c | 1 +
> hw/pci-bridge/ioh3420.c | 1 +
> hw/pci-bridge/pci_bridge_dev.c | 1 +
> hw/pci-bridge/pcie_pci_bridge.c | 2 +-
> hw/pci-bridge/xio3130_downstream.c | 1 +
> hw/pci-bridge/xio3130_upstream.c | 1 +
> include/migration/vmstate.h | 1 +
> 8 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/hw/pci-bridge/gen_pcie_root_port.c
> b/hw/pci-bridge/gen_pcie_root_port.c
> index 0e2f2e8bf1..435fbaa60e 100644
> --- a/hw/pci-bridge/gen_pcie_root_port.c
> +++ b/hw/pci-bridge/gen_pcie_root_port.c
> @@ -101,6 +101,7 @@ static void gen_rp_realize(DeviceState *dev, Error **errp)
>
> static const VMStateDescription vmstate_rp_dev = {
> .name = "pcie-root-port",
> + .priority = MIG_PRI_PCI_BUS,
> .version_id = 1,
> .minimum_version_id = 1,
> .post_load = pcie_cap_slot_post_load,
> diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
> index cb522bf30c..60df9b2c96 100644
> --- a/hw/pci-bridge/i82801b11.c
> +++ b/hw/pci-bridge/i82801b11.c
> @@ -80,6 +80,7 @@ err_bridge:
>
> static const VMStateDescription i82801b11_bridge_dev_vmstate = {
> .name = "i82801b11_bridge",
> + .priority = MIG_PRI_PCI_BUS,
> .fields = (VMStateField[]) {
> VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
> VMSTATE_END_OF_LIST()
> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> index 5f56a2feb6..a7bfbdd238 100644
> --- a/hw/pci-bridge/ioh3420.c
> +++ b/hw/pci-bridge/ioh3420.c
> @@ -83,6 +83,7 @@ static void ioh3420_interrupts_uninit(PCIDevice *d)
>
> static const VMStateDescription vmstate_ioh3420 = {
> .name = "ioh-3240-express-root-port",
> + .priority = MIG_PRI_PCI_BUS,
> .version_id = 1,
> .minimum_version_id = 1,
> .post_load = pcie_cap_slot_post_load,
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index d56f6638c2..b2d861d216 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -174,6 +174,7 @@ static bool pci_device_shpc_present(void *opaque, int
> version_id)
>
> static const VMStateDescription pci_bridge_dev_vmstate = {
> .name = "pci_bridge",
> + .priority = MIG_PRI_PCI_BUS,
> .fields = (VMStateField[]) {
> VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
> SHPC_VMSTATE(shpc, PCIDevice, pci_device_shpc_present),
> diff --git a/hw/pci-bridge/pcie_pci_bridge.c b/hw/pci-bridge/pcie_pci_bridge.c
> index a4d827c99d..e5ac7974cf 100644
> --- a/hw/pci-bridge/pcie_pci_bridge.c
> +++ b/hw/pci-bridge/pcie_pci_bridge.c
> @@ -129,6 +129,7 @@ static Property pcie_pci_bridge_dev_properties[] = {
>
> static const VMStateDescription pcie_pci_bridge_dev_vmstate = {
> .name = TYPE_PCIE_PCI_BRIDGE_DEV,
> + .priority = MIG_PRI_PCI_BUS,
> .fields = (VMStateField[]) {
> VMSTATE_PCI_DEVICE(parent_obj, PCIBridge),
> SHPC_VMSTATE(shpc, PCIDevice, NULL),
> @@ -178,7 +179,6 @@ static void pcie_pci_bridge_class_init(ObjectClass
> *klass, void *data)
> k->config_write = pcie_pci_bridge_write_config;
> dc->vmsd = &pcie_pci_bridge_dev_vmstate;
> dc->props = pcie_pci_bridge_dev_properties;
> - dc->vmsd = &pcie_pci_bridge_dev_vmstate;
> dc->reset = &pcie_pci_bridge_reset;
> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> hc->plug = pcie_pci_bridge_hotplug_cb;
> diff --git a/hw/pci-bridge/xio3130_downstream.c
> b/hw/pci-bridge/xio3130_downstream.c
> index 1e09d2afb7..4dd2e65118 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -161,6 +161,7 @@ static Property xio3130_downstream_props[] = {
>
> static const VMStateDescription vmstate_xio3130_downstream = {
> .name = "xio3130-express-downstream-port",
> + .priority = MIG_PRI_PCI_BUS,
> .version_id = 1,
> .minimum_version_id = 1,
> .post_load = pcie_cap_slot_post_load,
> diff --git a/hw/pci-bridge/xio3130_upstream.c
> b/hw/pci-bridge/xio3130_upstream.c
> index 227997ce46..c5f02a6ee8 100644
> --- a/hw/pci-bridge/xio3130_upstream.c
> +++ b/hw/pci-bridge/xio3130_upstream.c
> @@ -133,6 +133,7 @@ PCIEPort *xio3130_upstream_init(PCIBus *bus, int devfn,
> bool multifunction,
>
> static const VMStateDescription vmstate_xio3130_upstream = {
> .name = "xio3130-express-upstream-port",
> + .priority = MIG_PRI_PCI_BUS,
> .version_id = 1,
> .minimum_version_id = 1,
> .fields = (VMStateField[]) {
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 8c3889433c..df463fd33d 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -148,6 +148,7 @@ enum VMStateFlags {
> typedef enum {
> MIG_PRI_DEFAULT = 0,
> MIG_PRI_IOMMU, /* Must happen before PCI devices */
> + MIG_PRI_PCI_BUS, /* Must happen before IOMMU */
> MIG_PRI_GICV3_ITS, /* Must happen before PCI devices */
> MIG_PRI_GICV3, /* Must happen before the ITS */
> MIG_PRI_MAX,
> --
> 2.14.3
