On 10/5/20 12:01 PM, Cornelia Huck wrote:
On Mon, 5 Oct 2020 09:52:25 -0400
Matthew Rosato <mjrosato@xxxxxxxxxxxxx> wrote:
On 10/2/20 5:44 PM, Alex Williamson wrote:
Can you discuss why a region with embedded capability chain is a better
solution than extending the VFIO_DEVICE_GET_INFO ioctl to support a
capability chain and providing this info there? This all appears to be
read-only info, so what's the benefit of duplicating yet another
It is indeed read-only info, and the device region was defined as such.
I would not necessarily be opposed to extending VFIO_DEVICE_GET_INFO
with these defined as capabilities; I'd say a primary motivating factor
to putting these in their own region was to avoid stuffing a bunch of
s390-specific capabilities into a general-purpose ioctl response.
Can't you make the zdev code register the capabilities? That would put
them nicely into their own configurable part.
I can still keep the code that adds these capabilities in the zdev .c
file, thus meaning they will only be added for s390 zpci devices -- but
the actual definition of them should probably instead be in vfio.h, no?
(maybe that's what you mean, but let's lay it out just in case)
The capability IDs would be shared with any other potential user of
VFIO_DEVICE_GET_INFO (I guess there is precedent for this already,
nvlink2 does this for vfio_region_info, see
VFIO_REGION_INFO_CAP_NVLINK2_SSATGT as an example).
Today, ZPCI would be the only users of VFIO_DEVICE_GET_INFO capability
chains. Tomorrow, some other type might use them too. Unless we want
to put a stake in the ground that says there will never be a case for a
capability that all devices share on VFIO_DEVICE_GET_INFO, I think we
should keep the IDs unique and define the capabilities in vfio.h but do
the corresponding add_capability() calls from a zdev-specific file.
But if you're OK with that notion, I can give that a crack in v3.
capability chain in a region? It would also be possible to define four
separate device specific regions, one for each of these capabilities
rather than creating this chain. It just seems like a strange approach
I'm not sure if creating separate regions would be the right approach
though; these are just the first 4. There will definitely be additional
capabilities in support of new zPCI features moving forward, I'm not
sure how many regions we really want to end up with. Some might be as
small as a single field, which seems more in-line with capabilities vs
an entire region.
If we are expecting more of these in the future, going with GET_INFO
capabilities when adding new ones seems like the best approach.
