On Mon, 25 Jan 2021 20:45:22 -0400
Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
On Mon, Jan 25, 2021 at 04:31:51PM -0700, Alex Williamson wrote:I reject this assertion, there are plenty of structs that clearly
We're supposed to be enlightened by a vendor driver that does nothingThe end driver already havs the pdev, the RFC doesn't go enough into
more than pass the opaque device_data through to the core functions,
but in reality this is exactly the point of concern above. At a
minimum that vendor driver needs to look at the vdev to get the
pdev,
those bits, it is a good comment.
The dd_data pased to the vfio_create_pci_device() will be retrieved
from the ops to get back to the end drivers data. This can cleanly
include everything: the VF pci_device, PF pci_device, mlx5_core
pointer, vfio_device and vfio_pci_device.
This is why the example passes in the mvadev:
+ vdev = vfio_create_pci_device(pdev, &mlx5_vfio_pci_ops, mvadev);
The mvadev has the PF, VF, and mlx5 core driver pointer.
Getting that back out during the ops is enough to do what the mlx5
driver needs to do, which is relay migration related IOCTLs to the PF
function via the mlx5_core driver so the device can execute them on
behalf of the VF.
but then what else does it look at, consume, or modify. Now we haveThe kernel has never followed rigid rules for data isolation, it is
vendor drivers misusing the core because it's not clear which fields
are private and how public fields can be used safely,
normal to have whole private structs exposed in headers so that
container_of can be used to properly compose data structures.
indicate private vs public data or, as we've done in mdev, clearly
marking the private data in a "private" header and provide access
functions for public fields. Including a "private" header to make use
of a "library" is just wrong. In the example above, there's no way for
the mlx vendor driver to get back dd_data without extracting it from
struct vfio_pci_device itself.
Look at struct device, for instance. Most of that is private to theThat's not really what I'm getting from your feedback, indicating
driver core.
A few 'private to vfio-pci-core' comments would help, it is good
feedback to make that more clear.
extensions potentially break vendor drivers, etc. We're only even handThis is a RFC, not a complete patch series. The RFC is to get feedback
waving that existing device specific support could be farmed out to new
device specific drivers without even going to the effort to prove that.
on the general design before everyone comits alot of resources and
positions get dug in.
Do you really think the existing device specific support would be a
problem to lift? It already looks pretty clean with the
vfio_pci_regops, looks easy enough to lift to the parent.
So far the TODOs rather mask the dirty little secrets of theIt would be helpful to get actual feedback on the high level design -
extension rather than showing how a vendor derived driver needs to
root around in struct vfio_pci_device to do something useful, so
probably porting actual device specific support rather than further
hand waving would be more helpful.
someting like this was already tried in May and didn't go anywhere -
are you surprised that we are reluctant to commit alot of resources
doing a complete job just to have it go nowhere again?
vfio-pci is essentially done, the mlx stub driver should be enough to
see the direction, and additional concerns can be handled with TODO
comments. Sorry if this is not construed as actual feedback, I think
both Connie and I are making an effort to understand this and being
hampered by lack of a clear api or a vendor driver that's anything more
than vfio-pci plus an aux bus interface. Thanks,
Alex