From patchwork Thu Jun 16 08:15:57 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Auger X-Patchwork-Id: 9179979 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 7DC7660573 for ; Thu, 16 Jun 2016 08:17:49 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 6ADD126B39 for ; Thu, 16 Jun 2016 08:17:49 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 5D75727F07; Thu, 16 Jun 2016 08:17:49 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id E773D26B39 for ; Thu, 16 Jun 2016 08:17:48 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1bDSTS-0006ZD-TW; Thu, 16 Jun 2016 08:16:26 +0000 Received: from mx1.redhat.com ([209.132.183.28]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1bDSTO-00065b-Qc for linux-arm-kernel@lists.infradead.org; Thu, 16 Jun 2016 08:16:24 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A47967F0B3; Thu, 16 Jun 2016 08:16:01 +0000 (UTC) Received: from localhost.localdomain (vpn1-4-111.ams2.redhat.com [10.36.4.111]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u5G8FvJ5018777 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 16 Jun 2016 04:15:58 -0400 Subject: Re: [PATCH V7 7/9] vfio, platform: make reset driver a requirement by default To: Sinan Kaya , kvm@vger.kernel.org, timur@codeaurora.org, cov@codeaurora.org, jcm@redhat.com, alex.williamson@redhat.com, eric.auger@linaro.org References: <1465792001-27960-1-git-send-email-okaya@codeaurora.org> <1465792001-27960-8-git-send-email-okaya@codeaurora.org> From: Auger Eric Message-ID: <8eaecc6e-aece-e859-281a-b02e2b6eced7@redhat.com> Date: Thu, 16 Jun 2016 10:15:57 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: <1465792001-27960-8-git-send-email-okaya@codeaurora.org> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Thu, 16 Jun 2016 08:16:02 +0000 (UTC) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160616_011622_933449_CE25E69D X-CRM114-Status: GOOD ( 26.97 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Baptiste Reynal , linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org, agross@codeaurora.org, linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Sinan, On 13/06/2016 06:26, Sinan Kaya wrote: > The code was allowing platform devices to be used without a supporting > VFIO reset driver. The hardware can be left in some inconsistent state > after a guest machine abort. > > The reset driver will put the hardware back to safe state and disable > interrupts before returning the control back to the host machine. > > Adding a new reset_required kernel module option to AMBA and platform > VFIO drivers with a default value of true. > > New requirements are: > 1. A reset function needs to be implemented by the corresponding driver > via DT/ACPI. > 2. The reset function needs to be discovered via DT/ACPI. > > The probe of the driver will fail if any of the above conditions are > not satisfied. > > Signed-off-by: Sinan Kaya > Reviewed-by: Eric Auger > --- > drivers/vfio/platform/vfio_amba.c | 5 +++++ > drivers/vfio/platform/vfio_platform.c | 5 +++++ > drivers/vfio/platform/vfio_platform_common.c | 22 +++++++++++++++------- > drivers/vfio/platform/vfio_platform_private.h | 1 + > 4 files changed, 26 insertions(+), 7 deletions(-) > > diff --git a/drivers/vfio/platform/vfio_amba.c b/drivers/vfio/platform/vfio_amba.c > index a66479b..7585902 100644 > --- a/drivers/vfio/platform/vfio_amba.c > +++ b/drivers/vfio/platform/vfio_amba.c > @@ -23,6 +23,10 @@ > #define DRIVER_AUTHOR "Antonios Motakis " > #define DRIVER_DESC "VFIO for AMBA devices - User Level meta-driver" > > +static bool reset_required = true; > +module_param(reset_required, bool, 0644); > +MODULE_PARM_DESC(reset_required, "override reset requirement (default: 1)"); > + > /* probing devices from the AMBA bus */ > > static struct resource *get_amba_resource(struct vfio_platform_device *vdev, > @@ -68,6 +72,7 @@ static int vfio_amba_probe(struct amba_device *adev, const struct amba_id *id) > vdev->get_resource = get_amba_resource; > vdev->get_irq = get_amba_irq; > vdev->parent_module = THIS_MODULE; > + vdev->reset_required = reset_required; > > ret = vfio_platform_probe_common(vdev, &adev->dev); > if (ret) { > diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c > index b1cc3a7..ef89146 100644 > --- a/drivers/vfio/platform/vfio_platform.c > +++ b/drivers/vfio/platform/vfio_platform.c > @@ -23,6 +23,10 @@ > #define DRIVER_AUTHOR "Antonios Motakis " > #define DRIVER_DESC "VFIO for platform devices - User Level meta-driver" > > +static bool reset_required = true; > +module_param(reset_required, bool, 0644); > +MODULE_PARM_DESC(reset_required, "override reset requirement (default: 1)"); > + > /* probing devices from the linux platform bus */ > > static struct resource *get_platform_resource(struct vfio_platform_device *vdev, > @@ -66,6 +70,7 @@ static int vfio_platform_probe(struct platform_device *pdev) > vdev->get_resource = get_platform_resource; > vdev->get_irq = get_platform_irq; > vdev->parent_module = THIS_MODULE; > + vdev->reset_required = reset_required; > > ret = vfio_platform_probe_common(vdev, &pdev->dev); > if (ret) > diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c > index 0ea8c26..d84c399 100644 > --- a/drivers/vfio/platform/vfio_platform_common.c > +++ b/drivers/vfio/platform/vfio_platform_common.c > @@ -128,10 +128,10 @@ static bool vfio_platform_has_reset(struct vfio_platform_device *vdev) > return vdev->of_reset ? true : false; > } > > -static void vfio_platform_get_reset(struct vfio_platform_device *vdev) > +static int vfio_platform_get_reset(struct vfio_platform_device *vdev) > { > if (vdev->acpihid) > - return; > + return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT; > > vdev->of_reset = vfio_platform_lookup_reset(vdev->compat, > &vdev->reset_module); > @@ -140,6 +140,8 @@ static void vfio_platform_get_reset(struct vfio_platform_device *vdev) > vdev->of_reset = vfio_platform_lookup_reset(vdev->compat, > &vdev->reset_module); > } > + > + return vdev->of_reset ? 0 : -ENOENT; > } > > static void vfio_platform_put_reset(struct vfio_platform_device *vdev) > @@ -670,16 +672,22 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev, > } > > ret = vfio_add_group_dev(dev, &vfio_platform_ops, vdev); > - if (ret) { > - iommu_group_put(group); > - return ret; > - } > + if (ret) > + goto out; > > - vfio_platform_get_reset(vdev); > + ret = vfio_platform_get_reset(vdev); > + if (ret && vdev->reset_required) { > + pr_err("vfio: no reset function found for device %s\n", > + vdev->name); > + goto out; > + } Unfortunately this causes a crash when the reset module is not available. You should do the vfio_add_group_dev at the end. Something like below. Best Regards Eric --- drivers/vfio/platform/vfio_platform_common.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) if (!group) { @@ -672,22 +680,12 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev, } ret = vfio_add_group_dev(dev, &vfio_platform_ops, vdev); - if (ret) - goto out; - - ret = vfio_platform_get_reset(vdev); - if (ret && vdev->reset_required) { - pr_err("vfio: no reset function found for device %s\n", - vdev->name); - goto out; - } - - mutex_init(&vdev->igate); + if (ret) { + iommu_group_put(group); + return ret; + } return 0; -out: - iommu_group_put(group); - return ret; } EXPORT_SYMBOL_GPL(vfio_platform_probe_common); > > mutex_init(&vdev->igate); > > return 0; > +out: > + iommu_group_put(group); > + return ret; > } > EXPORT_SYMBOL_GPL(vfio_platform_probe_common); > > diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h > index ba9e4f8..68fbc00 100644 > --- a/drivers/vfio/platform/vfio_platform_private.h > +++ b/drivers/vfio/platform/vfio_platform_private.h > @@ -50,6 +50,7 @@ struct vfio_platform_region { > }; > > struct vfio_platform_device { > + bool reset_required; > struct vfio_platform_region *regions; > u32 num_regions; > struct vfio_platform_irq *irqs; > diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c index d84c399..0928f7d 100644 --- a/drivers/vfio/platform/vfio_platform_common.c +++ b/drivers/vfio/platform/vfio_platform_common.c @@ -664,6 +664,14 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev, return ret; vdev->device = dev; + mutex_init(&vdev->igate); + + ret = vfio_platform_get_reset(vdev); + if (ret && vdev->reset_required) { + pr_err("vfio: no reset function found for device %s\n", + vdev->name); + return ret; + } group = iommu_group_get(dev);