[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:08 -0400 |
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