* Re: [PATCH v1] hw/pci-host: save/restore pci host config register
@ 2020-07-24 3:21 Wangjing (Hogan, Cloud Infrastructure Service Product Dept.)
2020-07-24 12:58 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Wangjing (Hogan, Cloud Infrastructure Service Product Dept.) @ 2020-07-24 3:21 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Michael S. Tsirkin
Cc: jusual, Wangxin (Alexander), Huangweidong (C), qemu-devel
On Sat, Jul 25, 2020 at 10:53:03AM Hogan Wang 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, Hogan Wang 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
>
The i440fx and q35 machines integrate i440FX or ICH9-LPC PCI device by
default. Refer to i440FX and ICH9-LPC spcifications, there are some reserved
configuration registers can used to save/restore PCIHostState.config_reg,
like i440FX.config[0x57] used for Older coreboot to get RAM size from QEMU.
whitch is nasty but it friendly to old ones.
> >
> > > >
> > > > > ---
> > > > > 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(+)
> > > > >
> > > >
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] hw/pci-host: save/restore pci host config register
2020-07-24 3:21 [PATCH v1] hw/pci-host: save/restore pci host config register Wangjing (Hogan, Cloud Infrastructure Service Product Dept.)
@ 2020-07-24 12:58 ` Michael S. Tsirkin
0 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2020-07-24 12:58 UTC (permalink / raw)
To: Wangjing (Hogan, Cloud Infrastructure Service Product Dept.)
Cc: Huangweidong (C), Wangxin (Alexander),
jusual, Dr. David Alan Gilbert, qemu-devel
On Fri, Jul 24, 2020 at 03:21:58AM +0000, Wangjing (Hogan, Cloud Infrastructure Service Product Dept.) wrote:
> On Sat, Jul 25, 2020 at 10:53:03AM Hogan Wang 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, Hogan Wang 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
> >
>
> The i440fx and q35 machines integrate i440FX or ICH9-LPC PCI device by
> default. Refer to i440FX and ICH9-LPC spcifications, there are some reserved
> configuration registers can used to save/restore PCIHostState.config_reg,
> like i440FX.config[0x57] used for Older coreboot to get RAM size from QEMU.
>
> whitch is nasty but it friendly to old ones.
Right. So what I propose is a series of two patches:
1. a patch to add it in a clean way to new machine types only.
2. a patch on top for old machine types to stick it in some
reserved register.
Then people can review both approaches and we can decide.
> > >
> > > > >
> > > > > > ---
> > > > > > 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(+)
> > > > > >
> > > > >
> > > > --
> > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > >
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] hw/pci-host: save/restore pci host config register
2020-07-23 16:04 ` Dr. David Alan Gilbert
@ 2020-07-24 12:58 ` Michael S. Tsirkin
0 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2020-07-24 12:58 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: weidong.huang, wangxinxin.wang, jusual, qemu-devel, Wang King
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] hw/pci-host: save/restore pci host config register
2020-07-23 15:50 ` Michael S. Tsirkin
@ 2020-07-23 16:04 ` Dr. David Alan Gilbert
2020-07-24 12:58 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Dr. David Alan Gilbert @ 2020-07-23 16:04 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: weidong.huang, wangxinxin.wang, jusual, qemu-devel, Wang King
* 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
>
> > >
> > > > ---
> > > > 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] hw/pci-host: save/restore pci host config register
2020-07-23 13:12 ` Dr. David Alan Gilbert
@ 2020-07-23 15:50 ` Michael S. Tsirkin
2020-07-23 16:04 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2020-07-23 15:50 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: weidong.huang, wangxinxin.wang, jusual, qemu-devel, Wang King
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.
> >
> > > ---
> > > 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] hw/pci-host: save/restore pci host config register
2020-07-23 12:56 ` Michael S. Tsirkin
@ 2020-07-23 13:12 ` Dr. David Alan Gilbert
2020-07-23 15:50 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Dr. David Alan Gilbert @ 2020-07-23 13:12 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: weidong.huang, wangxinxin.wang, jusual, qemu-devel, Wang King
* 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
>
> > ---
> > 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] hw/pci-host: save/restore pci host config register
2020-07-23 12:53 Wang King
@ 2020-07-23 12:56 ` Michael S. Tsirkin
2020-07-23 13:12 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2020-07-23 12:56 UTC (permalink / raw)
To: Wang King; +Cc: jsuaul, wangxinxin.wang, qemu-devel, dgilbert, weidong.huang
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?
> ---
> 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v1] hw/pci-host: save/restore pci host config register
@ 2020-07-23 12:53 Wang King
2020-07-23 12:56 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Wang King @ 2020-07-23 12:53 UTC (permalink / raw)
To: mst, marcel.apfelbaum
Cc: wangxinxin.wang, weidong.huang, qemu-devel, king.wang
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>
---
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-07-24 12:59 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-24 3:21 [PATCH v1] hw/pci-host: save/restore pci host config register Wangjing (Hogan, Cloud Infrastructure Service Product Dept.)
2020-07-24 12:58 ` Michael S. Tsirkin
-- strict thread matches above, loose matches on Subject: below --
2020-07-23 12:53 Wang King
2020-07-23 12:56 ` Michael S. Tsirkin
2020-07-23 13:12 ` Dr. David Alan Gilbert
2020-07-23 15:50 ` Michael S. Tsirkin
2020-07-23 16:04 ` Dr. David Alan Gilbert
2020-07-24 12:58 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.