From patchwork Thu Dec 4 12:24:47 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Vrabel X-Patchwork-Id: 5437961 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: X-Original-To: patchwork-linux-pci@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 348FEBEEA8 for ; Thu, 4 Dec 2014 12:25:33 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 6E4C820253 for ; Thu, 4 Dec 2014 12:25:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 5B1F920218 for ; Thu, 4 Dec 2014 12:25:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753574AbaLDMZZ (ORCPT ); Thu, 4 Dec 2014 07:25:25 -0500 Received: from smtp02.citrix.com ([66.165.176.63]:3501 "EHLO SMTP02.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753248AbaLDMZY (ORCPT ); Thu, 4 Dec 2014 07:25:24 -0500 X-IronPort-AV: E=Sophos;i="5.07,515,1413244800"; d="scan'208,223";a="200035145" Message-ID: <5480528F.8010106@citrix.com> Date: Thu, 4 Dec 2014 12:24:47 +0000 From: David Vrabel User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.2.0 MIME-Version: 1.0 To: Konrad Rzeszutek Wilk , David Vrabel CC: , , Boris Ostrovsky , , Subject: Re: [Xen-devel] [PATCH v5 9/9] xen/pciback: Implement PCI reset slot or bus with 'do_flr' SysFS attribute References: <201412041206.sB4C6XVQ009497@userz7022.oracle.com> In-Reply-To: <201412041206.sB4C6XVQ009497@userz7022.oracle.com> X-DLP: MIA2 Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, T_TVD_MIME_EPI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 04/12/14 12:06, Konrad Rzeszutek Wilk wrote: > > On Dec 4, 2014 6:30 AM, David Vrabel wrote: >> >> On 03/12/14 21:40, Konrad Rzeszutek Wilk wrote: >>> >>> Instead of doing all this complex dance, we depend on the toolstack >>> doing the right thing. As such implement the 'do_flr' SysFS attribute >>> which 'xl' uses when a device is detached or attached from/to a guest. >>> It bypasses the need to worry about the PCI lock. >> >> No. Get pciback to add its own "reset" sysfs file (as I have repeatedly >> proposed). >> > > Which does not work as the kobj will complain (as there is already an 'reset' associated with the PCI device). It is only needed if the core won't provide one. +static int pcistub_try_create_reset_file(struct pci_dev *pci) +{ + struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(pci); + struct device *dev = &pci->dev; + int ret; + + /* Already have a per-function reset? */ + if (pci_probe_reset_function(pci) == 0) + return 0; + + ret = device_create_file(dev, &dev_attr_reset); + if (ret < 0) + return ret; + dev_data->created_reset_file = true; + return 0; +} David From e65a814106bc9994ec47b5317f7cdc975270d27f Mon Sep 17 00:00:00 2001 From: David Vrabel Date: Thu, 10 Jul 2014 13:09:25 +0100 Subject: [PATCH 2/2] xen-pciback: provide a "reset" sysfs file to try harder at an SBR The standard implementation of pci_reset_function() does not try an SBR if there are other sibling devices. This is a common configuration for multi-function devices (e.g., GPUs with a HDMI audio device function). If all the functions are co-assigned to the same domain at the same time, then it is safe to perform an SBR to reset all functions at the same time. Add a "reset" sysfs file with the same interface as the standard one (i.e., write 1 to reset) that will try an SBR if all sibling devices are unbound or bound to pciback. Note that this is weaker than the requirement for functions to be simultaneously co-assigned, but the toolstack is expected to ensure this. Signed-off-by: David Vrabel diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c index 4e8ba38..d6c29aa 100644 --- a/drivers/xen/xen-pciback/pci_stub.c +++ b/drivers/xen/xen-pciback/pci_stub.c @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -60,6 +61,114 @@ static LIST_HEAD(pcistub_devices); static int initialize_devices; static LIST_HEAD(seized_devices); +/* + * pci_reset_function() will only work if there is a mechanism to + * reset that single function (e.g., FLR or a D-state transition). + * For PCI hardware that has two or more functions but no per-function + * reset, we can do a bus reset iff all the functions are co-assigned + * to the same domain. + * + * If a function has no per-function reset mechanism the 'reset' sysfs + * file that the toolstack uses to reset a function prior to assigning + * the device will be missing. In this case, pciback adds its own + * which will try a bus reset. + * + * Note: pciback does not check for co-assigment before doing a bus + * reset, only that the devices are bound to pciback. The toolstack + * is assumed to have done the right thing. + */ +static int __pcistub_reset_function(struct pci_dev *dev) +{ + struct pci_dev *pdev; + u16 ctrl; + int ret; + + ret = __pci_reset_function_locked(dev); + if (ret == 0) + return 0; + + if (pci_is_root_bus(dev->bus) || dev->subordinate || !dev->bus->self) + return -ENOTTY; + + list_for_each_entry(pdev, &dev->bus->devices, bus_list) { + if (pdev != dev && (!pdev->driver + || strcmp(pdev->driver->name, "pciback"))) + return -ENOTTY; + pci_save_state(pdev); + } + + pci_read_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, &ctrl); + ctrl |= PCI_BRIDGE_CTL_BUS_RESET; + pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl); + msleep(200); + + ctrl &= ~PCI_BRIDGE_CTL_BUS_RESET; + pci_write_config_word(dev->bus->self, PCI_BRIDGE_CONTROL, ctrl); + msleep(200); + + list_for_each_entry(pdev, &dev->bus->devices, bus_list) + pci_restore_state(pdev); + + return 0; +} + +static int pcistub_reset_function(struct pci_dev *dev) +{ + int ret; + + device_lock(&dev->dev); + ret = __pcistub_reset_function(dev); + device_unlock(&dev->dev); + + return ret; +} + +static ssize_t pcistub_reset_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct pci_dev *pdev = to_pci_dev(dev); + unsigned long val; + ssize_t result = strict_strtoul(buf, 0, &val); + + if (result < 0) + return result; + + if (val != 1) + return -EINVAL; + + result = pcistub_reset_function(pdev); + if (result < 0) + return result; + return count; +} +static DEVICE_ATTR(reset, 0200, NULL, pcistub_reset_store); + +static int pcistub_try_create_reset_file(struct pci_dev *pci) +{ + struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(pci); + struct device *dev = &pci->dev; + int ret; + + /* Already have a per-function reset? */ + if (pci_probe_reset_function(pci) == 0) + return 0; + + ret = device_create_file(dev, &dev_attr_reset); + if (ret < 0) + return ret; + dev_data->created_reset_file = true; + return 0; +} + +static void pcistub_remove_reset_file(struct pci_dev *pci) +{ + struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(pci); + + if (dev_data && dev_data->created_reset_file) + device_remove_file(&pci->dev, &dev_attr_reset); +} + static struct pcistub_device *pcistub_device_alloc(struct pci_dev *dev) { struct pcistub_device *psdev; @@ -100,7 +209,8 @@ static void pcistub_device_release(struct kref *kref) /* Call the reset function which does not take lock as this * is called from "unbind" which takes a device_lock mutex. */ - __pci_reset_function_locked(dev); + __pcistub_reset_function(psdev->dev); + if (pci_load_and_free_saved_state(dev, &dev_data->pci_saved_state)) dev_dbg(&dev->dev, "Could not reload PCI state\n"); else @@ -268,7 +378,7 @@ void pcistub_put_pci_dev(struct pci_dev *dev) /* This is OK - we are running from workqueue context * and want to inhibit the user from fiddling with 'reset' */ - pci_reset_function(dev); + pcistub_reset_function(dev); pci_restore_state(psdev->dev); /* This disables the device. */ @@ -392,7 +502,7 @@ static int pcistub_init_device(struct pci_dev *dev) dev_err(&dev->dev, "Could not store PCI conf saved state!\n"); else { dev_dbg(&dev->dev, "resetting (FLR, D3, etc) the device\n"); - __pci_reset_function_locked(dev); + __pcistub_reset_function(dev); pci_restore_state(dev); } /* Now disable the device (this also ensures some private device @@ -401,6 +511,10 @@ static int pcistub_init_device(struct pci_dev *dev) dev_dbg(&dev->dev, "reset device\n"); xen_pcibk_reset_device(dev); + err = pcistub_try_create_reset_file(dev); + if (err < 0) + goto config_release; + dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED; return 0; @@ -526,6 +640,8 @@ static void pcistub_remove(struct pci_dev *dev) dev_dbg(&dev->dev, "removing\n"); + pcistub_remove_reset_file(dev); + spin_lock_irqsave(&pcistub_devices_lock, flags); xen_pcibk_config_quirk_release(dev); diff --git a/drivers/xen/xen-pciback/pciback.h b/drivers/xen/xen-pciback/pciback.h index f72af87..708ade9 100644 --- a/drivers/xen/xen-pciback/pciback.h +++ b/drivers/xen/xen-pciback/pciback.h @@ -42,6 +42,7 @@ struct xen_pcibk_device { struct xen_pcibk_dev_data { struct list_head config_fields; struct pci_saved_state *pci_saved_state; + bool created_reset_file; unsigned int permissive:1; unsigned int warned_on_write:1; unsigned int enable_intx:1;