[PATCH 0/2] Store and load PCI device saved state across function resets

[PATCH 0/2] Store and load PCI device saved state across function resets

[Alex Williamson]

Bug https://bugs.launchpad.net/qemu/+bug/754591 is caused because
the KVM module attempts to do a pci_save_state() before assigning
the device to a VM, expecting that the saved state will remain
valid until we release the device.  This is in conflict with our
need to reset devices using PCI sysfs during a VM reset to
quiesce the device.  Any calls to pci_reset_function() will
overwrite the device saved stated prior to reset, and reload and
invalidate the state after.  KVM then ends up trying to restore
the state, but it's already invalid, so the device ends up with
reset values.

This series adds a mechanism to pull the saved state off into
an opaue buffer, which can be reloaded into the device at a later
point.  Thanks,

Alex

---

Alex Williamson (2):
      KVM: Use pci_store/load_saved_state() around VM device usage
      PCI: Add interfaces to store and load the device saved state


 drivers/pci/pci.c        |   94 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/kvm_host.h |    1 
 include/linux/pci.h      |    3 +
 virt/kvm/assigned-dev.c  |    8 ++--
 4 files changed, 103 insertions(+), 3 deletions(-)

[PATCH 1/2] PCI: Add interfaces to store and load the device saved state

[Alex Williamson]

For KVM device assignment, we'd like to save off the state of a device
prior to passing it to the guest and restore it later.  We also want
to allow pci_reset_funciton() to be called while the device is owned
by the guest.  This however overwrites and invalidates the struct pci_dev
buffers, so we can't just manually call save and restore.  Add generic
interfaces for the saved state to be stored into a buffer and reloaded
back into struct pci_dev at a later time.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 drivers/pci/pci.c   |   94 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h |    3 ++
 2 files changed, 97 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 2472e71..2b00354 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -975,6 +975,100 @@ void pci_restore_state(struct pci_dev *dev)
 	dev->state_saved = false;
 }
 
+struct pci_state {
+	u32 config_space[16];
+	u16 pcie_state[PCI_EXP_SAVE_REGS];
+	u16 pcix_state[1];
+};
+
+/**
+ * pci_store_saved_state - Store the device saved state into a buffer
+ * @dev: - PCI device that we're dealing with
+ *
+ * Returns an opaque buffer containing the device saved state.
+ * NULL if no state or error.
+ */
+void *pci_store_saved_state(struct pci_dev *dev)
+{
+	struct pci_state *state;
+	struct pci_cap_saved_state *cap_state;
+	int pos;
+
+	if (!dev->state_saved)
+		return NULL;
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return NULL;
+
+	memcpy(state->config_space, dev->saved_config_space,
+	       sizeof(state->config_space));
+
+	pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
+	cap_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
+	if (cap_state && pos)
+		memcpy(state->pcie_state, cap_state->data,
+		       sizeof(state->pcie_state));
+
+	pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
+	cap_state = pci_find_saved_cap(dev, PCI_CAP_ID_PCIX);
+	if (cap_state && pos)
+		memcpy(state->pcix_state, cap_state->data,
+		       sizeof(state->pcix_state));
+
+	return state;
+}
+EXPORT_SYMBOL_GPL(pci_store_saved_state);
+
+/**
+ * pci_load_saved_state - Load the device saved state from buffer
+ * @dev: - PCI device that we're dealing with
+ * @buf: - Saved state returned from pci_store_saved_state()
+ */
+void pci_load_saved_state(struct pci_dev *dev, void *buf)
+{
+	struct pci_state *state = buf;
+	struct pci_cap_saved_state *cap_state;
+	int pos;
+
+	if (!state) {
+		dev->state_saved = false;
+		return;
+	}
+
+	memcpy(dev->saved_config_space, state->config_space,
+	       sizeof(state->config_space));
+
+	pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
+	cap_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
+	if (cap_state && pos)
+		memcpy(cap_state->data, state->pcie_state,
+		       sizeof(state->pcie_state));
+
+	pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
+	cap_state = pci_find_saved_cap(dev, PCI_CAP_ID_PCIX);
+	if (cap_state && pos)
+		memcpy(cap_state->data, state->pcix_state,
+		       sizeof(state->pcix_state));
+
+	dev->state_saved = true;
+}
+EXPORT_SYMBOL_GPL(pci_load_saved_state);
+
+/**
+ * pci_load_and_free_saved_state - Load the device saved state from buffer
+ *				   and free the buffer
+ * @dev: - PCI device that we're dealing with
+ * @buf: - Pointer to saved state returned from pci_store_saved_state()
+ */
+void pci_load_and_free_saved_state(struct pci_dev *dev, void **buf)
+{
+	pci_load_saved_state(dev, *buf);
+	kfree(*buf);
+	*buf = NULL;
+}
+EXPORT_SYMBOL_GPL(pci_load_and_free_saved_state);
+
 static int do_pci_enable_device(struct pci_dev *dev, int bars)
 {
 	int err;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 96f70d7..67ce42a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -807,6 +807,9 @@ size_t pci_get_rom_size(struct pci_dev *pdev, void __iomem *rom, size_t size);
 /* Power management related routines */
 int pci_save_state(struct pci_dev *dev);
 void pci_restore_state(struct pci_dev *dev);
+void *pci_store_saved_state(struct pci_dev *dev);
+void pci_load_saved_state(struct pci_dev *dev, void *buf);
+void pci_load_and_free_saved_state(struct pci_dev *dev, void **buf);
 int __pci_complete_power_transition(struct pci_dev *dev, pci_power_t state);
 int pci_set_power_state(struct pci_dev *dev, pci_power_t state);
 pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state);

Re: [PATCH 1/2] PCI: Add interfaces to store and load the device saved state

[Avi Kivity]

On 04/15/2011 10:54 PM, Alex Williamson wrote:
> For KVM device assignment, we'd like to save off the state of a device
> prior to passing it to the guest and restore it later.  We also want
> to allow pci_reset_funciton() to be called while the device is owned
> by the guest.  This however overwrites and invalidates the struct pci_dev
> buffers, so we can't just manually call save and restore.  Add generic
> interfaces for the saved state to be stored into a buffer and reloaded
> back into struct pci_dev at a later time.
>
> +/**
> + * pci_store_saved_state - Store the device saved state into a buffer
> + * @dev: - PCI device that we're dealing with
> + *
> + * Returns an opaque buffer containing the device saved state.
> + * NULL if no state or error.
> + */
> +void *pci_store_saved_state(struct pci_dev *dev)

This is too opaque - no type safety.  You can return a struct pci_state 
*, leaving it undefined in headers files.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/2] PCI: Add interfaces to store and load the device saved state

[Alex Williamson]

On Sun, 2011-04-17 at 12:23 +0300, Avi Kivity wrote:
> On 04/15/2011 10:54 PM, Alex Williamson wrote:
> > For KVM device assignment, we'd like to save off the state of a device
> > prior to passing it to the guest and restore it later.  We also want
> > to allow pci_reset_funciton() to be called while the device is owned
> > by the guest.  This however overwrites and invalidates the struct pci_dev
> > buffers, so we can't just manually call save and restore.  Add generic
> > interfaces for the saved state to be stored into a buffer and reloaded
> > back into struct pci_dev at a later time.
> >
> > +/**
> > + * pci_store_saved_state - Store the device saved state into a buffer
> > + * @dev: - PCI device that we're dealing with
> > + *
> > + * Returns an opaque buffer containing the device saved state.
> > + * NULL if no state or error.
> > + */
> > +void *pci_store_saved_state(struct pci_dev *dev)
> 
> This is too opaque - no type safety.  You can return a struct pci_state 
> *, leaving it undefined in headers files.

Good idea, I'll roll this in.  Thanks,

Alex

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

[Alex Williamson]

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);
 	list_del(&match->list);
 	pci_release_regions(dev);
 out_disable:

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2233 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

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

[Alex Williamson]

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
> 

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2696 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

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

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

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

[Alex Williamson]

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().  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.
Thanks,

Alex

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

[Avi Kivity]

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?

Note avoiding the pointer would have removed the problem altogether.

-- 
error compiling committee.c: too many arguments to function

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

[Alex Williamson]

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

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

[Avi Kivity]

On 04/20/2011 06:13 PM, Alex Williamson wrote:
> >
> >  >     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.

"X didn't work" vs "X didn't work and I got this in the log"

> >  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,

Avoid the allocation altogether.  Having the caller be responsible for 
storage (in our case, embed the struct instead of the pointer).

You can encrypt the contents using the TPM, or maybe a comment 
indicating that the contents should suffice.

-- 
error compiling committee.c: too many arguments to function