Hi Alex: Thanks for your quick response and is helpful to me. I did some more investigation and some comments inline. On Tue, Sep 08, 2020 at 03:10:02PM -0600, Alex Williamson wrote: > On Tue, 8 Sep 2020 15:13:32 +0800 > Xu Yilun <yilun.xu@xxxxxxxxx> wrote: > > > This patch adds description for VFIO Mdev support for dfl devices on > > dfl bus. > > > > Signed-off-by: Xu Yilun <yilun.xu@xxxxxxxxx> > > Signed-off-by: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx> > > --- > > Documentation/fpga/dfl.rst | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst > > index 0404fe6..f077754 100644 > > --- a/Documentation/fpga/dfl.rst > > +++ b/Documentation/fpga/dfl.rst > > @@ -502,6 +502,26 @@ FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-pr.c) > > could be a reference. > > > > > > +VFIO Mdev support for DFL devices > > +================================= > > +As we introduced a dfl bus for private features, they could be added to dfl bus > > +as independent dfl devices. There is a requirement to handle these devices > > +either by kernel drivers or by direct access from userspace. Usually we bind > > +the kernel drivers to devices which provide board management functions, and > > +gives user direct access to devices which cooperate closely with user > > +controlled Accelerated Function Unit (AFU). We realize this with a VFIO Mdev > > +implementation. When we bind the vfio-mdev-dfl driver to a dfl device, it > > +realizes a group of callbacks and registers to the Mdev framework as a > > +parent (physical) device. It could then create one (available_instances == 1) > > +mdev device. > > +Since dfl devices are sub devices of FPGA DFL physical devices (e.g. PCIE > > +device), which provide no DMA isolation for each sub device, this may leads to > > +DMA isolation problem if a private feature is designed to be capable of DMA. > > +The AFU user could potentially access the whole device addressing space and > > +impact the private feature. So now the general HW design rule is, no DMA > > +capability for private features. It eliminates the DMA isolation problem. > > What's the advantage of entangling mdev/vfio in this approach versus > simply exposing the MMIO region of the device via sysfs (similar to a > resource file in pci-sysfs)? This implementation doesn't support > interrupts, it doesn't support multiplexing of a device, it doesn't > perform any degree of mediation, it seems to simply say "please don't > do DMA". I don't think that's acceptable for an mdev driver. If you > want to play loose with isolation, do it somewhere else. Thanks, The intention of the patchset is to enable the userspace drivers for dfl devices. The dfl devices are actually different IP blocks integrated in FPGA to support different board functionalities. They are sub devices of the FPGA PCIe device. Their mmio blocks are in PCIE bar regions. And we want some of the dfl devices handled by the userspace drivers. Some dfl devices are capable of interrupt. I didn't add interrupt code in this patch cause now the IRQ capable dfl devices are all handled by kernel drivers. But as a generic FPGA platform, IRQ handling for userspace drivers should be supported. And I can see there are several ways to enable the userspace driver. 1. Some specific sysfs like pci do. But seems it is not the common way for userspace driver. It does't support interrupt. And potentially users operate on the same mmio region together with kernel driver at the same time. 2. VFIO driver with NOIOMMU enabled. I think it meets our needs. Do you think it is good we implement an VFIO driver for dfl devices? 3. VFIO mdev. I implemented it because it will not block us from lacking of valid iommu group. And since the driver didn't perform any mediation, I should give up. 4. UIO driver. It should work. I'm wondering if option 2 covers the functionalities of UIO and has more enhancement. So option 2 may be better? Thanks again for your time, and I really appreciate you would give some guide on it. Yilun. > > Alex
