Received-SPF: pass (google.com: domain of linux-kernel+bounces-73914-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 20 Feb 2024 17:49:04 -0800 (PST)
From: matthew.gerlach@linux.intel.com
To: Xu Yilun <yilun.xu@linux.intel.com>
cc: hao.wu@intel.com, trix@redhat.com, mdf@kernel.org, yilun.xu@intel.com, 
    linux-fpga@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] fpga: dfl: afu: support Rev 2 of DFL Port feature
In-Reply-To: <ZdF4JvYQQL8irnbW@yilunxu-OptiPlex-7050>
Message-ID: <alpine.DEB.2.22.394.2402201658400.191484@sj-4150-psse-sw-opae-dev2>
References: <20240125233715.861883-1-matthew.gerlach@linux.intel.com> <ZbjHl8ptQG5FdHvC@yilunxu-OptiPlex-7050> <alpine.DEB.2.22.394.2401300948590.112016@sj-4150-psse-sw-opae-dev2> <Zbnd8W1ciTKeoKc4@yilunxu-OptiPlex-7050> <alpine.DEB.2.22.394.2401311610020.112016@sj-4150-psse-sw-opae-dev2>
 <ZcBIjcFJjGKf0qcO@yilunxu-OptiPlex-7050> <alpine.DEB.2.22.394.2402051600190.122158@sj-4150-psse-sw-opae-dev2> <ZdF4JvYQQL8irnbW@yilunxu-OptiPlex-7050>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII; format=flowed


On Sun, 18 Feb 2024, Xu Yilun wrote:

> On Wed, Feb 07, 2024 at 08:40:55AM -0800, matthew.gerlach@linux.intel.com wrote:
>>
>>
>> On Mon, 5 Feb 2024, Xu Yilun wrote:
>>
>>> On Wed, Jan 31, 2024 at 04:26:27PM -0800, matthew.gerlach@linux.intel.com wrote:
>>>>
>>>>
>>>> On Wed, 31 Jan 2024, Xu Yilun wrote:
>>>>
>>>>> On Tue, Jan 30, 2024 at 10:00:16AM -0800, matthew.gerlach@linux.intel.com wrote:
>>>>>>
>>>>>>
>>>>>> On Tue, 30 Jan 2024, Xu Yilun wrote:
>>>>>>
>>>>>>> On Thu, Jan 25, 2024 at 03:37:15PM -0800, Matthew Gerlach wrote:
>>>>>>>> Revision 2 of the Device Feature List (DFL) Port feature
>>>>>>>> adds support for connecting the contents of the port to
>>>>>>>> multiple PCIe Physical Functions (PF).
>>>>>>>>
>>>>>>>> This new functionality requires changing the port reset
>>>>>>>> behavior during FPGA and software initialization from
>>>>>>>> revision 1 of the port feature. With revision 1, the initial
>>>>>>>> state of the logic inside the port was not guaranteed to
>>>>>>>> be valid until a port reset was performed by software during
>>>>>>>> driver initialization. With revision 2, the initial state
>>>>>>>> of the logic inside the port is guaranteed to be valid,
>>>>>>>> and a port reset is not required during driver initialization.
>>>>>>>>
>>>>>>>> This change in port reset behavior avoids a potential race
>>>>>>>> condition during PCI enumeration when a port is connected to
>>>>>>>> multiple PFs. Problems can occur if the driver attached to
>>>>>>>> the PF managing the port asserts reset in its probe function
>>>>>>>> when a driver attached to another PF accesses the port in its
>>>>>>>> own probe function. The potential problems include failed or hung
>>>>>>>
>>>>>>> Only racing during probe functions? I assume any time port_reset()
>>>>>>> would fail TLPs for the other PF. And port_reset() could be triggered
>>>>>>> at runtime by ioctl().
>>>>>>
>>>>>> Yes, a port_reset() triggered by ioctl could result in failed TLP for the
>>>>>> other PFs. The user space SW performing the ioctl needs to ensure all PFs
>>>>>> involved are properly quiesced before the port_reset is performed.
>>>>>
>>>>> How would user get an insight into other PF drivers to know everything
>>>>> is quiesced?  I mean do we need driver level management for this?
>>>>
>>>> Since this is an FPGA, the number of other PFs and the drivers bound to
>>>> those PFs depends on the FPGA image. There would also be user space software
>>>> stacks involved with the other PFs as well. The user would have to ensure
>>>> all the SW stacks and drivers are quiesced as appropriate for the FPGA
>>>
>>> User may not know everything about the device, they only get part of the
>>> controls that drivers grant. This is still true for vfio + userspace
>>> drivers.
>>
>> A user performing a port reset would have to know the impact to the specific
>> FPGA image being run in order to ensure all SW stacks are ready for the
>> reset.
>
> We are not going to change the logic of the whole driver model just
> because the device is backed up by an FPGA image.  The *driver* should be
> fully responsible for matched devices.  A HW reset unaware to the
> device driver is not wanted.  Assuming that the userspace could control
> every access to device makes no sense.
>
> For your case, there is no garantee userspace could block every access
> to "other PF" initiated by "other PF" driver.  There is also no
> notification to "other PF" driver that userspace is doing reset to
> "other PF" via "management PF" interface.  "other PF" driver just break
> on reset.

Hi Yilun,

I think this conversation has gotten a little off track. This patch 
only changes the port reset behavior at driver initialization for revision 
2 of the port IP. The behavior and the requirements of port reset during 
run time have not changed. The existing implementation requires the user 
performing the port reset to ensure appropriate SW was quiesced. This 
patch does not change this requirement.

>
>>
>>>
>>>> image. I don't think the driver performing the port_reset() can know all the
>>>
>>> Other PF drivers should know their own components. They should be aware
>>> that their devices are being reset.
>>
>> The other PF drivers depend on the actual FPGA image being run.
>>
>>>
>>>> components to be able to provide any meaningful management.
>>>
>>> If the reset provider and reset consumer are not in the same driver,
>>> they should interact with each other. IIRC, some reset controller class
>>> works for this purpose.
>>
>> The other PFs in many cases can present as standard devices with existing
>> drivers like virtio-net or virtio-blk. It does not seem desireable to have
>> to change existing drivers for a particular FPGA implementation
>
> If you have to use a specific method for reset, it is not a standard virtio
> pci device, and you have to make some change.

From the perspective of the PCI PF/VF implementing the virtio or 
other standard device, the pci endpoint is completely compliant to the 
standard, and no driver changes should be required. As mentioned above, 
this patch does nothing to change any of this behavior. Consider a port 
reset that is part of a partial configuration flow. The virtio endpoint 
can become something completely different with a completely different 
driver. This patch does not affect this flow either.

Thanks,
Matthew

>
> Thanks,
> Yilun
>
>>
>> Thanks,
>> Matthew
>>>
>>> Thanks,
>>> Yilun
>>>
>>>>
>>>> Thanks,
>>>> Matthew
>>>>
>>>>>
>>>>> Thanks,
>>>>> Yilun
>>>>>
>>>>>>
>>>>>> Do you want me to update the commit message with this information?
>>>>>>
>>>>>> Thanks,
>>>>>> Matthew
>>>>>
>>>>>
>>>
>>>
>