Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1356059 > unrolled thread

[PATCH V2 2/3] vfio, platform: make reset driver a requirement

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.

Contents

  [PATCH V2 2/3] vfio, platform: make reset driver a requirement Sinan Kaya <okaya@codeaurora.org> - 2016-03-11 18:00 +0100
    Re: [PATCH V2 2/3] vfio, platform: make reset driver a requirement Sinan Kaya <okaya@codeaurora.org> - 2016-03-13 18:30 +0100
      Re: [PATCH V2 2/3] vfio, platform: make reset driver a requirement Eric Auger <eric.auger@linaro.org> - 2016-03-16 05:40 +0100
        Re: [PATCH V2 2/3] vfio, platform: make reset driver a requirement Sinan Kaya <okaya@codeaurora.org> - 2016-03-16 15:30 +0100

#1356059 — [PATCH V2 2/3] vfio, platform: make reset driver a requirement

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.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/vfio/platform/vfio_platform_common.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
index b3f6ba2..5535fe1 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -165,12 +165,8 @@ static void vfio_platform_release(void *device_data)
 	mutex_lock(&driver_lock);
 
 	if (!(--vdev->refcnt)) {
-		if (vdev->reset) {
-			dev_info(vdev->device, "reset\n");
-			vdev->reset(vdev);
-		} else {
-			dev_warn(vdev->device, "no reset function found!\n");
-		}
+		dev_info(vdev->device, "reset\n");
+		vdev->reset(vdev);
 		vfio_platform_regions_cleanup(vdev);
 		vfio_platform_irq_cleanup(vdev);
 	}
@@ -634,7 +630,13 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
 		return ret;
 	}
 
-	vfio_platform_get_reset(vdev);
+	ret = vfio_platform_get_reset(vdev);
+	if (ret) {
+		pr_err("vfio: no reset function found for device %s\n",
+		       vdev->name);
+		iommu_group_put(group);
+		return ret;
+	}
 	mutex_init(&vdev->igate);
 
 	return 0;
-- 
1.8.2.1

[toc] | [next] | [standalone]

#1356704

On 3/11/2016 11:54 AM, 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.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/vfio/platform/vfio_platform_common.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
I was looking at the code. It looks like this is going to break VFIO AMBA. The
common implementation is shared with AMBA and platform devices.

I couldn't see a reset function for AMBA devices.

Is there anyway to write reset function for it? I have no experience with 
AMBA devices.

Would you include a reset needed flag and just not require it for AMBA? 
(I honestly don't like this idea)

-- 
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] | [next] | [standalone]

#1358519

Hi Sinan,
On 03/13/2016 06:25 PM, Sinan Kaya wrote:
> On 3/11/2016 11:54 AM, 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.
>>
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> ---
>>  drivers/vfio/platform/vfio_platform_common.c | 16 +++++++++-------
>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>
> I was looking at the code. It looks like this is going to break VFIO AMBA. The
> common implementation is shared with AMBA and platform devices.
> 
> I couldn't see a reset function for AMBA devices.
> 
> Is there anyway to write reset function for it? I have no experience with 
> AMBA devices.
To my knowledge only the PL330 DMA controller (drivers/dma/pl330.c) was
passthrough'ed at some point, rather for development purpose than
production purpose. This was done by Virtual Open Systems (ask Baptiste
for more details). But I don't think this is really used.
> 
> Would you include a reset needed flag and just not require it for AMBA? 
> (I honestly don't like this idea)
> 
I think the requirement also makes sense for AMBA.

Maybe an option would be to add a module parameter that would allow to
use the vfio platform/amba driver without reset module (with explicit
opt-in from the user). Maybe this can be done later on.

FYI I will not have access to my mailbox until the end of the week.

Best Regards

Eric

[toc] | [prev] | [next] | [standalone]

#1359043

Hi Baptiste,

>> I couldn't see a reset function for AMBA devices.
>>
>> Is there anyway to write reset function for it? I have no experience with 
>> AMBA devices.
> To my knowledge only the PL330 DMA controller (drivers/dma/pl330.c) was
> passthrough'ed at some point, rather for development purpose than
> production purpose. This was done by Virtual Open Systems (ask Baptiste
> for more details). But I don't think this is really used.

Are you OK with requiring reset driver for AMBA devices too? The PL330 
driver won't work until a reset driver is submitted for it.

>>
>> Would you include a reset needed flag and just not require it for AMBA? 
>> (I honestly don't like this idea)
>>
> I think the requirement also makes sense for AMBA.
> 
> Maybe an option would be to add a module parameter that would allow to
> use the vfio platform/amba driver without reset module (with explicit
> opt-in from the user). Maybe this can be done later on.

I can see this flag useful for testing purposes but it should not be used in 
production.

How about I add a module parameter which is not set by default? When set,
I don't perform the reset requirement check.

> 
> FYI I will not have access to my mailbox until the end of the week.

OK, no rush.

> 
> Best Regards
> 
> Eric
> 


-- 
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