On Mon, 20 May 2019 13:19:23 +0200
Pierre Morel <pmorel@xxxxxxxxxxxxx> wrote:
> On 17/05/2019 20:04, Pierre Morel wrote:
> > On 17/05/2019 18:41, Alex Williamson wrote:
> >> On Fri, 17 May 2019 18:16:50 +0200
> >> Pierre Morel <pmorel@xxxxxxxxxxxxx> wrote:
> >>
> >>> We implement the capability interface for VFIO_IOMMU_GET_INFO.
> >>>
> >>> When calling the ioctl, the user must specify
> >>> VFIO_IOMMU_INFO_CAPABILITIES to retrieve the capabilities and
> >>> must check in the answer if capabilities are supported.
> >>>
> >>> The iommu get_attr callback will be used to retrieve the specific
> >>> attributes and fill the capabilities.
> >>>
> >>> Currently two Z-PCI specific capabilities will be queried and
> >>> filled by the underlying Z specific s390_iommu:
> >>> VFIO_IOMMU_INFO_CAP_QFN for the PCI query function attributes
> >>> and
> >>> VFIO_IOMMU_INFO_CAP_QGRP for the PCI query function group.
> >>>
> >>> Other architectures may add new capabilities in the same way
> >>> after enhancing the architecture specific IOMMU driver.
> >>>
> >>> Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
> >>> ---
> >>> drivers/vfio/vfio_iommu_type1.c | 122
> >>> +++++++++++++++++++++++++++++++++++++++-
> >>> 1 file changed, 121 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/vfio/vfio_iommu_type1.c
> >>> b/drivers/vfio/vfio_iommu_type1.c
> >>> index d0f731c..9435647 100644
> >>> --- a/drivers/vfio/vfio_iommu_type1.c
> >>> +++ b/drivers/vfio/vfio_iommu_type1.c
> >>> @@ -1658,6 +1658,97 @@ static int
> >>> vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
> >>> return ret;
> >>> }
> >>> +static int vfio_iommu_type1_zpci_fn(struct iommu_domain *domain,
> >>> + struct vfio_info_cap *caps, size_t size)
> >>> +{
> >>> + struct vfio_iommu_type1_info_pcifn *info_fn;
> >>> + int ret;
> >>> +
> >>> + info_fn = kzalloc(size, GFP_KERNEL);
> >>> + if (!info_fn)
> >>> + return -ENOMEM;
> >>> +
> >>> + ret = iommu_domain_get_attr(domain, DOMAIN_ATTR_ZPCI_FN,
> >>> + &info_fn->response);
> >>
> >> What ensures that the 'struct clp_rsp_query_pci' returned from this
> >> get_attr remains consistent with a 'struct vfio_iommu_pci_function'?
> >> Why does the latter contains so many reserved fields (beyond simply
> >> alignment) for a user API? What fields of these structures are
> >> actually useful to userspace? Should any fields not be exposed to the
> >> user? Aren't BAR sizes redundant to what's available through the vfio
> >> PCI API? I'm afraid that simply redefining an internal structure as
> >> the API leaves a lot to be desired too. Thanks,
> >>
> >> Alex
> >>
> > Hi Alex,
> >
> > I simply used the structure returned by the firmware to be sure to be
> > consistent with future evolutions and facilitate the copy from CLP and
> > to userland.
> >
> > If you prefer, and I understand that this is the case, I can define a
> > specific VFIO_IOMMU structure with only the fields relevant to the user,
> > leaving future enhancement of the user's interface being implemented in
> > another kernel patch when the time has come.
> >
> > In fact, the struct will have all defined fields I used but not the BAR
> > size and address (at least for now because there are special cases we do
> > not support yet with bars).
> > All the reserved fields can go away.
> >
> > Is it more conform to your idea?
> >
> > Also I have 2 interfaces:
> >
> > s390_iommu.get_attr <-I1-> VFIO_IOMMU <-I2-> userland
> >
> > Do you prefer:
> > - 2 different structures, no CLP raw structure
> > - the CLP raw structure for I1 and a VFIO specific structure for I2
<entering from the sideline>
IIUC, get_attr extracts various data points via clp, and we then make
it available to userspace. The clp interface needs to be abstracted
away at some point... one question from me: Is there a chance that
someone else may want to make use of the userspace interface (extra
information about a function)? If yes, I'd expect the get_attr to
obtain some kind of portable information already (basically your third
option, below).
>
> Hi Alex,
>
> I am back again on this.
> This solution here above seems to me the best one but in this way I must
> include S390 specific include inside the iommu_type1, which is AFAIU not
> a good thing.
> It seems that the powerpc architecture use a solution with a dedicated
> VFIO_IOMMU, the vfio_iommu_spar_tce.
>
> Wouldn't it be a solution for s390 too, to use the vfio_iommu_type1 as a
> basis to have a s390 dedicated solution.
> Then it becomes easier to have on one side the s390_iommu interface,
> S390 specific, and on the other side a VFIO interface without a blind
> copy of the firmware values.
If nobody else would want this exact interface, it might be a solution.
It would still be better not to encode clp data explicitly in the
userspace interface.
>
> Do you think it is a viable solution?
>
> Thanks,
> Pierre
>
>
>
> > - the same VFIO structure for both I1 and I2
