Re: [PATCH V9 7/9] vfio, platform: make reset driver a requirement by default

2016-07-15 Thread Sinan Kaya 
On 7/14/2016 6:24 PM, Alex Williamson wrote:
>> struct vfio_platform_device {
>> > +  boolreset_required;
>> >struct vfio_platform_region *regions;
>> >u32 num_regions;
>> >struct vfio_platform_irq*irqs;
> Either you have 64bit bools or you're not paying any attention to
> to the alignment of this structure.  If we only care about 64bit
> systems we could tuck this into the gap after num_regions, otherwise
> append it to the end of the structure, we certainly don't care about
> keeping a reset flag within cache line boundaries.  Thanks,

OK, I moved it to the end of the structure.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

Re: [PATCH V9 7/9] vfio, platform: make reset driver a requirement by default

2016-07-15 Thread Sinan Kaya 
On 7/14/2016 6:24 PM, Alex Williamson wrote:
>> struct vfio_platform_device {
>> > +  boolreset_required;
>> >struct vfio_platform_region *regions;
>> >u32 num_regions;
>> >struct vfio_platform_irq*irqs;
> Either you have 64bit bools or you're not paying any attention to
> to the alignment of this structure.  If we only care about 64bit
> systems we could tuck this into the gap after num_regions, otherwise
> append it to the end of the structure, we certainly don't care about
> keeping a reset flag within cache line boundaries.  Thanks,

OK, I moved it to the end of the structure.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

Re: [PATCH V9 7/9] vfio, platform: make reset driver a requirement by default

2016-07-14 Thread Alex Williamson 
On Wed, 13 Jul 2016 22:06:33 -0400
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 platform VFIO drivers.
> The default value is true for the DT and ACPI based drivers.
> The reset requirement value for AMBA drivers is set to false and is
> unchangeable to maintain the existing functionality.
> 
> 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 
> ---
>  drivers/vfio/platform/vfio_amba.c |  1 +
>  drivers/vfio/platform/vfio_platform.c |  5 +
>  drivers/vfio/platform/vfio_platform_common.c  | 15 +++
>  drivers/vfio/platform/vfio_platform_private.h |  1 +
>  4 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_amba.c 
> b/drivers/vfio/platform/vfio_amba.c
> index a66479b..31372fb 100644
> --- a/drivers/vfio/platform/vfio_amba.c
> +++ b/drivers/vfio/platform/vfio_amba.c
> @@ -68,6 +68,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 = false;
>  
>   ret = vfio_platform_probe_common(vdev, >dev);
>   if (ret) {
> diff --git a/drivers/vfio/platform/vfio_platform.c 
> b/drivers/vfio/platform/vfio_platform.c
> index b1cc3a7..6561751 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, 0444);
> +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, >dev);
>   if (ret)
> diff --git a/drivers/vfio/platform/vfio_platform_common.c 
> b/drivers/vfio/platform/vfio_platform_common.c
> index 2d9c905..71248a9 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -123,10 +123,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 (VFIO_PLATFORM_IS_ACPI(vdev))
> - return;
> + return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT;
>  
>   vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>   >reset_module);
> @@ -135,6 +135,8 @@ static void vfio_platform_get_reset(struct 
> vfio_platform_device *vdev)
>   vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>   >reset_module);
>   }
> +
> + return vdev->of_reset ? 0 : -ENOENT;
>  }
>  
>  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
> @@ -675,6 +677,13 @@ int vfio_platform_probe_common(struct 
> vfio_platform_device *vdev,
>  
>   vdev->device = dev;
>  
> + 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) {
>   pr_err("VFIO: No IOMMU group for device %s\n", vdev->name);
> @@ -687,8 +696,6 @@ int vfio_platform_probe_common(struct 
> vfio_platform_device *vdev,
>   return ret;
>   }
>  
> - vfio_platform_get_reset(vdev);
> -
>   mutex_init(>igate);
>  
>   return 0;
> diff --git a/drivers/vfio/platform/vfio_platform_private.h 
> b/drivers/vfio/platform/vfio_platform_private.h
> index ba9e4f8..68fbc00 100644
> ---

Re: [PATCH V9 7/9] vfio, platform: make reset driver a requirement by default

2016-07-14 Thread Alex Williamson 
On Wed, 13 Jul 2016 22:06:33 -0400
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 platform VFIO drivers.
> The default value is true for the DT and ACPI based drivers.
> The reset requirement value for AMBA drivers is set to false and is
> unchangeable to maintain the existing functionality.
> 
> 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 
> ---
>  drivers/vfio/platform/vfio_amba.c |  1 +
>  drivers/vfio/platform/vfio_platform.c |  5 +
>  drivers/vfio/platform/vfio_platform_common.c  | 15 +++
>  drivers/vfio/platform/vfio_platform_private.h |  1 +
>  4 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vfio/platform/vfio_amba.c 
> b/drivers/vfio/platform/vfio_amba.c
> index a66479b..31372fb 100644
> --- a/drivers/vfio/platform/vfio_amba.c
> +++ b/drivers/vfio/platform/vfio_amba.c
> @@ -68,6 +68,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 = false;
>  
>   ret = vfio_platform_probe_common(vdev, >dev);
>   if (ret) {
> diff --git a/drivers/vfio/platform/vfio_platform.c 
> b/drivers/vfio/platform/vfio_platform.c
> index b1cc3a7..6561751 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, 0444);
> +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, >dev);
>   if (ret)
> diff --git a/drivers/vfio/platform/vfio_platform_common.c 
> b/drivers/vfio/platform/vfio_platform_common.c
> index 2d9c905..71248a9 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -123,10 +123,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 (VFIO_PLATFORM_IS_ACPI(vdev))
> - return;
> + return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT;
>  
>   vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>   >reset_module);
> @@ -135,6 +135,8 @@ static void vfio_platform_get_reset(struct 
> vfio_platform_device *vdev)
>   vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>   >reset_module);
>   }
> +
> + return vdev->of_reset ? 0 : -ENOENT;
>  }
>  
>  static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
> @@ -675,6 +677,13 @@ int vfio_platform_probe_common(struct 
> vfio_platform_device *vdev,
>  
>   vdev->device = dev;
>  
> + 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) {
>   pr_err("VFIO: No IOMMU group for device %s\n", vdev->name);
> @@ -687,8 +696,6 @@ int vfio_platform_probe_common(struct 
> vfio_platform_device *vdev,
>   return ret;
>   }
>  
> - vfio_platform_get_reset(vdev);
> -
>   mutex_init(>igate);
>  
>   return 0;
> 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
> +++

[PATCH V9 7/9] vfio, platform: make reset driver a requirement by default

2016-07-13 Thread Sinan Kaya 
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 platform VFIO drivers.
The default value is true for the DT and ACPI based drivers.
The reset requirement value for AMBA drivers is set to false and is
unchangeable to maintain the existing functionality.

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 
---
 drivers/vfio/platform/vfio_amba.c |  1 +
 drivers/vfio/platform/vfio_platform.c |  5 +
 drivers/vfio/platform/vfio_platform_common.c  | 15 +++
 drivers/vfio/platform/vfio_platform_private.h |  1 +
 4 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/vfio/platform/vfio_amba.c 
b/drivers/vfio/platform/vfio_amba.c
index a66479b..31372fb 100644
--- a/drivers/vfio/platform/vfio_amba.c
+++ b/drivers/vfio/platform/vfio_amba.c
@@ -68,6 +68,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 = false;
 
ret = vfio_platform_probe_common(vdev, >dev);
if (ret) {
diff --git a/drivers/vfio/platform/vfio_platform.c 
b/drivers/vfio/platform/vfio_platform.c
index b1cc3a7..6561751 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, 0444);
+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, >dev);
if (ret)
diff --git a/drivers/vfio/platform/vfio_platform_common.c 
b/drivers/vfio/platform/vfio_platform_common.c
index 2d9c905..71248a9 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -123,10 +123,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 (VFIO_PLATFORM_IS_ACPI(vdev))
-   return;
+   return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT;
 
vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>reset_module);
@@ -135,6 +135,8 @@ static void vfio_platform_get_reset(struct 
vfio_platform_device *vdev)
vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>reset_module);
}
+
+   return vdev->of_reset ? 0 : -ENOENT;
 }
 
 static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
@@ -675,6 +677,13 @@ int vfio_platform_probe_common(struct vfio_platform_device 
*vdev,
 
vdev->device = dev;
 
+   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) {
pr_err("VFIO: No IOMMU group for device %s\n", vdev->name);
@@ -687,8 +696,6 @@ int vfio_platform_probe_common(struct vfio_platform_device 
*vdev,
return ret;
}
 
-   vfio_platform_get_reset(vdev);
-
mutex_init(>igate);
 
return 0;
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 {
+   boolreset_required;
struct

[PATCH V9 7/9] vfio, platform: make reset driver a requirement by default

2016-07-13 Thread Sinan Kaya 
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 platform VFIO drivers.
The default value is true for the DT and ACPI based drivers.
The reset requirement value for AMBA drivers is set to false and is
unchangeable to maintain the existing functionality.

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 
---
 drivers/vfio/platform/vfio_amba.c |  1 +
 drivers/vfio/platform/vfio_platform.c |  5 +
 drivers/vfio/platform/vfio_platform_common.c  | 15 +++
 drivers/vfio/platform/vfio_platform_private.h |  1 +
 4 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/vfio/platform/vfio_amba.c 
b/drivers/vfio/platform/vfio_amba.c
index a66479b..31372fb 100644
--- a/drivers/vfio/platform/vfio_amba.c
+++ b/drivers/vfio/platform/vfio_amba.c
@@ -68,6 +68,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 = false;
 
ret = vfio_platform_probe_common(vdev, >dev);
if (ret) {
diff --git a/drivers/vfio/platform/vfio_platform.c 
b/drivers/vfio/platform/vfio_platform.c
index b1cc3a7..6561751 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, 0444);
+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, >dev);
if (ret)
diff --git a/drivers/vfio/platform/vfio_platform_common.c 
b/drivers/vfio/platform/vfio_platform_common.c
index 2d9c905..71248a9 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -123,10 +123,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 (VFIO_PLATFORM_IS_ACPI(vdev))
-   return;
+   return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT;
 
vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>reset_module);
@@ -135,6 +135,8 @@ static void vfio_platform_get_reset(struct 
vfio_platform_device *vdev)
vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>reset_module);
}
+
+   return vdev->of_reset ? 0 : -ENOENT;
 }
 
 static void vfio_platform_put_reset(struct vfio_platform_device *vdev)
@@ -675,6 +677,13 @@ int vfio_platform_probe_common(struct vfio_platform_device 
*vdev,
 
vdev->device = dev;
 
+   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) {
pr_err("VFIO: No IOMMU group for device %s\n", vdev->name);
@@ -687,8 +696,6 @@ int vfio_platform_probe_common(struct vfio_platform_device 
*vdev,
return ret;
}
 
-   vfio_platform_get_reset(vdev);
-
mutex_init(>igate);
 
return 0;
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 {
+   boolreset_required;
struct vfio_platform_region *regions;
u32