[PATCH v2] hw/pci-host: save/restore pci host config register

Wang King posted 1 patch 3 years, 9 months ago
Test docker-quick@centos7 failed
Test docker-mingw@fedora failed
Test checkpatch failed
Test FreeBSD failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200723122301.2344-1-king.wang@huawei.com
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
hw/pci-host/i440fx.c       | 11 +++++++++++
hw/pci-host/q35.c          | 11 +++++++++++
hw/pci/pci_host.c          | 11 +++++++++++
hw/pci/pcie_host.c         | 11 +++++++++++
include/hw/pci/pci_host.h  | 10 ++++++++++
include/hw/pci/pcie_host.h | 10 ++++++++++
6 files changed, 64 insertions(+)
[PATCH v2] hw/pci-host: save/restore pci host config register
Posted by Wang King 3 years, 9 months ago
From: Hogan Wang <king.wang@huawei.com>

The pci host config register is used to save PCI address for
read/write config data. If guest write a value to config register,
and then pause the vcpu to migrate, After the migration, the guest
continue to write pci config data, and the write data will be ignored
because of new qemu process lost the config register state.

Reproduction steps are:
1. guest booting in seabios.
2. guest enable the SMRAM in seabios:piix4_apmc_smm_setup, and then
   expect to disable the SMRAM by pci_config_writeb.
3. after guest write the pci host config register, and then pasued vcpu
   to finish migration.
4. guest write config data(0x0A) fail to disable the SMRAM becasue of
   config register state lost.
5. guest continue to boot and crash in ipxe option ROM due to SMRAM in
   enabled state.

---
 hw/pci-host/i440fx.c       | 11 +++++++++++
 hw/pci-host/q35.c          | 11 +++++++++++
 hw/pci/pci_host.c          | 11 +++++++++++
 hw/pci/pcie_host.c         | 11 +++++++++++
 include/hw/pci/pci_host.h  | 10 ++++++++++
 include/hw/pci/pcie_host.h | 10 ++++++++++
 6 files changed, 64 insertions(+)

diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
index 8ed2417f0c..17705bb025 100644
--- a/hw/pci-host/i440fx.c
+++ b/hw/pci-host/i440fx.c
@@ -118,6 +118,16 @@ static const VMStateDescription vmstate_i440fx = {
     }
 };
 
+static const VMStateDescription vmstate_i440fx_pcihost = {
+    .name = "I440FX_PCIHost",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_PCI_HOST(parent_obj, I440FXState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void i440fx_pcihost_get_pci_hole_start(Object *obj, Visitor *v,
                                               const char *name, void *opaque,
                                               Error **errp)
@@ -398,6 +408,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
     hc->root_bus_path = i440fx_pcihost_root_bus_path;
     dc->realize = i440fx_pcihost_realize;
     dc->fw_name = "pci";
+    dc->vmsd = &vmstate_i440fx_pcihost;
     device_class_set_props(dc, i440fx_props);
     /* Reason: needs to be wired up by pc_init1 */
     dc->user_creatable = false;
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index b67cb9c29f..5e323be2e3 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -165,6 +165,16 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
     visit_type_uint64(v, name, &value, errp);
 }
 
+static const VMStateDescription vmstate_q35_pcihost = {
+    .name = "Q35_PCIHost",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_PCIE_HOST(parent_obj, Q35PCIHost),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 /*
  * NOTE: setting defaults for the mch.* fields in this table
  * doesn't work, because mch is a separate QOM object that is
@@ -194,6 +204,7 @@ static void q35_host_class_init(ObjectClass *klass, void *data)
 
     hc->root_bus_path = q35_host_root_bus_path;
     dc->realize = q35_host_realize;
+    dc->vmsd = &vmstate_q35_pcihost;
     device_class_set_props(dc, q35_host_props);
     /* Reason: needs to be wired up by pc_q35_init */
     dc->user_creatable = false;
diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
index ce7bcdb1d5..7cdd5a3ea3 100644
--- a/hw/pci/pci_host.c
+++ b/hw/pci/pci_host.c
@@ -24,6 +24,7 @@
 #include "hw/pci/pci_host.h"
 #include "qemu/module.h"
 #include "hw/pci/pci_bus.h"
+#include "migration/vmstate.h"
 #include "trace.h"
 
 /* debug PCI */
@@ -200,6 +201,16 @@ const MemoryRegionOps pci_host_data_be_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
+const VMStateDescription vmstate_pcihost = {
+    .name = "PCIHost",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(config_reg, PCIHostState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const TypeInfo pci_host_type_info = {
     .name = TYPE_PCI_HOST_BRIDGE,
     .parent = TYPE_SYS_BUS_DEVICE,
diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
index 3534006f99..a653c39bb7 100644
--- a/hw/pci/pcie_host.c
+++ b/hw/pci/pcie_host.c
@@ -24,6 +24,7 @@
 #include "hw/pci/pcie_host.h"
 #include "qemu/module.h"
 #include "exec/address-spaces.h"
+#include "migration/vmstate.h"
 
 /* a helper function to get a PCIDevice for a given mmconfig address */
 static inline PCIDevice *pcie_dev_find_by_mmcfg_addr(PCIBus *s,
@@ -121,6 +122,16 @@ void pcie_host_mmcfg_update(PCIExpressHost *e,
     memory_region_transaction_commit();
 }
 
+const VMStateDescription vmstate_pciehost = {
+    .name = "PCIEHost",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_PCI_HOST(pci, PCIExpressHost),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const TypeInfo pcie_host_type_info = {
     .name = TYPE_PCIE_HOST_BRIDGE,
     .parent = TYPE_PCI_HOST_BRIDGE,
diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
index 9ce088bd13..fc88305e04 100644
--- a/include/hw/pci/pci_host.h
+++ b/include/hw/pci/pci_host.h
@@ -70,4 +70,14 @@ extern const MemoryRegionOps pci_host_conf_be_ops;
 extern const MemoryRegionOps pci_host_data_le_ops;
 extern const MemoryRegionOps pci_host_data_be_ops;
 
+extern const VMStateDescription vmstate_pcihost;
+
+#define VMSTATE_PCI_HOST(_field, _state) {                           \
+    .name       = (stringify(_field)),                               \
+    .size       = sizeof(PCIHostState),                              \
+    .vmsd       = &vmstate_pcihost,                                  \
+    .flags      = VMS_STRUCT,                                        \
+    .offset     = vmstate_offset_value(_state, _field, PCIHostState),\
+}
+
 #endif /* PCI_HOST_H */
diff --git a/include/hw/pci/pcie_host.h b/include/hw/pci/pcie_host.h
index 3f7b9886d1..e8856f03e9 100644
--- a/include/hw/pci/pcie_host.h
+++ b/include/hw/pci/pcie_host.h
@@ -78,4 +78,14 @@ void pcie_host_mmcfg_update(PCIExpressHost *e,
                                          PCIE_MMCFG_DEVFN_MASK)
 #define PCIE_MMCFG_CONFOFFSET(addr)     ((addr) & PCIE_MMCFG_CONFOFFSET_MASK)
 
+extern const VMStateDescription vmstate_pciehost;
+
+#define VMSTATE_PCIE_HOST(_field, _state) {                            \
+    .name       = (stringify(_field)),                                 \
+    .size       = sizeof(PCIExpressHost),                              \
+    .vmsd       = &vmstate_pciehost,                                   \
+    .flags      = VMS_STRUCT,                                          \
+    .offset     = vmstate_offset_value(_state, _field, PCIExpressHost),\
+}
+
 #endif /* PCIE_HOST_H */
-- 
2.23.0



Re: [PATCH v2] hw/pci-host: save/restore pci host config register
Posted by Michael S. Tsirkin 3 years, 9 months ago
On Thu, Jul 23, 2020 at 08:23:01PM +0800, Wang King wrote:
> From: Hogan Wang <king.wang@huawei.com>
> 
> The pci host config register is used to save PCI address for
> read/write config data. If guest write a value to config register,
> and then pause the vcpu to migrate, After the migration, the guest
> continue to write pci config data, and the write data will be ignored
> because of new qemu process lost the config register state.
> 
> Reproduction steps are:
> 1. guest booting in seabios.
> 2. guest enable the SMRAM in seabios:piix4_apmc_smm_setup, and then
>    expect to disable the SMRAM by pci_config_writeb.
> 3. after guest write the pci host config register, and then pasued vcpu
>    to finish migration.
> 4. guest write config data(0x0A) fail to disable the SMRAM becasue of
>    config register state lost.
> 5. guest continue to boot and crash in ipxe option ROM due to SMRAM in
>    enabled state.


Could you pls add: changes from v1?
Also my comments on v1 still apply ...

> ---
>  hw/pci-host/i440fx.c       | 11 +++++++++++
>  hw/pci-host/q35.c          | 11 +++++++++++
>  hw/pci/pci_host.c          | 11 +++++++++++
>  hw/pci/pcie_host.c         | 11 +++++++++++
>  include/hw/pci/pci_host.h  | 10 ++++++++++
>  include/hw/pci/pcie_host.h | 10 ++++++++++
>  6 files changed, 64 insertions(+)
> 
> diff --git a/hw/pci-host/i440fx.c b/hw/pci-host/i440fx.c
> index 8ed2417f0c..17705bb025 100644
> --- a/hw/pci-host/i440fx.c
> +++ b/hw/pci-host/i440fx.c
> @@ -118,6 +118,16 @@ static const VMStateDescription vmstate_i440fx = {
>      }
>  };
>  
> +static const VMStateDescription vmstate_i440fx_pcihost = {
> +    .name = "I440FX_PCIHost",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_PCI_HOST(parent_obj, I440FXState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static void i440fx_pcihost_get_pci_hole_start(Object *obj, Visitor *v,
>                                                const char *name, void *opaque,
>                                                Error **errp)
> @@ -398,6 +408,7 @@ static void i440fx_pcihost_class_init(ObjectClass *klass, void *data)
>      hc->root_bus_path = i440fx_pcihost_root_bus_path;
>      dc->realize = i440fx_pcihost_realize;
>      dc->fw_name = "pci";
> +    dc->vmsd = &vmstate_i440fx_pcihost;
>      device_class_set_props(dc, i440fx_props);
>      /* Reason: needs to be wired up by pc_init1 */
>      dc->user_creatable = false;
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index b67cb9c29f..5e323be2e3 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -165,6 +165,16 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v,
>      visit_type_uint64(v, name, &value, errp);
>  }
>  
> +static const VMStateDescription vmstate_q35_pcihost = {
> +    .name = "Q35_PCIHost",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_PCIE_HOST(parent_obj, Q35PCIHost),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  /*
>   * NOTE: setting defaults for the mch.* fields in this table
>   * doesn't work, because mch is a separate QOM object that is
> @@ -194,6 +204,7 @@ static void q35_host_class_init(ObjectClass *klass, void *data)
>  
>      hc->root_bus_path = q35_host_root_bus_path;
>      dc->realize = q35_host_realize;
> +    dc->vmsd = &vmstate_q35_pcihost;
>      device_class_set_props(dc, q35_host_props);
>      /* Reason: needs to be wired up by pc_q35_init */
>      dc->user_creatable = false;
> diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> index ce7bcdb1d5..7cdd5a3ea3 100644
> --- a/hw/pci/pci_host.c
> +++ b/hw/pci/pci_host.c
> @@ -24,6 +24,7 @@
>  #include "hw/pci/pci_host.h"
>  #include "qemu/module.h"
>  #include "hw/pci/pci_bus.h"
> +#include "migration/vmstate.h"
>  #include "trace.h"
>  
>  /* debug PCI */
> @@ -200,6 +201,16 @@ const MemoryRegionOps pci_host_data_be_ops = {
>      .endianness = DEVICE_BIG_ENDIAN,
>  };
>  
> +const VMStateDescription vmstate_pcihost = {
> +    .name = "PCIHost",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(config_reg, PCIHostState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const TypeInfo pci_host_type_info = {
>      .name = TYPE_PCI_HOST_BRIDGE,
>      .parent = TYPE_SYS_BUS_DEVICE,
> diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
> index 3534006f99..a653c39bb7 100644
> --- a/hw/pci/pcie_host.c
> +++ b/hw/pci/pcie_host.c
> @@ -24,6 +24,7 @@
>  #include "hw/pci/pcie_host.h"
>  #include "qemu/module.h"
>  #include "exec/address-spaces.h"
> +#include "migration/vmstate.h"
>  
>  /* a helper function to get a PCIDevice for a given mmconfig address */
>  static inline PCIDevice *pcie_dev_find_by_mmcfg_addr(PCIBus *s,
> @@ -121,6 +122,16 @@ void pcie_host_mmcfg_update(PCIExpressHost *e,
>      memory_region_transaction_commit();
>  }
>  
> +const VMStateDescription vmstate_pciehost = {
> +    .name = "PCIEHost",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_PCI_HOST(pci, PCIExpressHost),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const TypeInfo pcie_host_type_info = {
>      .name = TYPE_PCIE_HOST_BRIDGE,
>      .parent = TYPE_PCI_HOST_BRIDGE,
> diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> index 9ce088bd13..fc88305e04 100644
> --- a/include/hw/pci/pci_host.h
> +++ b/include/hw/pci/pci_host.h
> @@ -70,4 +70,14 @@ extern const MemoryRegionOps pci_host_conf_be_ops;
>  extern const MemoryRegionOps pci_host_data_le_ops;
>  extern const MemoryRegionOps pci_host_data_be_ops;
>  
> +extern const VMStateDescription vmstate_pcihost;
> +
> +#define VMSTATE_PCI_HOST(_field, _state) {                           \
> +    .name       = (stringify(_field)),                               \
> +    .size       = sizeof(PCIHostState),                              \
> +    .vmsd       = &vmstate_pcihost,                                  \
> +    .flags      = VMS_STRUCT,                                        \
> +    .offset     = vmstate_offset_value(_state, _field, PCIHostState),\
> +}
> +
>  #endif /* PCI_HOST_H */
> diff --git a/include/hw/pci/pcie_host.h b/include/hw/pci/pcie_host.h
> index 3f7b9886d1..e8856f03e9 100644
> --- a/include/hw/pci/pcie_host.h
> +++ b/include/hw/pci/pcie_host.h
> @@ -78,4 +78,14 @@ void pcie_host_mmcfg_update(PCIExpressHost *e,
>                                           PCIE_MMCFG_DEVFN_MASK)
>  #define PCIE_MMCFG_CONFOFFSET(addr)     ((addr) & PCIE_MMCFG_CONFOFFSET_MASK)
>  
> +extern const VMStateDescription vmstate_pciehost;
> +
> +#define VMSTATE_PCIE_HOST(_field, _state) {                            \
> +    .name       = (stringify(_field)),                                 \
> +    .size       = sizeof(PCIExpressHost),                              \
> +    .vmsd       = &vmstate_pciehost,                                   \
> +    .flags      = VMS_STRUCT,                                          \
> +    .offset     = vmstate_offset_value(_state, _field, PCIExpressHost),\
> +}
> +
>  #endif /* PCIE_HOST_H */
> -- 
> 2.23.0
>