Re: [PATCH 2/2] KVM: Use pci_store/load_saved_state() around VM device usage
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] KVM: Use pci_store/load_saved_state() around VM device usage
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] KVM: Use pci_store/load_saved_state() around VM device usage
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] KVM: Use pci_store/load_saved_state() around VM device usage
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] KVM: Use pci_store/load_saved_state() around VM device usage
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] KVM: Use pci_store/load_saved_state() around VM device usage
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
>>> ---
>>>
>>> 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
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 2/2] KVM: Use pci_store/load_saved_state() around VM device usage
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
> > ---
> >
> > 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] KVM: Use pci_store/load_saved_state() around VM device usage
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
> ---
>
> 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
signature.asc
Description: OpenPGP digital signature
[PATCH 2/2] KVM: Use pci_store/load_saved_state() around VM device usage
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
---
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:
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
