diff mbox

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

Message ID 20110415195439.2838.58928.stgit@s20.home (mailing list archive)
State New, archived
Headers show

Commit Message

Alex Williamson April 15, 2011, 7:54 p.m. UTC 
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 kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Jan Kiszka April 15, 2011, 8:03 p.m. UTC | #1 
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
Alex Williamson April 15, 2011, 8:13 p.m. UTC | #2 
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 kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Kiszka April 15, 2011, 8:28 p.m. UTC | #3 
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 mbox

Patch

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: