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

[Michael S. Tsirkin]

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