Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1420438 > unrolled thread
| Started by | Sinan Kaya <okaya@codeaurora.org> |
|---|---|
| First post | 2016-06-13 06:30 +0200 |
| Last post | 2016-06-16 19:10 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH V7 7/9] vfio, platform: make reset driver a requirement by default Sinan Kaya <okaya@codeaurora.org> - 2016-06-13 06:30 +0200
Re: [PATCH V7 7/9] vfio, platform: make reset driver a requirement by default Auger Eric <eric.auger@redhat.com> - 2016-06-16 10:20 +0200
Re: [PATCH V7 7/9] vfio, platform: make reset driver a requirement by default Sinan Kaya <okaya@codeaurora.org> - 2016-06-16 19:10 +0200
| From | Sinan Kaya <okaya@codeaurora.org> |
|---|---|
| Date | 2016-06-13 06:30 +0200 |
| Subject | [PATCH V7 7/9] vfio, platform: make reset driver a requirement by default |
| Message-ID | <rJrFD-1gJ-19@gated-at.bofh.it> |
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 <okaya@codeaurora.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
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 <a.motakis@virtualopensystems.com>"
#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 <a.motakis@virtualopensystems.com>"
#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;
+ }
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;
--
1.8.2.1
[toc] | [next] | [standalone]
| From | Auger Eric <eric.auger@redhat.com> |
|---|---|
| Date | 2016-06-16 10:20 +0200 |
| Subject | Re: [PATCH V7 7/9] vfio, platform: make reset driver a requirement by default |
| Message-ID | <rKAGR-5Qj-7@gated-at.bofh.it> |
| In reply to | #1420438 |
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 <okaya@codeaurora.org>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> ---
> 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 <a.motakis@virtualopensystems.com>"
> #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 <a.motakis@virtualopensystems.com>"
> #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(-)
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);
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;
>
[toc] | [prev] | [next] | [standalone]
| From | Sinan Kaya <okaya@codeaurora.org> |
|---|---|
| Date | 2016-06-16 19:10 +0200 |
| Subject | Re: [PATCH V7 7/9] vfio, platform: make reset driver a requirement by default |
| Message-ID | <rKIXL-2Gg-3@gated-at.bofh.it> |
| In reply to | #1423778 |
On 6/16/2016 4:15 AM, Auger Eric wrote:
> 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.
>
Thanks for testing. I'll post v8 with this change.
> Best Regards
>
> Eric
>
> ---
> drivers/vfio/platform/vfio_platform_common.c | 26
> ++++++++++++--------------
> 1 file changed, 12 insertions(+), 14 deletions(-)
>
> 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);
> 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;
>>
--
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web