Message-ID: <eb964959-4425-4412-beae-265fa02fd800@redhat.com>
Date: Mon, 25 Aug 2025 18:48:30 +0200
From: Eric Auger <eric.auger@...hat.com>
To: Alex Williamson <alex.williamson@...hat.com>,
 Pranjal Shrivastava <praan@...gle.com>
Cc: Mostafa Saleh <smostafa@...gle.com>, kvm@...r.kernel.org,
 linux-kernel@...r.kernel.org, clg@...hat.com
Subject: Re: [PATCH 2/2] vfio/platform: Mark for removal

Hi Alex,

On 8/25/25 6:15 PM, Alex Williamson wrote:
> On Mon, 25 Aug 2025 13:48:59 +0000
> Pranjal Shrivastava <praan@...gle.com> wrote:
>
>> On Wed, Aug 20, 2025 at 08:25:47PM +0000, Mostafa Saleh wrote:
>>> Hi Eric,
>>>
>>> On Wed, Aug 20, 2025 at 06:29:27PM +0200, Eric Auger wrote:  
>>>> Hi Mostafa,
>>>>
>>>> On 8/20/25 5:20 PM, Mostafa Saleh wrote:  
>>>>> Hi Eric,
>>>>>
>>>>> On Tue, Aug 19, 2025 at 11:58:32AM +0200, Eric Auger wrote:  
>>>>>> Hi Mostafa,
>>>>>>
>>>>>> On 8/18/25 7:33 PM, Mostafa Saleh wrote:  
>>>>>>> On Mon, Aug 18, 2025 at 10:52:42AM -0600, Alex Williamson wrote:  
>>>>>>>> On Fri, 15 Aug 2025 16:59:37 +0000
>>>>>>>> Mostafa Saleh <smostafa@...gle.com> wrote:
>>>>>>>>  
>>>>>>>>> Hi Alex,
>>>>>>>>>
>>>>>>>>> On Wed, Aug 06, 2025 at 11:03:12AM -0600, Alex Williamson wrote:  
>>>>>>>>>> vfio-platform hasn't had a meaningful contribution in years.  In-tree
>>>>>>>>>> hardware support is predominantly only for devices which are long since
>>>>>>>>>> e-waste.  QEMU support for platform devices is slated for removal in
>>>>>>>>>> QEMU-10.2.  Eric Auger presented on the future of the vfio-platform
>>>>>>>>>> driver and difficulties supporting new devices at KVM Forum 2024,
>>>>>>>>>> gaining some support for removal, some disagreement, but garnering no
>>>>>>>>>> new hardware support, leaving the driver in a state where it cannot
>>>>>>>>>> be tested.
>>>>>>>>>>
>>>>>>>>>> Mark as obsolete and subject to removal.    
>>>>>>>>> Recently(this year) in Android, we enabled VFIO-platform for protected KVM,
>>>>>>>>> and it’s supported in our VMM (CrosVM) [1].
>>>>>>>>> CrosVM support is different from Qemu, as it doesn't require any device
>>>>>>>>> specific logic in the VMM, however, it relies on loading a device tree
>>>>>>>>> template in runtime (with “compatiable” string...) and it will just
>>>>>>>>> override regs, irqs.. So it doesn’t need device knowledge (at least for now)
>>>>>>>>> Similarly, the kernel doesn’t need reset drivers as the hypervisor handles that.  
>>>>>>>> I think what we attempt to achieve in vfio is repeatability and data
>>>>>>>> integrity independent of the hypervisor.  IOW, if we 'kill -9' the
>>>>>>>> hypervisor process, the kernel can bring the device back to a default
>>>>>>>> state where the device isn't wedged or leaking information through the
>>>>>>>> device to the next use case.  If the hypervisor wants to support
>>>>>>>> enhanced resets on top of that, that's great, but I think it becomes
>>>>>>>> difficult to argue that vfio-platform itself holds up its end of the
>>>>>>>> bargain if we're really trusting the hypervisor to handle these aspects.  
>>>>>>> Sorry I was not clear, we only use that in Android for ARM64 and pKVM,
>>>>>>> where the hypervisor in this context means the code running in EL2 which
>>>>>>> is more privileged than the kernel, so it should be trusted.
>>>>>>> However, as I mentioned that code is not upstream yet, so it's a valid
>>>>>>> concern that the kernel still needs a reset driver.
>>>>>>>  
>>>>>>>>> Unfortunately, there is no upstream support at the moment, we are making
>>>>>>>>> some -slow- progress on that [2][3]
>>>>>>>>>
>>>>>>>>> If it helps, I have access to HW that can run that and I can review/test
>>>>>>>>> changes, until upstream support lands; if you are open to keeping VFIO-platform.
>>>>>>>>> Or I can look into adding support for existing upstream HW(with platforms I am
>>>>>>>>> familiar with as Pixel-6)  
>>>>>>>> Ultimately I'll lean on Eric to make the call.  I know he's concerned
>>>>>>>> about testing, but he raised that and various other concerns whether
>>>>>>>> platform device really have a future with vfio nearly a year ago and
>>>>>>>> nothing has changed.  Currently it requires a module option opt-in to
>>>>>>>> enable devices that the kernel doesn't know how to reset.  Is that
>>>>>>>> sufficient or should use of such a device taint the kernel?  If any
>>>>>>>> device beyond the few e-waste devices that we know how to reset taint
>>>>>>>> the kernel, should this support really even be in the kernel?  Thanks,  
>>>>>>> I think with the way it’s supported at the moment we need the kernel
>>>>>>> to ensure that reset happens.  
>>>>>> Effectively my main concern is I cannot test vfio-platform anymore. We
>>>>>> had some CVEs also impacting the vfio platform code base and it is a
>>>>>> major issue not being able to test. That's why I was obliged, last year,
>>>>>> to resume the integration of a new device (the tegra234 mgbe), nobody
>>>>>> seemed to be really interested in and this work could not be upstreamed
>>>>>> due to lack of traction and its hacky nature.
>>>>>>
>>>>>> You did not really comment on which kind of devices were currently
>>>>>> integrated. Are they within the original scope of vfio (with DMA
>>>>>> capabilities and protected by an IOMMU)? Last discussion we had in
>>>>>> https://lore.kernel.org/all/ZvvLpLUZnj-Z_tEs@google.com/ led to the
>>>>>> conclusion that maybe VFIO was not the best suited framework.  
>>>>> At the moment, Android device assignement only supports DMA capable
>>>>> devices which are behind an IOMMU, and we use VFIO-platform for that,
>>>>> most of our use cases are accelerators.
>>>>>
>>>>> In that thread, I was looking into adding support for simpler devices
>>>>> (such as sensors) but as discussed that won’t be done through
>>>>> VFIO-platform.
>>>>>
>>>>> Ignoring Android, as I mentioned, I can work on adding support for
>>>>> existing upstream platforms (preferably ARM64, that I can get access to)
>>>>> such as Pixel-6, which should make it easier to test.
>>>>>
>>>>> Also, we have some interest on adding new features such as run-time
>>>>> power management.  
>>>> OK fair enough. If Alex agrees then we can wait for those efforts. Also
>>>> I think it would make sense to formalize the way you reset the devices
>>>> (I understand the hyp does that under the hood).  
>>> I think currently - with some help from the platform bus- we can rely on
>>> the existing shutdown method, instead of specific hooks.
>>> As the hypervisor logic will only be for ARM64 (at least for now), I can
>>> look more into this.
>>>
>>> But I think the top priority would be to establish a decent platform to
>>> test with, I will start looking into Pixel-6 (although that would need
>>> to land IOMMU support for it upstream first). I also have a morello
>>> board with SMMUv3, but I think it's all PCI.
>>>   
>>>>>  
>>>>>> In case we keep the driver in, I think we need to get a garantee that
>>>>>> you or someone else at Google commits to review and test potential
>>>>>> changes with a perspective to take over its maintenance.  
>>>>> I can’t make guarantees on behalf of Google, but I can contribute in
>>>>> reviewing/testing/maintenance of the driver as far as I am able to.
>>>>> If you want, you can add me as reviewer to the driver.  
>>>> I understand. I think the usual way then is for you to send a patch to
>>>> update the Maintainers file.  
>>> I see, I will send one shortly.
>>>   
>> I could contribute time to help with the maintenance effort here, if
>> needed. Please let me know if you'd like that.
> You can join Mostafa and post a patch to be added as a designated
> reviewer.
>
> If we're not going to deprecate vfio-platform overall for now, what
> about vfio-amba and all the reset drivers?  It seems that even if
> Google cares about vfio-platform, these still fall outside of what's
> being used or tested.  Should we drive something like below to see what
> comes out of the woodwork?:

As far as I know vfio-amba has never been used. With regards to the
reset driver I think this is reasonable to deprecate them as the HW has
ewasted

Thanks

Eric
>
> diff --git a/drivers/vfio/platform/Kconfig b/drivers/vfio/platform/Kconfig
> index 88fcde51f024..c6be29b2c24b 100644
> --- a/drivers/vfio/platform/Kconfig
> +++ b/drivers/vfio/platform/Kconfig
> @@ -17,10 +17,13 @@ config VFIO_PLATFORM
>  	  If you don't know what to do here, say N.
>  
>  config VFIO_AMBA
> -	tristate "VFIO support for AMBA devices"
> +	tristate "VFIO support for AMBA devices (DEPRECATED)"
>  	depends on ARM_AMBA || COMPILE_TEST
>  	select VFIO_PLATFORM_BASE
>  	help
> +	  The vfio-amba driver is deprecated and will be removed in a
> +	  future kernel release.
> +
>  	  Support for ARM AMBA devices with VFIO. This is required to make
>  	  use of ARM AMBA devices present on the system using the VFIO
>  	  framework.
> diff --git a/drivers/vfio/platform/reset/Kconfig b/drivers/vfio/platform/reset/Kconfig
> index dcc08dc145a5..70af0dbe293b 100644
> --- a/drivers/vfio/platform/reset/Kconfig
> +++ b/drivers/vfio/platform/reset/Kconfig
> @@ -1,21 +1,21 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  if VFIO_PLATFORM
>  config VFIO_PLATFORM_CALXEDAXGMAC_RESET
> -	tristate "VFIO support for calxeda xgmac reset"
> +	tristate "VFIO support for calxeda xgmac reset (DEPRECATED)"
>  	help
>  	  Enables the VFIO platform driver to handle reset for Calxeda xgmac
>  
>  	  If you don't know what to do here, say N.
>  
>  config VFIO_PLATFORM_AMDXGBE_RESET
> -	tristate "VFIO support for AMD XGBE reset"
> +	tristate "VFIO support for AMD XGBE reset (DEPRECATED)"
>  	help
>  	  Enables the VFIO platform driver to handle reset for AMD XGBE
>  
>  	  If you don't know what to do here, say N.
>  
>  config VFIO_PLATFORM_BCMFLEXRM_RESET
> -	tristate "VFIO support for Broadcom FlexRM reset"
> +	tristate "VFIO support for Broadcom FlexRM reset (DEPRECATED)"
>  	depends on ARCH_BCM_IPROC || COMPILE_TEST
>  	default ARCH_BCM_IPROC
>  	help
> diff --git a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c
> index abdca900802d..45f386a042a9 100644
> --- a/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c
> +++ b/drivers/vfio/platform/reset/vfio_platform_amdxgbe.c
> @@ -52,6 +52,8 @@ static int vfio_platform_amdxgbe_reset(struct vfio_platform_device *vdev)
>  	u32 dma_mr_value, pcs_value, value;
>  	unsigned int count;
>  
> +	dev_err_once(vdev->device, "DEPRECATION: VFIO AMD XGBE platform reset is deprecated and will be removed in a future kernel release\n");
> +
>  	if (!xgmac_regs->ioaddr) {
>  		xgmac_regs->ioaddr =
>  			ioremap(xgmac_regs->addr, xgmac_regs->size);
> diff --git a/drivers/vfio/platform/reset/vfio_platform_bcmflexrm.c b/drivers/vfio/platform/reset/vfio_platform_bcmflexrm.c
> index 1131ebe4837d..51c9d156f307 100644
> --- a/drivers/vfio/platform/reset/vfio_platform_bcmflexrm.c
> +++ b/drivers/vfio/platform/reset/vfio_platform_bcmflexrm.c
> @@ -72,6 +72,8 @@ static int vfio_platform_bcmflexrm_reset(struct vfio_platform_device *vdev)
>  	int rc = 0, ret = 0, ring_num = 0;
>  	struct vfio_platform_region *reg = &vdev->regions[0];
>  
> +	dev_err_once(vdev->device, "DEPRECATION: VFIO Broadcom FlexRM platform reset is deprecated and will be removed in a future kernel release\n");
> +
>  	/* Map FlexRM ring registers if not mapped */
>  	if (!reg->ioaddr) {
>  		reg->ioaddr = ioremap(reg->addr, reg->size);
> diff --git a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
> index 63cc7f0b2e4a..a298045a8e19 100644
> --- a/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
> +++ b/drivers/vfio/platform/reset/vfio_platform_calxedaxgmac.c
> @@ -50,6 +50,8 @@ static int vfio_platform_calxedaxgmac_reset(struct vfio_platform_device *vdev)
>  {
>  	struct vfio_platform_region *reg = &vdev->regions[0];
>  
> +	dev_err_once(vdev->device, "DEPRECATION: VFIO Calxeda xgmac platform reset is deprecated and will be removed in a future kernel release\n");
> +
>  	if (!reg->ioaddr) {
>  		reg->ioaddr =
>  			ioremap(reg->addr, reg->size);
> diff --git a/drivers/vfio/platform/vfio_amba.c b/drivers/vfio/platform/vfio_amba.c
> index ff8ff8480968..9f5c527baa8a 100644
> --- a/drivers/vfio/platform/vfio_amba.c
> +++ b/drivers/vfio/platform/vfio_amba.c
> @@ -70,6 +70,8 @@ static int vfio_amba_probe(struct amba_device *adev, const struct amba_id *id)
>  	struct vfio_platform_device *vdev;
>  	int ret;
>  
> +	dev_err_once(&adev->dev, "DEPRECATION: vfio-amba is deprecated and will be removed in a future kernel release\n");
> +
>  	vdev = vfio_alloc_device(vfio_platform_device, vdev, &adev->dev,
>  				 &vfio_amba_ops);
>  	if (IS_ERR(vdev))
>

Powered by blists - more mailing lists