Re: [PATCH v1 4/7] fpga: dfl: fix VF creation when ports have no local BAR space

On 2/18/22 12:14 AM, Zhang, Tianfei wrote:

quoted

-----Original Message-----
From: Tom Rix <trix@redhat.com>
Sent: Tuesday, February 15, 2022 11:51 PM
To: Zhang, Tianfei <redacted>; Wu, Hao <redacted>;
mdf@kernel.org; Xu, Yilun [off-list ref]; linux-fpga@vger.kernel.org;
linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org
Cc: corbet@lwn.net; Matthew Gerlach <redacted>
Subject: Re: [PATCH v1 4/7] fpga: dfl: fix VF creation when ports have no local
BAR space


On 2/14/22 3:26 AM, Tianfei zhang wrote:

quoted

From: Matthew Gerlach <redacted>

When a port is not connected to the same PCIe endpoint as the FME, the
port does not need to be released before being virtualized.  Fix VF
creation code to handle this new use

Similar, how does this fit in with iofs, this looks like it would not be valid for the
existing cards

IOFS introducing multiple methods for PR and AFU access.
1. Legacy Model.
2. Micro-Personas in AFU.
3. Multiple VFs per PR slot.

For 1 and 2 model, there are 1:1 mapping between Port device and PR slot (or entire AFU). In virtualization,
it should release the Port device firstly and then assign to VM. In this models, the DFL driver will track  that
the number of port devices has released (cdev->released_port_num in dfl_fpga_cdev_config_ports_vf() function)
are equal with the numbers of SRIOV or not. But in model 3, it has multiple VFs per PR slot, and assign the VF to VM
without release the port device, so the tracking mechanism of cdev->released_port_num is not workable on new

If ->release_port_num is not workable, then it needs to be generalized.

Refactor to handle all the cases.

Tom

model. This patch want to handle this new model during VF creation.

quoted

quoted

case.

Signed-off-by: Matthew Gerlach <redacted>
Signed-off-by: Tianfei Zhang <redacted>
---
   drivers/fpga/dfl.c | 11 +++++++++--
   1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c index

26f8cf890700..cfc539a656f0 100644

--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c

@@ -1705,15 +1705,22 @@

EXPORT_SYMBOL_GPL(dfl_fpga_cdev_config_ports_pf);

quoted

   int dfl_fpga_cdev_config_ports_vf(struct dfl_fpga_cdev *cdev, int num_vfs)
   {
   	struct dfl_feature_platform_data *pdata;
-	int ret = 0;
+	int ret = 0, port_count = 0;

   	mutex_lock(&cdev->lock);
+
+	list_for_each_entry(pdata, &cdev->port_dev_list, node) {
+		if (pdata->dev)

This looks wrong,

pdata->dev is dereferenced below, if there is a case were (!pdata->dev)
here there would be crash later.

quoted

+			continue;
+		port_count++;

how does this work when only some of the ports are handled in the new way ?

This code want to handle the " Multiple VFs per PR slot" model as I mentioned above.
In new model, the port_count want to count that how many port devices have released.
This code looks not good readability, I try to re-write it.

quoted

Tom

quoted

+	}
+
   	/*
   	 * can't turn multiple ports into 1 VF device, only 1 port for 1 VF
   	 * device, so if released port number doesn't match VF device number,
   	 * then reject the request with -EINVAL error code.
   	 */
-	if (cdev->released_port_num != num_vfs) {
+	if (port_count && cdev->released_port_num != num_vfs) {
   		ret = -EINVAL;
   		goto done;
   	}