linux-kernel - Re: [PATCH V1 1/3] drivers/fpga/amd: Add new driver for AMD Versal PCIe card

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zzw5MOb6dUD6BLYh@yilunxu-OptiPlex-7050>
Date: Tue, 19 Nov 2024 15:07:28 +0800
From: Xu Yilun <yilun.xu@...ux.intel.com>
To: Yidong Zhang <yidong.zhang@....com>
Cc: linux-kernel@...r.kernel.org, linux-fpga@...r.kernel.org,
	mdf@...nel.org, hao.wu@...el.com, yilun.xu@...el.com,
	lizhi.hou@....com, DMG Karthik <Karthik.DMG@....com>,
	Nishad Saraf <nishads@....com>,
	Prapul Krishnamurthy <prapulk@....com>
Subject: Re: [PATCH V1 1/3] drivers/fpga/amd: Add new driver for AMD Versal
 PCIe card

> > > +obj-$(CONFIG_AMD_VERSAL_MGMT)                        += amd-vmgmt.o
> > 
> > IMHO the naming vmgmt is hard to understand, any better idea?
> 
> The "v" stand for Versal. We would change to amd-vpci for Versal based pcie

"v" + "pci" is quite a misleading term, maybe just versal-pci?

> devices.
> 
>

[...]
 
> > 
> > > +{
> > > +     struct comms_device *ccdev;
> > > +
> > > +     ccdev = devm_kzalloc(&vdev->pdev->dev, sizeof(*ccdev), GFP_KERNEL);
> > > +     if (!ccdev)
> > > +             return ERR_PTR(-ENOMEM);
> > > +
> > > +     ccdev->vdev = vdev;
> > > +
> > > +     ccdev->regmap = devm_regmap_init_mmio(&vdev->pdev->dev,
> > > +                                           vdev->tbl + COMMS_PCI_BAR_OFF,
> > > +                                           &comms_regmap_config);
> > 
> > I'm not sure why a regmap is needed. All register accessing is within
> > the same module/file, and I assume a base+offset is enough to position
> > the register addr.
> 
> I thought the regmap is preferred. We can use some common APIs like
> regmap_bulk_*. The base+offset works too, then we will implement our own
> bulk_* functions. Please let me know.

I didn't see any regmap_bulk_*. AFAICS regmap is not needed here.

[...]

> > > +     /* create fgpa bridge, region for the base shell */
> > > +     fdev->bridge = fpga_bridge_register(dev, "AMD Versal FPGA Bridge",
> > > +                                         &vmgmt_br_ops, fdev);
> > 
> > I didn't find the br_ops anywhere in this patchset. So how to gate the
> > FPGA region when it is being reprogrammed? What is the physical link
> > between the FPGA region and outside visitors?
> 
> The FPGA region gate operation is done in the FW running in this PCIe card.
> The FW will "freeze" the gate before programing the PL. After downloading
> the new hardware. The FW will then "free" the gate.

So no OS operation is needed, then seems no need the fpga_bridge object.

> 
> No physical link between FPGA region and outside visitors, the FW handles
> all requests.
> 
> > 
> > > +     if (IS_ERR(fdev->bridge)) {
> > > +             vmgmt_err(vdev, "Failed to register FPGA bridge, err %ld",
> > > +                       PTR_ERR(fdev->bridge));
> > > +             ret = PTR_ERR(fdev->bridge);
> > > +             goto unregister_fpga_mgr;
> > > +     }
> > > +
> > > +     region = (struct fpga_region_info) {
> > > +             .compat_id = (struct fpga_compat_id *)&vdev->intf_uuid,
> > > +             .get_bridges = vmgmt_get_bridges,
> > > +             .mgr = fdev->mgr,
> > > +             .priv = fdev,
> > > +     };
> > > +
> > > +     fdev->region = fpga_region_register_full(dev, &region);
> > 
> > I assume the fpga region represents the user PF, is it? If you
> > reprogram the FPGA region, how does the user PF driver aware the HW is
> > changing?
> 
> The HW changing request is always requested from the user PF driver. The

I don't understand. In your patch the FPGA reprograming is triggered by
an IOCTL, usually a userspace application calls it. But here says it is 
triggered by the user PF *driver*, which IIUC is a kernel driver.
Anything I missed?

> user PF driver will make sure it is safe to change hardware. Then, the user
> PF driver notify the mgmt PF driver by a unique identify of the HW bitstream
> (PL Data).
> 
> The mgmt PF driver, the amd-vpci driver, will check the unique identify and
> then find the same PL Data from its local storage which is previously
> installed, and start downloading it.

Is the flow included in this patchset? Please elaborate more.

[...]

> > > +/**
> > > + * VERSAL_MGMT_LOAD_XCLBIN_IOCTL - Download XCLBIN to the device
> > > + *
> > > + * This IOCTL is used to download XCLBIN down to the device.
> > > + * Return: 0 on success, -errno on failure.
> > > + */
> > > +#define VERSAL_MGMT_LOAD_XCLBIN_IOCTL        _IOW(VERSAL_MGMT_MAGIC,         \
> > > +                                          VERSAL_MGMT_BASE + 0, void *)
> > 
> > Many definitions are added in a batch but some are not used in this
> > patch. Please reorganize the patches for easer review, even for first
> > version.
> > 
> > Thanks,
> > Yilun
> 
> Hi Yilun,
> 
> Thanks for taking your time, and yes for sure I will make each patch more
> self-contained.
> 
> Here is my thoughts on upcoming patches structure:
> 1st patch, adding driver probe and FPGA framework; the actual ops

Just adding driver probe for 1st patch please.

Thanks,
Yilun

> of handling communication channel message and remote queue message
> will present as no-op with comments.
> 
> 2nd patch, adding the communication channel services
> 3rd patch, adding the remote queue services
> 4th patch, adding the callers of using the remote queue services
> 
> Thanks,
> David
> 
> > 
> > > +
> > > +#endif /* _UAPI_LINUX_VMGMT_H */
> > > --
> > > 2.34.1
> > > 
> > >