Re: [PATCH 1/2] vfio, platform: add support for ACPI while detecting the reset driver

[Marc Zyngier]

On Thu, 10 Mar 2016 12:13:28 -0500
Sinan Kaya <okaya@xxxxxxxxxxxxxx> wrote:

> On 3/10/2016 5:11 AM, Eric Auger wrote:
> > Hi Sinan,
> > On 03/08/2016 04:33 PM, Sinan Kaya wrote:
> >> The  code is using the compatible DT string to associate a reset driver with
> >> the actual device itself. The compatible string does not exist on ACPI
> >> based systems. HID is the unique identifier for a device driver instead.
> >> The change allows a driver to register with DT compatible string or ACPI
> >> HID and then match the object with one of these conditions.
> >>
> >> Rules for loading the reset driver are as follow:
> >> - ACPI HID needs match for ACPI systems
> >> - DT compat needs to match for OF systems
> >>
> >> Tested-by: Eric Auger <eric.auger@xxxxxxxxxx> (device tree only)
> >> Tested-by: Shanker Donthineni <shankerd@xxxxxxxxxxxxxx> (ACPI only)
> >> Signed-off-by: Sinan Kaya <okaya@xxxxxxxxxxxxxx>
> >> ---
> >>  .../vfio/platform/reset/vfio_platform_amdxgbe.c    |   4 +-
> >>  .../platform/reset/vfio_platform_calxedaxgmac.c    |   4 +-
> >>  drivers/vfio/platform/vfio_platform_common.c       | 112 +++++++++++++++++----
> >>  drivers/vfio/platform/vfio_platform_private.h      |  43 ++++----
> >>  4 files changed, 125 insertions(+), 38 deletions(-)

[...]

> >> -	vfio_platform_get_reset(vdev);
> >> +	ret = vfio_platform_get_reset(vdev);
> >> +	if (ret) {
> >> +		iommu_group_put(group);
> >> +		return ret;
> >> +	}
> > This change is not related to your commit message. Also here you change
> > the use model of VFIO platform and forbid any usage if no reset module
> > is available, right? I don't think this is something we discussed and I
> > think it removes some flexibility. Currently a warning is emitted in
> > case we don't have a reset function.
> 
> Well, I haven't seen that warning during testing. I was trying to be more 
> proactive.
> 
> I'm fine removing these checks but not having a reset driver needs a really
> big warning here.

This code really needs to be moved to a separate patch, but I agree
overall: a reset function should be made mandatory - I cannot imagine
trusting the HW to be left in a sane state after being used by a guest.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.