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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
