pci-assign: Do not reset the device unless the kernel supports it
PATCH 1/1 pci-assign: Do not reset the device unless the kernel supports it
From: Jan Kiszka <[email protected]>
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.
---
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(-)
hw/device-assignment.c +19 -14
@@ -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
Quoted 6 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
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.
Quoted 4 lines
Signed-off-by Jan Kiszka <[email protected]>---
PS: What's the state of those KVM patches? Will they make it into 3.0?
The PCI save/restore ones are in:
f8fcfd775523347afe460dc3a0f45d0479e784a2ffbdd3f7931fb7cb7e36d00d16303ec433be514524a4742f0be6226eb0106fbb17caf4d711d1ad43
Thanks,
Alex
Quoted 58 lines
hw/device-assignment.c | 33 +++++++++++++++++++-------------- 1 files changed, 19 insertions(+), 14 deletions(-)hw/device-assignment.c +19
-15@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 8 lines · Alex Williamson
Quoted 6 lines · Jan Kiszka
From: Jan Kiszka <[email protected]>
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.
Quoted 6 lines
Wouldn't it be easier just to revert
ed78661fin 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 whileed78661fis 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.
Quoted 11 lines
Quoted 5 lines
Signed-off-by Jan Kiszka <[email protected]>---
PS: What's the state of those KVM patches? Will they make it into 3.0?
The PCI save/restore ones are in:
f8fcfd775523347afe460dc3a0f45d0479e784a2ffbdd3f7931fb7cb7e36d00d16303ec433be514524a4742f0be6226eb0106fbb17caf4d711d1ad43
Oh, they are just missing in kvm.git so far.
Jan
Quoted 16 lines · Jan Kiszka
Quoted 8 lines · Alex Williamson
Quoted 6 lines · Jan Kiszka
From: Jan Kiszka<[email protected]>
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.Quoted 6 lines
Wouldn't it be easier just to revert
ed78661fin 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 whileed78661fis mostly a theoretical change.
Quoted 3 lines
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).
Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.
Quoted 20 lines · Avi Kivity
Quoted 19 lines · Jan Kiszka
Quoted 6 lines · Alex Williamson
Quoted 5 lines · Jan Kiszka
From: Jan Kiszka<[email protected]>
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
Quoted 3 lines
Quoted 1 line
the to-be-introduced
KVM_CAP_DEVICE_RESET.Wouldn't it be easier just to revert
ed78661fin 2.6.39 stable? Iguess
Quoted 2 lines
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
Quoted 1 line
that release. It seems like the userspace invoked reset resolves
known,
Quoted 1 line
demonstrable issues of devices continuing to DMA into guest memory
while
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
ed78661fis 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
Quoted 23 lines · Jan Kiszka
Quoted 22 lines · Avi Kivity
Quoted 21 lines · Jan Kiszka
Quoted 6 lines · Alex Williamson
Quoted 5 lines · Jan Kiszka
From: Jan Kiszka<[email protected]>
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
Quoted 3 lines
Quoted 1 line
the to-be-introduced
KVM_CAP_DEVICE_RESET.Wouldn't it be easier just to revert
ed78661fin 2.6.39 stable? Iguess
Quoted 2 lines
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
Quoted 1 line
that release. It seems like the userspace invoked reset resolves
known,
Quoted 1 line
demonstrable issues of devices continuing to DMA into guest memory
while
Quoted 1 line
ed78661fis mostly a theoretical change.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
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 <[email protected]>
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.
---
virt/kvm/assigned-dev.c | 5 +---- 1 files changed, 1 insertions(+), 4 deletions(-)
virt/kvm/assigned-dev.c +1 -4
@@ -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:
Jan Kiszka
Acked-byQuoted 41 lines · Alex Williamson
Quoted 23 lines · Jan Kiszka
Quoted 22 lines · Avi Kivity
Quoted 21 lines · Jan Kiszka
Quoted 6 lines · Alex Williamson
Quoted 5 lines · Jan Kiszka
From: Jan Kiszka<[email protected]>
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
Quoted 3 lines
Quoted 1 line
the to-be-introduced
KVM_CAP_DEVICE_RESET.Wouldn't it be easier just to revert
ed78661fin 2.6.39 stable? Iguess
Quoted 2 lines
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
Quoted 1 line
that release. It seems like the userspace invoked reset resolves
known,
Quoted 1 line
demonstrable issues of devices continuing to DMA into guest memory
while
Quoted 1 line
ed78661fis mostly a theoretical change.Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
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 <[email protected]>
Quoted 8 lines
This reverts
ed78661f2614d3c9f69c23e280db3bafdabdf5bbas 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". :) ]
Quoted 38 lines
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 <[email protected]>---
virt/kvm/assigned-dev.c | 5 +---- 1 files changed, 1 insertions(+), 4 deletions(-)virt/kvm/assigned-dev.c +1
-3@@ -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;
virt/kvm/assigned-dev.c out: · line 549
Quoted 7 lines
virt/kvm/assigned-dev.c +0
-1@@ -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:
Jan
Quoted 11 lines · Alex Williamson
Revert "KVM: Save/restore state of assigned PCI device"
From: Alex Williamson<[email protected]>
This reverts
ed78661f2614d3c9f69c23e280db3bafdabdf5bbas 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.
Quoted 6 lines
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.
Signature · 1 line
error compiling committee.c: too many arguments to function