Re: [Qemu-devel] Re: [PATCH] PCI: Bus number from the bridge, not the device

[Michael S. Tsirkin]

On Tue, Dec 21, 2010 at 07:13:57PM +0900, Isaku Yamahata wrote:
> On Thu, Dec 16, 2010 at 10:36:08AM +0200, Michael S. Tsirkin wrote:
> > On Thu, Dec 16, 2010 at 04:08:16PM +0900, Isaku Yamahata wrote:
> > > On Wed, Dec 15, 2010 at 08:27:49AM -0700, Alex Williamson wrote:
> > > > On Wed, 2010-12-15 at 11:56 +0200, Michael S. Tsirkin wrote:
> > > > > On Tue, Dec 14, 2010 at 11:34:53AM -0700, Alex Williamson wrote:
> > > > > > On Tue, 2010-12-14 at 14:26 +0200, Michael S. Tsirkin wrote:
> > > > > > > On Mon, Dec 13, 2010 at 10:04:24PM -0700, Alex Williamson wrote:
> > > > > > > > 
> > > > > > > > I've only ever seen config[PCI_SECONDARY_BUS] be non-zero for an
> > > > > > > > assigned device, so I'm pretty sure we're not going to hurt 
> > > > > > > > migration,
> > > > > > > > but the code is clearly wrong and I'd like to make sure we 
> > > > > > > > don't trip on
> > > > > > > > a migration failure for a minor device config space change.
> > > > > > > 
> > > > > > > Which reminds me: maybe just mark nested bridges as 
> > > > > > > non-migrateable
> > > > > > > for now?  Care writing such a patch?
> > > > > > 
> > > > > > Hmm, this is trickier than it sounds.
> > > > > 
> > > > > Hmm, since 0 is put in the path instead of the bridge number,
> > > > > will the correct bridge be restored?
> > > > > 
> > > > > >  We're really only broken wrt
> > > > > > migration if a device under a bridge calls qemu_ram_alloc.
> > > > > 
> > > > > I guess there's more broken-ness. What exactly breaks qemu_ram_alloc?
> > > > 
> > > > You're right, it's more broken than that.  Anything that calls
> > > > get_dev_path is broken for migration of bridges since the path is
> > > > determined before the guest updates bus numbers.  That includes
> > > > qemu_ram_alloc and vmstate.  I was only looking at the qemu_ram_alloc
> > > > side.  So perhaps the right answer, for the moment, is to block
> > > > migration if there's a p2p bridge.
> > > 
> > > Eww. That's bad. Anyway, I agree to disable it for the moment.
> > 
> > Patch?
> 
> Although I'm willing to create patch, how to disable the migration?

Set the no_migrate flag in SaveStateEntry.

> As long as I know, Alex Williamson had tried to push the patch series
> "Save state error handling (kill off no_migrate)", they haven't been
> merged.
> If they are merged in, it seems quite easy to disable the migration.

These look unlikely to be merged: they seem to go
contrary to the table-driver approach to migration that we are trying to
take.

> Anyway register_device_unmigratable() seems to need some clean up
> for DeviceInfo::vmsd. Any ideas?

So clean it up if you like.

> -- 
> yamahata