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

[Auger Eric]

Hi,

On 07/02/18 10:56, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (address@hidden) 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 <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?
> 
> 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.

The order looks OK to me. My understanding is all devices likely to
generate MSIs must be restored after the ITS MSI controller which is the
case here.

Thanks

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 / address@hidden / Manchester, UK
>