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

[Alex Williamson <alex.williamson@redhat.com>]

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