Re: [PATCH V2 1/4] drivers/fpga/amd: Add new driver amd versal-pci

[Yidong Zhang <yidong.zhang@amd.com>]

On 2/11/25 01:09, Xu Yilun wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Mon, Feb 10, 2025 at 03:33:07AM -0800, Yidong Zhang wrote:
>>
>>
>> On 2/6/25 20:40, Xu Yilun wrote:
>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> On Thu, Feb 06, 2025 at 07:16:25PM -0800, Yidong Zhang wrote:
>>>>
>>>>
>>>> On 2/6/25 18:19, Xu Yilun wrote:
>>>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>>>
>>>>>
>>>>>>> No need to detach a specific driver, remove all devices in the
>>>>>>> fpga-region. I imagine this could also be a generic flow for all PCI/e
>>>>>>> based FPGA cards.
>>>>>>
>>>>>> I see your point. Is there an existing example in current fpga driver for
>>>>>> PCI/e based cards?
>>>>>
>>>>> No. The fpga-region re-enumeration arch is still WIP, so no existing
>>>>> implementation.
>>>>
>>>> Got you. I can also help to test or provide feedback.
>>>> Actually, I had sent Nava my protype of using configfs for the non-OF
>>>> driver. He should have the updated patch soon.
>>>>
>>>>>
>>>>>>
>>>>>> We will need to talk to our management team and re-design our driver.
>>>>>> I think we have 2 approaches:
>>>>>> 1) Align with linux fpga, having one driver for both userPF and mgmtPF; or
>>>>>
>>>>> Don't get you. Linux FPGA doesn't require one driver for both PFs.
>>>>
>>>> I assume when you said "generic flow for removing all devices in
>>>> fpga-region" means that there is a single driver which use the fpga-region
>>>> callbacks to remove all devices and then re-progream the FPGA.
>>>>
>>>> On AMD V70 hardware, the userPF has different deviceid than the mgmtPF.
>>>> Thus, our current design is having 2 separate pcie drivers for each
>>>> different deviceid.
>>>>
>>>> Thus, in our current design, the fpga-region should be in the userPF driver
>>>> which has callbacks to remove all devices. But in mgmtPF, it is more like a
>>>
>>> A question, if the FPGA logic is cleared, does the userPF still exist on
>>> PCIe bus?
>>
>> I am not sure I fully understand your question by "FPGA logic is cleared".
>> Based on our design, the userPF exists, but all services need to be
>> re-configured after hardware is changed.
> 
> I briefly read your XRT docs. IIUC we are talking about reprogramming
> the "Dynamic Region", not the "Shell".
> 
> The userPF's PCIe basic functionality is in Shell and is out of
> our reprograming discussion. So userPF PCI device always exists even if
> the "Dynamic Region" is being reprogrammed. In this case, we should
> create fpga-region for this "Dynamic Region" in userPF driver.
> 
> userPF uses some HW communication channel to require reprograming, so
> fpga-manager could also be implemented in userPF driver.
> 

Thanks for taking your time reading our documents. You are exactly 
correct. We will use fpga-manager and fpga-region in our userPF driver 
for upstreaming. If you think this is acceptable by linux fpga 
framework. I will talk to my team for next step.

> And I agree the mgmtPF driver is a utility driver that just monitor on
> the communication channel and serve requirements, nothing to do with
> FPGA framework. I have no objection to put it in misc or firmware
> directory.
> 
> What I'm not sure is whether a standalone utility driver in kernel tree
> is preferred (at least I don't). You may talk to other maintainers.
> 

My last question for this topic:
If we decide to upstream both userPF and mgmtPF driver together, could 
them be both within the drivers/fpga/amd as in-tree driver? This will 
help user find source code easily.

Thanks,
David

>>
>>
>>>
>>>> utility which only handles request from the userPF but it has the management
>>>> privilege. Usually, cloud vendors require the mgmtPF deployed in their
>>>> secured domain (can be a separate physical machine).
>>>
>>> I though mgmtPF & userPF are just 2 functions on one PCIe card, they are not?
>>> Then how the mgmtPF writes data to another physical machine and
>>> influence the userPF?
>>>
>>
>> They are functions but they also have different device id thus we can bind
>> different driver onto each of them.
>>
>> I think I should interrupt it accurately as Host and VM. See details here
>> please:
>> https://xilinx.github.io/XRT/2019.2/html/cloud_vendor_support.html
>>
>> But, my point is that we do have 2 separate drivers.
> 
> OK, having 2 separate drivers is definitely fine.
> 
> Thanks,
> Yilun