[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1] hw/pci-host: save/restore pci host config register
|
From: |
Michael S. Tsirkin |
|
Subject: |
Re: [PATCH v1] hw/pci-host: save/restore pci host config register |
|
Date: |
Fri, 24 Jul 2020 08:58:56 -0400 |
On Thu, Jul 23, 2020 at 05:04:53PM +0100, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > On Thu, Jul 23, 2020 at 02:12:54PM +0100, Dr. David Alan Gilbert wrote:
> > > * Michael S. Tsirkin (mst@redhat.com) wrote:
> > > > On Thu, Jul 23, 2020 at 08:53:03PM +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.
> > > > >
> > > > > Signed-off-by: Hogan Wang <king.wang@huawei.com>
> > > >
> > > > I guess this is like v3 right?
> > > >
> > > > thanks a lot for the patch!
> > > >
> > > > My question stands : does anyone see a way to pass this
> > > > info around without breaking migration for all existing
> > > > machine types?
> > >
> > > You need a .needed clause in the vmstate_i440fx_pcihost and
> > > vmstate_q35_pcihost which is a pointer to a function which enables it on
> > > new machine types and ignores it on old ones.
> > >
> > > Or, if it always crashes if the SMRAM is enabled, then the migration is
> > > dead anyway; so you could make the .needed only save the config if
> > > the SMRAM is opened, so you'd get a unknown section error, which is
> > > nasty but it would only happen in the case it would crash anyway.
> > >
> > > Dave
> >
> > Problem is we never know whether it's needed.
> >
> > For example: guest programs cf8, then cfc.
> > Guest on destination can crash if migrated after
> > writing cf8 before writing cfc.
> > But in theory it can also crash if guest assumes
> > cf8 is unchanged and just writes cfc.
> >
> > So what I'd prefer to do is put it in some data that
> > old qemu ignores. Then once qemu on destination
> > is updated, it will start interpreting it.
>
> We don't have a way to do that; the choice is:
> a) Not sending it for old versions, so you only get the
> fix for new machine types
>
> b) Trying to second guess when it will crash
>
> I recommend (a) generally - but the format has no
> way to ignore unknown data.
>
> Dave
How about adding that finally, so we have a way to fix bugs like this one?
> >
> > > >
> > > > > ---
> > > > > 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
> > > > >
> > > >
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK