| Message ID | 20110415195439.2838.58928.stgit@s20.home (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 2011-04-15 21:54, 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. > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > --- > > include/linux/kvm_host.h | 1 + > virt/kvm/assigned-dev.c | 8 +++++--- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index ab42855..d8a1d18 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -513,6 +513,7 @@ struct kvm_assigned_dev_kernel { > struct kvm *kvm; > spinlock_t intx_lock; > char irq_name[32]; > + void *pci_saved_state; > }; > > struct kvm_irq_mask_notifier { > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c > index ae72ae6..66c6ccd 100644 > --- a/virt/kvm/assigned-dev.c > +++ b/virt/kvm/assigned-dev.c > @@ -197,7 +197,9 @@ static void kvm_free_assigned_device(struct kvm *kvm, > { > kvm_free_assigned_irq(kvm, assigned_dev); > > - __pci_reset_function(assigned_dev->dev); > + pci_reset_function(assigned_dev->dev); > + pci_load_and_free_saved_state(assigned_dev->dev, > + &assigned_dev->pci_saved_state); > pci_restore_state(assigned_dev->dev); > > pci_release_regions(assigned_dev->dev); > @@ -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; > match->host_segnr = assigned_dev->segnr; > match->host_busnr = assigned_dev->busnr; > @@ -546,7 +548,7 @@ out: > mutex_unlock(&kvm->lock); > return r; > out_list_del: > - pci_restore_state(dev); > + pci_load_and_free_saved_state(dev, &match->pci_saved_state); Don't you need to keep the balance, ie. load_and_free, then restore? > list_del(&match->list); > pci_release_regions(dev); > out_disable: > Thanks for addressing the issue! Jan
On Fri, 2011-04-15 at 22:03 +0200, Jan Kiszka wrote: > On 2011-04-15 21:54, 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. > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > --- > > > > include/linux/kvm_host.h | 1 + > > virt/kvm/assigned-dev.c | 8 +++++--- > > 2 files changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > index ab42855..d8a1d18 100644 > > --- a/include/linux/kvm_host.h > > +++ b/include/linux/kvm_host.h > > @@ -513,6 +513,7 @@ struct kvm_assigned_dev_kernel { > > struct kvm *kvm; > > spinlock_t intx_lock; > > char irq_name[32]; > > + void *pci_saved_state; > > }; > > > > struct kvm_irq_mask_notifier { > > diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c > > index ae72ae6..66c6ccd 100644 > > --- a/virt/kvm/assigned-dev.c > > +++ b/virt/kvm/assigned-dev.c > > @@ -197,7 +197,9 @@ static void kvm_free_assigned_device(struct kvm *kvm, > > { > > kvm_free_assigned_irq(kvm, assigned_dev); > > > > - __pci_reset_function(assigned_dev->dev); > > + pci_reset_function(assigned_dev->dev); > > + pci_load_and_free_saved_state(assigned_dev->dev, > > + &assigned_dev->pci_saved_state); > > pci_restore_state(assigned_dev->dev); > > > > pci_release_regions(assigned_dev->dev); > > @@ -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; > > match->host_segnr = assigned_dev->segnr; > > match->host_busnr = assigned_dev->busnr; > > @@ -546,7 +548,7 @@ out: > > mutex_unlock(&kvm->lock); > > return r; > > out_list_del: > > - pci_restore_state(dev); > > + pci_load_and_free_saved_state(dev, &match->pci_saved_state); > > Don't you need to keep the balance, ie. load_and_free, then restore? I don't see that pci_save_state() does anything more than buffer the hardware device state into save areas in struct pci_dev. So by not doing a restore, we are leaving that valid, but I don't really see how that hurts anything. The only reason we even really need to call pci_load_and_free_saved_state() here is to free the buffer Thanks, Alex > > list_del(&match->list); > > pci_release_regions(dev); > > out_disable: > > > > Thanks for addressing the issue! > > Jan > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2011-04-15 22:13, Alex Williamson wrote: > On Fri, 2011-04-15 at 22:03 +0200, Jan Kiszka wrote: >> On 2011-04-15 21:54, 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. >>> >>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com> >>> --- >>> >>> include/linux/kvm_host.h | 1 + >>> virt/kvm/assigned-dev.c | 8 +++++--- >>> 2 files changed, 6 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >>> index ab42855..d8a1d18 100644 >>> --- a/include/linux/kvm_host.h >>> +++ b/include/linux/kvm_host.h >>> @@ -513,6 +513,7 @@ struct kvm_assigned_dev_kernel { >>> struct kvm *kvm; >>> spinlock_t intx_lock; >>> char irq_name[32]; >>> + void *pci_saved_state; >>> }; >>> >>> struct kvm_irq_mask_notifier { >>> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c >>> index ae72ae6..66c6ccd 100644 >>> --- a/virt/kvm/assigned-dev.c >>> +++ b/virt/kvm/assigned-dev.c >>> @@ -197,7 +197,9 @@ static void kvm_free_assigned_device(struct kvm *kvm, >>> { >>> kvm_free_assigned_irq(kvm, assigned_dev); >>> >>> - __pci_reset_function(assigned_dev->dev); >>> + pci_reset_function(assigned_dev->dev); >>> + pci_load_and_free_saved_state(assigned_dev->dev, >>> + &assigned_dev->pci_saved_state); >>> pci_restore_state(assigned_dev->dev); >>> >>> pci_release_regions(assigned_dev->dev); >>> @@ -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; >>> match->host_segnr = assigned_dev->segnr; >>> match->host_busnr = assigned_dev->busnr; >>> @@ -546,7 +548,7 @@ out: >>> mutex_unlock(&kvm->lock); >>> return r; >>> out_list_del: >>> - pci_restore_state(dev); >>> + pci_load_and_free_saved_state(dev, &match->pci_saved_state); >> >> Don't you need to keep the balance, ie. load_and_free, then restore? > > I don't see that pci_save_state() does anything more than buffer the > hardware device state into save areas in struct pci_dev. So by not > doing a restore, we are leaving that valid, but I don't really see how > that hurts anything. Right, I'm just unsure if we should encode this knowledge about how pci_save/restore_state works internally into KVM. Jan
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index ab42855..d8a1d18 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -513,6 +513,7 @@ struct kvm_assigned_dev_kernel { struct kvm *kvm; spinlock_t intx_lock; char irq_name[32]; + void *pci_saved_state; }; struct kvm_irq_mask_notifier { diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c index ae72ae6..66c6ccd 100644 --- a/virt/kvm/assigned-dev.c +++ b/virt/kvm/assigned-dev.c @@ -197,7 +197,9 @@ static void kvm_free_assigned_device(struct kvm *kvm, { kvm_free_assigned_irq(kvm, assigned_dev); - __pci_reset_function(assigned_dev->dev); + pci_reset_function(assigned_dev->dev); + pci_load_and_free_saved_state(assigned_dev->dev, + &assigned_dev->pci_saved_state); pci_restore_state(assigned_dev->dev); pci_release_regions(assigned_dev->dev); @@ -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; match->host_segnr = assigned_dev->segnr; match->host_busnr = assigned_dev->busnr; @@ -546,7 +548,7 @@ out: mutex_unlock(&kvm->lock); return r; out_list_del: - pci_restore_state(dev); + pci_load_and_free_saved_state(dev, &match->pci_saved_state); list_del(&match->list); pci_release_regions(dev); out_disable:
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. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- include/linux/kvm_host.h | 1 + virt/kvm/assigned-dev.c | 8 +++++--- 2 files changed, 6 insertions(+), 3 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html