Re: [PATCH 1/4] s390: pci: Exporting access to CLP PCI function and PCI group

[Pierre Morel]

On 10/05/2019 12:21, Robin Murphy wrote:
> On 10/05/2019 09:22, Pierre Morel wrote:
> > For the generic implementation of VFIO PCI we need to retrieve
> > the hardware configuration for the PCI functions and the
> > PCI function groups.
> >
> > We modify the internal function using CLP Query PCI function and
> > CLP query PCI function group so that they can be called from
> > outside the S390 architecture PCI code and prefix the two
> > functions with "zdev" to make clear that they can be called
> > knowing only the associated zdevice.
> >
> > Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
> > Reviewed-by: Sebastian Ott <sebott@xxxxxxxxxxxxx>
> > ---
> > Â arch/s390/include/asm/pci.h |Â 3 ++
> > Â arch/s390/pci/pci_clp.cÂÂÂÂ | 72 
> > ++++++++++++++++++++++++---------------------
> > Â 2 files changed, 41 insertions(+), 34 deletions(-)
> >
> > diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> > index 305befd..e66b246 100644
> > --- a/arch/s390/include/asm/pci.h
> > +++ b/arch/s390/include/asm/pci.h
> > @@ -261,4 +261,7 @@ cpumask_of_pcibus(const struct pci_bus *bus)
> > Â #endif /* CONFIG_NUMA */
> > +int zdev_query_pci_fngrp(struct zpci_dev *zdev,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂ struct clp_req_rsp_query_pci_grp *rrb);
> > +int zdev_query_pci_fn(struct zpci_dev *zdev, struct 
> > clp_req_rsp_query_pci *rrb);
> > Â #endif
> > diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c
> > index 3a36b07..4ae5d77 100644
> > --- a/arch/s390/pci/pci_clp.c
> > +++ b/arch/s390/pci/pci_clp.c
> > @@ -113,32 +113,18 @@ static void clp_store_query_pci_fngrp(struct 
> > zpci_dev *zdev,
> > ÂÂÂÂÂ }
> > Â }
> > -static int clp_query_pci_fngrp(struct zpci_dev *zdev, u8 pfgid)
> > +int zdev_query_pci_fngrp(struct zpci_dev *zdev,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂ struct clp_req_rsp_query_pci_grp *rrb)
> > Â {
> > -ÂÂÂ struct clp_req_rsp_query_pci_grp *rrb;
> > -ÂÂÂ int rc;
> > -
> > -ÂÂÂ rrb = clp_alloc_block(GFP_KERNEL);
> > -ÂÂÂ if (!rrb)
> > -ÂÂÂÂÂÂÂ return -ENOMEM;
> > -
> > ÂÂÂÂÂ memset(rrb, 0, sizeof(*rrb));
> > ÂÂÂÂÂ rrb->request.hdr.len = sizeof(rrb->request);
> > ÂÂÂÂÂ rrb->request.hdr.cmd = CLP_QUERY_PCI_FNGRP;
> > ÂÂÂÂÂ rrb->response.hdr.len = sizeof(rrb->response);
> > -ÂÂÂ rrb->request.pfgid = pfgid;
> > +ÂÂÂ rrb->request.pfgid = zdev->pfgid;
> > -ÂÂÂ rc = clp_req(rrb, CLP_LPS_PCI);
> > -ÂÂÂ if (!rc && rrb->response.hdr.rsp == CLP_RC_OK)
> > -ÂÂÂÂÂÂÂ clp_store_query_pci_fngrp(zdev, &rrb->response);
> > -ÂÂÂ else {
> > -ÂÂÂÂÂÂÂ zpci_err("Q PCI FGRP:\n");
> > -ÂÂÂÂÂÂÂ zpci_err_clp(rrb->response.hdr.rsp, rc);
> > -ÂÂÂÂÂÂÂ rc = -EIO;
> > -ÂÂÂ }
> > -ÂÂÂ clp_free_block(rrb);
> > -ÂÂÂ return rc;
> > +ÂÂÂ return clp_req(rrb, CLP_LPS_PCI);
> > Â }
> > +EXPORT_SYMBOL(zdev_query_pci_fngrp);
>
> AFAICS it's only the IOMMU driver itself which needs to call these. That 
> can't be built as a module, so you shouldn't need explicit exports - the 
> header declaration is enough.
>
> Robin.

This is right and seeing the pointer type only zPCI and s390iommu can 
make use of it.
If nobody has another point of view I will remove the export on the
next iteration.

Thanks,
Pierre

> > Â static int clp_store_query_pci_fn(struct zpci_dev *zdev,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct clp_rsp_query_pci *response)
> > @@ -174,32 +160,50 @@ static int clp_store_query_pci_fn(struct 
> > zpci_dev *zdev,
> > ÂÂÂÂÂ return 0;
> > Â }
> > -static int clp_query_pci_fn(struct zpci_dev *zdev, u32 fh)
> > +int zdev_query_pci_fn(struct zpci_dev *zdev, struct 
> > clp_req_rsp_query_pci *rrb)
> > +{
> > +
> > +ÂÂÂ memset(rrb, 0, sizeof(*rrb));
> > +ÂÂÂ rrb->request.hdr.len = sizeof(rrb->request);
> > +ÂÂÂ rrb->request.hdr.cmd = CLP_QUERY_PCI_FN;
> > +ÂÂÂ rrb->response.hdr.len = sizeof(rrb->response);
> > +ÂÂÂ rrb->request.fh = zdev->fh;
> > +
> > +ÂÂÂ return clp_req(rrb, CLP_LPS_PCI);
> > +}
> > +EXPORT_SYMBOL(zdev_query_pci_fn);
> > +
> > +static int clp_query_pci(struct zpci_dev *zdev)
> > Â {
> > ÂÂÂÂÂ struct clp_req_rsp_query_pci *rrb;
> > +ÂÂÂ struct clp_req_rsp_query_pci_grp *grrb;
> > ÂÂÂÂÂ int rc;
> > ÂÂÂÂÂ rrb = clp_alloc_block(GFP_KERNEL);
> > ÂÂÂÂÂ if (!rrb)
> > ÂÂÂÂÂÂÂÂÂ return -ENOMEM;
> > -ÂÂÂ memset(rrb, 0, sizeof(*rrb));
> > -ÂÂÂ rrb->request.hdr.len = sizeof(rrb->request);
> > -ÂÂÂ rrb->request.hdr.cmd = CLP_QUERY_PCI_FN;
> > -ÂÂÂ rrb->response.hdr.len = sizeof(rrb->response);
> > -ÂÂÂ rrb->request.fh = fh;
> > -
> > -ÂÂÂ rc = clp_req(rrb, CLP_LPS_PCI);
> > -ÂÂÂ if (!rc && rrb->response.hdr.rsp == CLP_RC_OK) {
> > -ÂÂÂÂÂÂÂ rc = clp_store_query_pci_fn(zdev, &rrb->response);
> > -ÂÂÂÂÂÂÂ if (rc)
> > -ÂÂÂÂÂÂÂÂÂÂÂ goto out;
> > -ÂÂÂÂÂÂÂ rc = clp_query_pci_fngrp(zdev, rrb->response.pfgid);
> > -ÂÂÂ } else {
> > +ÂÂÂ rc = zdev_query_pci_fn(zdev, rrb);
> > +ÂÂÂ if (rc || rrb->response.hdr.rsp != CLP_RC_OK) {
> > ÂÂÂÂÂÂÂÂÂ zpci_err("Q PCI FN:\n");
> > ÂÂÂÂÂÂÂÂÂ zpci_err_clp(rrb->response.hdr.rsp, rc);
> > ÂÂÂÂÂÂÂÂÂ rc = -EIO;
> > +ÂÂÂÂÂÂÂ goto out;
> > ÂÂÂÂÂ }
> > +ÂÂÂ rc = clp_store_query_pci_fn(zdev, &rrb->response);
> > +ÂÂÂ if (rc)
> > +ÂÂÂÂÂÂÂ goto out;
> > +
> > +ÂÂÂ grrb = (struct clp_req_rsp_query_pci_grp *)rrb;
> > +ÂÂÂ rc = zdev_query_pci_fngrp(zdev, grrb);
> > +ÂÂÂ if (rc || grrb->response.hdr.rsp != CLP_RC_OK) {
> > +ÂÂÂÂÂÂÂ zpci_err("Q PCI FGRP:\n");
> > +ÂÂÂÂÂÂÂ zpci_err_clp(grrb->response.hdr.rsp, rc);
> > +ÂÂÂÂÂÂÂ rc = -EIO;
> > +ÂÂÂÂÂÂÂ goto out;
> > +ÂÂÂ }
> > +ÂÂÂ clp_store_query_pci_fngrp(zdev, &grrb->response);
> > +
> > Â out:
> > ÂÂÂÂÂ clp_free_block(rrb);
> > ÂÂÂÂÂ return rc;
> > @@ -219,7 +223,7 @@ int clp_add_pci_device(u32 fid, u32 fh, int 
> > configured)
> > ÂÂÂÂÂ zdev->fid = fid;
> > ÂÂÂÂÂ /* Query function properties and update zdev */
> > -ÂÂÂ rc = clp_query_pci_fn(zdev, fh);
> > +ÂÂÂ rc = clp_query_pci(zdev);
> > ÂÂÂÂÂ if (rc)
> > ÂÂÂÂÂÂÂÂÂ goto error;


--
Pierre Morel
Linux/KVM/QEMU in BÃblingen - Germany