Re: [Qemu-devel] [PATCH v2] pci/bus: let it has higher migration priority

[Michael S. Tsirkin]

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 <address@hidden>
> CC: Marcel Apfelbaum <address@hidden>
> CC: Michael S. Tsirkin <address@hidden>
> CC: Dr. David Alan Gilbert <address@hidden>
> CC: Juan Quintela <address@hidden>
> CC: Laurent Vivier <address@hidden>
> Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1538953
> Reported-by: Maxime Coquelin <address@hidden>
> Signed-off-by: Peter Xu <address@hidden>

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