linux-kernel - Re: [PATCH V8 7/9] vfio, platform: make reset driver a requirement by default

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4d95e48a-b5a1-c3eb-8aed-ddd307e1b028@codeaurora.org>
Date:	Wed, 13 Jul 2016 16:12:35 -0400
From:	Sinan Kaya <okaya@...eaurora.org>
To:	Alex Williamson <alex.williamson@...hat.com>
Cc:	kvm@...r.kernel.org, timur@...eaurora.org, cov@...eaurora.org,
	jcm@...hat.com, eric.auger@...hat.com, linux-acpi@...r.kernel.org,
	agross@...eaurora.org, linux-arm-msm@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	Baptiste Reynal <b.reynal@...tualopensystems.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V8 7/9] vfio, platform: make reset driver a requirement by
 default

On 6/23/2016 2:59 PM, Alex Williamson wrote:
>> >  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;
> 
> Do you see value in making the global reset_required changeable, with
> the behavior of any given device dependent on the setting of this
> variable at the time of probe?  It seems like a bit of a support issue
> to me.  Also, we're breaking existing users if there are any with this
> change.  Should we introduce a CONFIG option to set the default?  I
> think we can get away with changing the default that way, but I'm not
> so sure otherwise.
> 

We have two groups of existing users.

1. AMBA based drivers
2. DT based drivers

and now we are trying to add the ACPI based drivers in this series.

The AMBA based drivers do not have reset function implemented. Based on
previous conversation with Eric, these devices were mostly used for
bringing up the VFIO framework and were not intended for production. 
If we want to maintain existing functionality, I can change reset_required to
false by default for the AMBA based drivers.

The DT based drivers all have reset functions implemented. They shouldn't be
impacted by the reset_required flag. 

The reset_required flag is again useful for testing purposes when the reset
driver is broken or the ACPI _RST method is missing.

The previously agreed approach was to force the reset required by default
for production environment and be able to clear it for testing purposes.
When I was implementing HIDMA, I never realized that I needed a reset driver
until Arnd told me during the review. We want to avoid this for the long
term for DT and ACPI based implementations.

The reset_required command line parameter would be useful if somebody suspects
that the ACPI _RST implementation is broken or the DT based reset driver is
broken or you quickly want to test the virtualization without having a reset
driver ready yet.

Let us know which way you want to go. I can also add a Kconfig option and
set it by default. But then I have to recompile the kernel when I want to
test without the reset stuff.

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