Re: [PATCH 2/2] KVM: Use pci_store/load_saved_state() around VM device usage

[Alex Williamson <alex.williamson@redhat.com>]

On Wed, 2011-04-20 at 10:19 +0300, Avi Kivity wrote:
> On 04/18/2011 10:43 PM, Alex Williamson wrote:
> > On Sun, 2011-04-17 at 12:25 +0300, Avi Kivity wrote:
> > >  On 04/15/2011 10:54 PM, Alex Williamson wrote:
> > >  >  Store the device saved state so that we can reload the device back
> > >  >  to the original state when it's unassigned.  This has the benefit
> > >  >  that the state survives across pci_reset_function() calls via
> > >  >  the PCI sysfs reset interface while the VM is using the device.
> > >
> > >  >  @@ -516,7 +518,7 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
> > >  >
> > >  >    	pci_reset_function(dev);
> > >  >    	pci_save_state(dev);
> > >  >  -
> > >  >  +	match->pci_saved_state = pci_store_saved_state(dev);
> > >  >    	match->assigned_dev_id = assigned_dev->assigned_dev_id;
> > >
> > >  Error check?
> > >
> > >  It might be better to give up the opacity of the data structure and make
> > >  pci_saved_state the full struct, not a pointer.
> >
> > pci_store_saved_state() returns NULL on error, which is correctly
> > handled if we pass NULL to pci_load_saved_state() or a pointer to NULL
> > to pci_load_and_free_saved_state().
> 
> But we silently swallow an error, this isn't good.
> 
> >    This is also why I changed the
> > __pci_reset_function() back to a normal pci_reset_function(), so we're
> > never left with an uninitialized device like we are now.
> >
> > We could be more verbose or return an error here, but we've gone for a
> > long time not even doing this save/restore across VM usage, so I don't
> > think it's worthy of preventing the device attachment if it fails.
> 
> At least a log?

Ok, I'm not sure what corrective action a user would take or what they
should expect not to work, but I guess a KERN_DEBUG printk is
reasonable.

> Note avoiding the pointer would have removed the problem altogether.

Returning a struct on store?  We lose any kind of opacity that way since
the caller needs to know about the struct then.  I thought the pointer
makes it clear the caller shouldn't be touching the contents, but if you
think it's a better way to go, I can try it.  Thanks,

Alex