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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> 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.williamson@redhat.com>
> > CC: Marcel Apfelbaum <marcel@redhat.com>
> > CC: Michael S. Tsirkin <mst@redhat.com>
> > CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > CC: Juan Quintela <quintela@redhat.com>
> > CC: Laurent Vivier <lvivier@redhat.com>
> > Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1538953
> > Reported-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> dgilbert, could you pls confirm this looks like a sane approach to you?

Yes it does, although a couple of (minor) thoughts:
    a) I'm a little confused whether this needs to apply to all PCI
busses (and hence why it's MIG_PRI_PCI_BUS) or only the root ports
    b) It's probably worth getting one of the Arm [e[;e tp check the
order relative to the GICV3 entries; cc'ing Eric.

Dave


> > ---
> > 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
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK