[PATCH] pci-assign: Do not reset the device unless the kernel supports it

[PATCH] pci-assign: Do not reset the device unless the kernel supports it

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

At least kernels 2.6.38 and 2.6.39 do not properly support issuing a
reset on an assigned device and corrupt its config space. Prevent
this by checking for a host kernel with the required support, tagged by
the to-be-introduced KVM_CAP_DEVICE_RESET.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

PS: What's the state of those KVM patches? Will they make it into 3.0?

 hw/device-assignment.c |   33 +++++++++++++++++++--------------
 1 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 57d8dc0..97a1450 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -1689,26 +1689,31 @@ static const VMStateDescription vmstate_assigned_device = {
 static void reset_assigned_device(DeviceState *dev)
 {
     PCIDevice *pci_dev = DO_UPCAST(PCIDevice, qdev, dev);
+#ifdef KVM_CAP_DEVICE_RESET
     AssignedDevice *adev = DO_UPCAST(AssignedDevice, dev, pci_dev);
     char reset_file[64];
     const char reset[] = "1";
     int fd, ret;
 
-    snprintf(reset_file, sizeof(reset_file),
-             "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/reset",
-             adev->host.seg, adev->host.bus, adev->host.dev, adev->host.func);
-
-    /*
-     * Issue a device reset via pci-sysfs.  Note that we use write(2) here
-     * and ignore the return value because some kernels have a bug that
-     * returns 0 rather than bytes written on success, sending us into an
-     * infinite retry loop using other write mechanisms.
-     */
-    fd = open(reset_file, O_WRONLY);
-    if (fd != -1) {
-        ret = write(fd, reset, strlen(reset));
-        close(fd);
+    if (kvm_check_extension(kvm_state, KVM_CAP_DEVICE_RESET) {
+        snprintf(reset_file, sizeof(reset_file),
+                 "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/reset",
+                 adev->host.seg, adev->host.bus, adev->host.dev,
+                 adev->host.func);
+
+        /*
+         * Issue a device reset via pci-sysfs.  Note that we use write(2) here
+         * and ignore the return value because some kernels have a bug that
+         * returns 0 rather than bytes written on success, sending us into an
+         * infinite retry loop using other write mechanisms.
+         */
+        fd = open(reset_file, O_WRONLY);
+        if (fd != -1) {
+            ret = write(fd, reset, strlen(reset));
+            close(fd);
+        }
     }
+#endif /* KVM_CAP_DEVICE_RESET */
 
     /*
      * When a 0 is written to the command register, the device is logically

Re: [PATCH] pci-assign: Do not reset the device unless the kernel supports it

[Alex Williamson]

On Mon, 2011-06-06 at 23:30 +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> At least kernels 2.6.38 and 2.6.39 do not properly support issuing a
> reset on an assigned device and corrupt its config space. Prevent
> this by checking for a host kernel with the required support, tagged by
> the to-be-introduced KVM_CAP_DEVICE_RESET.

Wouldn't it be easier just to revert ed78661f in 2.6.39 stable?  I guess
we don't have an option to do that for .38 since stable is done there,
but there are also some intel-iommu breakages that won't make stable for
that release.  It seems like the userspace invoked reset resolves known,
demonstrable issues of devices continuing to DMA into guest memory while
ed78661f is mostly a theoretical change.

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> PS: What's the state of those KVM patches? Will they make it into 3.0?

The PCI save/restore ones are in:

f8fcfd775523347afe460dc3a0f45d0479e784a2
ffbdd3f7931fb7cb7e36d00d16303ec433be5145
24a4742f0be6226eb0106fbb17caf4d711d1ad43

Thanks,
Alex

> 
>  hw/device-assignment.c |   33 +++++++++++++++++++--------------
>  1 files changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 57d8dc0..97a1450 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -1689,26 +1689,31 @@ static const VMStateDescription vmstate_assigned_device = {
>  static void reset_assigned_device(DeviceState *dev)
>  {
>      PCIDevice *pci_dev = DO_UPCAST(PCIDevice, qdev, dev);
> +#ifdef KVM_CAP_DEVICE_RESET
>      AssignedDevice *adev = DO_UPCAST(AssignedDevice, dev, pci_dev);
>      char reset_file[64];
>      const char reset[] = "1";
>      int fd, ret;
>  
> -    snprintf(reset_file, sizeof(reset_file),
> -             "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/reset",
> -             adev->host.seg, adev->host.bus, adev->host.dev, adev->host.func);
> -
> -    /*
> -     * Issue a device reset via pci-sysfs.  Note that we use write(2) here
> -     * and ignore the return value because some kernels have a bug that
> -     * returns 0 rather than bytes written on success, sending us into an
> -     * infinite retry loop using other write mechanisms.
> -     */
> -    fd = open(reset_file, O_WRONLY);
> -    if (fd != -1) {
> -        ret = write(fd, reset, strlen(reset));
> -        close(fd);
> +    if (kvm_check_extension(kvm_state, KVM_CAP_DEVICE_RESET) {
> +        snprintf(reset_file, sizeof(reset_file),
> +                 "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/reset",
> +                 adev->host.seg, adev->host.bus, adev->host.dev,
> +                 adev->host.func);
> +
> +        /*
> +         * Issue a device reset via pci-sysfs.  Note that we use write(2) here
> +         * and ignore the return value because some kernels have a bug that
> +         * returns 0 rather than bytes written on success, sending us into an
> +         * infinite retry loop using other write mechanisms.
> +         */
> +        fd = open(reset_file, O_WRONLY);
> +        if (fd != -1) {
> +            ret = write(fd, reset, strlen(reset));
> +            close(fd);
> +        }
>      }
> +#endif /* KVM_CAP_DEVICE_RESET */
>  
>      /*
>       * When a 0 is written to the command register, the device is logically
> --
> 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

Re: [PATCH] pci-assign: Do not reset the device unless the kernel supports it

[Jan Kiszka]

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

On 2011-06-06 23:48, Alex Williamson wrote:
> On Mon, 2011-06-06 at 23:30 +0200, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> At least kernels 2.6.38 and 2.6.39 do not properly support issuing a
>> reset on an assigned device and corrupt its config space. Prevent
>> this by checking for a host kernel with the required support, tagged by
>> the to-be-introduced KVM_CAP_DEVICE_RESET.
> 
> Wouldn't it be easier just to revert ed78661f in 2.6.39 stable?  I guess
> we don't have an option to do that for .38 since stable is done there,
> but there are also some intel-iommu breakages that won't make stable for
> that release.  It seems like the userspace invoked reset resolves known,
> demonstrable issues of devices continuing to DMA into guest memory while
> ed78661f is mostly a theoretical change.

Easier would be this patch. But I don't mind reverting the problematic
commit in 39, whatever is preferred. We should just resolve the issue
finally.

> 
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> PS: What's the state of those KVM patches? Will they make it into 3.0?
> 
> The PCI save/restore ones are in:
> 
> f8fcfd775523347afe460dc3a0f45d0479e784a2
> ffbdd3f7931fb7cb7e36d00d16303ec433be5145
> 24a4742f0be6226eb0106fbb17caf4d711d1ad43

Oh, they are just missing in kvm.git so far.

Jan


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

Re: [PATCH] pci-assign: Do not reset the device unless the kernel supports it

[Avi Kivity]

On 06/07/2011 01:04 AM, Jan Kiszka wrote:
> On 2011-06-06 23:48, Alex Williamson wrote:
> >  On Mon, 2011-06-06 at 23:30 +0200, Jan Kiszka wrote:
> >>  From: Jan Kiszka<jan.kiszka@siemens.com>
> >>
> >>  At least kernels 2.6.38 and 2.6.39 do not properly support issuing a
> >>  reset on an assigned device and corrupt its config space. Prevent
> >>  this by checking for a host kernel with the required support, tagged by
> >>  the to-be-introduced KVM_CAP_DEVICE_RESET.
> >
> >  Wouldn't it be easier just to revert ed78661f in 2.6.39 stable?  I guess
> >  we don't have an option to do that for .38 since stable is done there,
> >  but there are also some intel-iommu breakages that won't make stable for
> >  that release.  It seems like the userspace invoked reset resolves known,
> >  demonstrable issues of devices continuing to DMA into guest memory while
> >  ed78661f is mostly a theoretical change.
>
> Easier would be this patch. But I don't mind reverting the problematic
> commit in 39, whatever is preferred. We should just resolve the issue
> finally.

Kernel problems should be solved in the kernel (with exceptions of 
course, but don't see the need here).


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH] pci-assign: Do not reset the device unless the kernel supports it

[Jan Kiszka]

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

On 2011-06-07 10:06, Avi Kivity wrote:
> On 06/07/2011 01:04 AM, Jan Kiszka wrote:
>> On 2011-06-06 23:48, Alex Williamson wrote:
>> >  On Mon, 2011-06-06 at 23:30 +0200, Jan Kiszka wrote:
>> >>  From: Jan Kiszka<jan.kiszka@siemens.com>
>> >>
>> >>  At least kernels 2.6.38 and 2.6.39 do not properly support issuing a
>> >>  reset on an assigned device and corrupt its config space. Prevent
>> >>  this by checking for a host kernel with the required support,
>> tagged by
>> >>  the to-be-introduced KVM_CAP_DEVICE_RESET.
>> >
>> >  Wouldn't it be easier just to revert ed78661f in 2.6.39 stable?  I
>> guess
>> >  we don't have an option to do that for .38 since stable is done there,
>> >  but there are also some intel-iommu breakages that won't make
>> stable for
>> >  that release.  It seems like the userspace invoked reset resolves
>> known,
>> >  demonstrable issues of devices continuing to DMA into guest memory
>> while
>> >  ed78661f is mostly a theoretical change.
>>
>> Easier would be this patch. But I don't mind reverting the problematic
>> commit in 39, whatever is preferred. We should just resolve the issue
>> finally.
> 
> Kernel problems should be solved in the kernel (with exceptions of
> course, but don't see the need here).

Then please file a revert for stable ASAP.

Jan


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

Re: [PATCH] pci-assign: Do not reset the device unless the kernel supports it

[Alex Williamson]

On Tue, 2011-06-07 at 10:14 +0200, Jan Kiszka wrote:
> On 2011-06-07 10:06, Avi Kivity wrote:
> > On 06/07/2011 01:04 AM, Jan Kiszka wrote:
> >> On 2011-06-06 23:48, Alex Williamson wrote:
> >> >  On Mon, 2011-06-06 at 23:30 +0200, Jan Kiszka wrote:
> >> >>  From: Jan Kiszka<jan.kiszka@siemens.com>
> >> >>
> >> >>  At least kernels 2.6.38 and 2.6.39 do not properly support issuing a
> >> >>  reset on an assigned device and corrupt its config space. Prevent
> >> >>  this by checking for a host kernel with the required support,
> >> tagged by
> >> >>  the to-be-introduced KVM_CAP_DEVICE_RESET.
> >> >
> >> >  Wouldn't it be easier just to revert ed78661f in 2.6.39 stable?  I
> >> guess
> >> >  we don't have an option to do that for .38 since stable is done there,
> >> >  but there are also some intel-iommu breakages that won't make
> >> stable for
> >> >  that release.  It seems like the userspace invoked reset resolves
> >> known,
> >> >  demonstrable issues of devices continuing to DMA into guest memory
> >> while
> >> >  ed78661f is mostly a theoretical change.
> >>
> >> Easier would be this patch. But I don't mind reverting the problematic
> >> commit in 39, whatever is preferred. We should just resolve the issue
> >> finally.
> > 
> > Kernel problems should be solved in the kernel (with exceptions of
> > course, but don't see the need here).
> 
> Then please file a revert for stable ASAP.

How's this?  For stable only or course.  Thanks,

Alex

Revert "KVM: Save/restore state of assigned PCI device"

From: Alex Williamson <alex.williamson@redhat.com>

This reverts ed78661f2614d3c9f69c23e280db3bafdabdf5bb as it assumes
the saved PCI state will remain valid for the entire length of time
that it is attached to a guest.  This fails when userspace makes use
of the pci-sysfs reset interface, which invalidates the saved device
state, leaving nothing to be restored after the device is reset on
de-assignment.  This leaves the device in an unusable state.

3.0.0 will add an interface for KVM to save the PCI state in a
buffer unaffected by other callers of pci_reset_function(), but the
most appropriate stable fix seems to be reverting this change since
the original assumption about the device saved state persisting is
incorrect.

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

 virt/kvm/assigned-dev.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)


diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
index ae72ae6..e3f1235 100644
--- a/virt/kvm/assigned-dev.c
+++ b/virt/kvm/assigned-dev.c
@@ -197,8 +197,7 @@ static void kvm_free_assigned_device(struct kvm *kvm,
 {
 	kvm_free_assigned_irq(kvm, assigned_dev);
 
-	__pci_reset_function(assigned_dev->dev);
-	pci_restore_state(assigned_dev->dev);
+	pci_reset_function(assigned_dev->dev);
 
 	pci_release_regions(assigned_dev->dev);
 	pci_disable_device(assigned_dev->dev);
@@ -515,7 +514,6 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	}
 
 	pci_reset_function(dev);
-	pci_save_state(dev);
 
 	match->assigned_dev_id = assigned_dev->assigned_dev_id;
 	match->host_segnr = assigned_dev->segnr;
@@ -546,7 +544,6 @@ out:
 	mutex_unlock(&kvm->lock);
 	return r;
 out_list_del:
-	pci_restore_state(dev);
 	list_del(&match->list);
 	pci_release_regions(dev);
 out_disable:

Re: [PATCH] pci-assign: Do not reset the device unless the kernel supports it

[Jan Kiszka]

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

On 2011-06-07 20:46, Alex Williamson wrote:
> On Tue, 2011-06-07 at 10:14 +0200, Jan Kiszka wrote:
>> On 2011-06-07 10:06, Avi Kivity wrote:
>>> On 06/07/2011 01:04 AM, Jan Kiszka wrote:
>>>> On 2011-06-06 23:48, Alex Williamson wrote:
>>>>>  On Mon, 2011-06-06 at 23:30 +0200, Jan Kiszka wrote:
>>>>>>  From: Jan Kiszka<jan.kiszka@siemens.com>
>>>>>>
>>>>>>  At least kernels 2.6.38 and 2.6.39 do not properly support issuing a
>>>>>>  reset on an assigned device and corrupt its config space. Prevent
>>>>>>  this by checking for a host kernel with the required support,
>>>> tagged by
>>>>>>  the to-be-introduced KVM_CAP_DEVICE_RESET.
>>>>>
>>>>>  Wouldn't it be easier just to revert ed78661f in 2.6.39 stable?  I
>>>> guess
>>>>>  we don't have an option to do that for .38 since stable is done there,
>>>>>  but there are also some intel-iommu breakages that won't make
>>>> stable for
>>>>>  that release.  It seems like the userspace invoked reset resolves
>>>> known,
>>>>>  demonstrable issues of devices continuing to DMA into guest memory
>>>> while
>>>>>  ed78661f is mostly a theoretical change.
>>>>
>>>> Easier would be this patch. But I don't mind reverting the problematic
>>>> commit in 39, whatever is preferred. We should just resolve the issue
>>>> finally.
>>>
>>> Kernel problems should be solved in the kernel (with exceptions of
>>> course, but don't see the need here).
>>
>> Then please file a revert for stable ASAP.
> 
> How's this?  For stable only or course.  Thanks,
> 
> Alex
> 
> Revert "KVM: Save/restore state of assigned PCI device"
> 
> From: Alex Williamson <alex.williamson@redhat.com>
> 
> This reverts ed78661f2614d3c9f69c23e280db3bafdabdf5bb as it assumes
> the saved PCI state will remain valid for the entire length of time
> that it is attached to a guest.  This fails when userspace makes use
> of the pci-sysfs reset interface, which invalidates the saved device
> state, leaving nothing to be restored after the device is reset on
> de-assignment.  This leaves the device in an unusable state.
> 
> 3.0.0 will add an interface for KVM to save the PCI state in a

[ It will be called "3.0". :) ]

> buffer unaffected by other callers of pci_reset_function(), but the
> most appropriate stable fix seems to be reverting this change since
> the original assumption about the device saved state persisting is
> incorrect.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
>  virt/kvm/assigned-dev.c |    5 +----
>  1 files changed, 1 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
> index ae72ae6..e3f1235 100644
> --- a/virt/kvm/assigned-dev.c
> +++ b/virt/kvm/assigned-dev.c
> @@ -197,8 +197,7 @@ static void kvm_free_assigned_device(struct kvm *kvm,
>  {
>  	kvm_free_assigned_irq(kvm, assigned_dev);
>  
> -	__pci_reset_function(assigned_dev->dev);
> -	pci_restore_state(assigned_dev->dev);
> +	pci_reset_function(assigned_dev->dev);
>  
>  	pci_release_regions(assigned_dev->dev);
>  	pci_disable_device(assigned_dev->dev);
> @@ -515,7 +514,6 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
>  	}
>  
>  	pci_reset_function(dev);
> -	pci_save_state(dev);
>  
>  	match->assigned_dev_id = assigned_dev->assigned_dev_id;
>  	match->host_segnr = assigned_dev->segnr;
> @@ -546,7 +544,6 @@ out:
>  	mutex_unlock(&kvm->lock);
>  	return r;
>  out_list_del:
> -	pci_restore_state(dev);
>  	list_del(&match->list);
>  	pci_release_regions(dev);
>  out_disable:
> 
> 
> 

Acked-by: Jan Kiszka <jan.kiszka@siemens.com>

Jan


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

Re: [PATCH] pci-assign: Do not reset the device unless the kernel supports it

[Avi Kivity]

On 06/07/2011 09:46 PM, Alex Williamson wrote:
> Revert "KVM: Save/restore state of assigned PCI device"
>
> From: Alex Williamson<alex.williamson@redhat.com>
>
> This reverts ed78661f2614d3c9f69c23e280db3bafdabdf5bb as it assumes
> the saved PCI state will remain valid for the entire length of time
> that it is attached to a guest.  This fails when userspace makes use
> of the pci-sysfs reset interface, which invalidates the saved device
> state, leaving nothing to be restored after the device is reset on
> de-assignment.  This leaves the device in an unusable state.
>
> 3.0.0 will add an interface for KVM to save the PCI state in a
> buffer unaffected by other callers of pci_reset_function(), but the
> most appropriate stable fix seems to be reverting this change since
> the original assumption about the device saved state persisting is
> incorrect.
>

Thanks, queued for 2.6.39.

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