[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] hw/pci-host: save/restore pci host config register
From: |
Laszlo Ersek |
Subject: |
Re: [PATCH] hw/pci-host: save/restore pci host config register |
Date: |
Thu, 23 Jul 2020 13:48:47 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 |
On 07/23/20 12:49, 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.
>
> Example:
> 1. guest booting in seabios.
> 2. guest enabled the SMM memory window in piix4_apmc_smm_setup, and
> then try to close the SMM memory window.
> 3. pasued vcpu to finish migration.
> 4. guest close the SMM memory window fail becasue of config register
> state lost.
> 5. guest continue to boot and crash in ipxe option ROM (SMM memory
> window is enabled).
>
> Due to the complex guest, the negative effect is unpredictable.
> ---
> 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..a91ba0241a 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_pcihost, \
> + .flags = VMS_STRUCT, \
> + .offset = vmstate_offset_value(_state, _field, PCIExpressHost),\
> +}
> +
> #endif /* PCIE_HOST_H */
>
I won't claim that I understand half of what's going on, but the
"vmstate_pciehost" object is never used. The VMSTATE_PCIE_HOST macro
refers to "vmstate_pcihost", not "vmstate_pciehost".
Thanks
Laszlo