On Tue, 4 Apr 2023 12:01:37 -0700 Brett Creeley <brett.creeley@xxxxxxx> wrote: > The pds_core driver will supply adminq services, so find the PF > and register with the DSC services. > > Use the following commands to enable a VF: > echo 1 > /sys/bus/pci/drivers/pds_core/$PF_BDF/sriov_numvfs > > Signed-off-by: Brett Creeley <brett.creeley@xxxxxxx> > Signed-off-by: Shannon Nelson <shannon.nelson@xxxxxxx> > --- > drivers/vfio/pci/pds/Makefile | 1 + > drivers/vfio/pci/pds/cmds.c | 69 +++++++++++++++++++++++++++++++++ > drivers/vfio/pci/pds/cmds.h | 10 +++++ > drivers/vfio/pci/pds/pci_drv.c | 16 +++++++- > drivers/vfio/pci/pds/pci_drv.h | 9 +++++ > drivers/vfio/pci/pds/vfio_dev.c | 5 +++ > drivers/vfio/pci/pds/vfio_dev.h | 2 + > include/linux/pds/pds_lm.h | 12 ++++++ > 8 files changed, 123 insertions(+), 1 deletion(-) > create mode 100644 drivers/vfio/pci/pds/cmds.c > create mode 100644 drivers/vfio/pci/pds/cmds.h > create mode 100644 drivers/vfio/pci/pds/pci_drv.h > create mode 100644 include/linux/pds/pds_lm.h > > diff --git a/drivers/vfio/pci/pds/Makefile b/drivers/vfio/pci/pds/Makefile > index e1a55ae0f079..87581111fa17 100644 > --- a/drivers/vfio/pci/pds/Makefile > +++ b/drivers/vfio/pci/pds/Makefile > @@ -4,5 +4,6 @@ > obj-$(CONFIG_PDS_VFIO_PCI) += pds_vfio.o > > pds_vfio-y := \ > + cmds.o \ > pci_drv.o \ > vfio_dev.o > diff --git a/drivers/vfio/pci/pds/cmds.c b/drivers/vfio/pci/pds/cmds.c > new file mode 100644 > index 000000000000..7807dbb2c72c > --- /dev/null > +++ b/drivers/vfio/pci/pds/cmds.c > @@ -0,0 +1,69 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright(c) 2023 Advanced Micro Devices, Inc. */ > + > +#include <linux/io.h> > +#include <linux/types.h> > + > +#include <linux/pds/pds_common.h> > +#include <linux/pds/pds_core_if.h> > +#include <linux/pds/pds_adminq.h> > +#include <linux/pds/pds_lm.h> > + > +#include "vfio_dev.h" > +#include "cmds.h" > + > +int > +pds_vfio_register_client_cmd(struct pds_vfio_pci_device *pds_vfio) > +{ > + union pds_core_adminq_comp comp = { 0 }; > + union pds_core_adminq_cmd cmd = { 0 }; > + struct device *dev; > + int err; > + u32 id; > + u16 ci; > + > + id = PCI_DEVID(pds_vfio->pdev->bus->number, > + pci_iov_virtfn_devfn(pds_vfio->pdev, pds_vfio->vf_id)); Does this actually work? pds_vfio_pci_probe() is presumably called with a VF pdev because it calls pdsc_get_pf_struct() -> pci_iov_get_pf_drvdata(), which returns an error if !dev->is_virtfn. pds_vfio_init_device() also stores the vf_id from pci_iov_vf_id(), which requires that it's called on a VF, not PF. This same pdev gets stored at pds_vfio->pdev. OTOH, pci_iov_virtfn_devfn() used here errors if !dev->is_physfn. So I expect we're calling PCI_DEVID with the second arg as -EINVAL. The first arg would also be suspicious if pds_vfio->pdev were the PF since then we'd be making a PCI_DEVID combining the PF bus number and the VF devfn. We've also already stored the VF PCI_DEVID at pds_vfio->pci_id, so creating it again seems entirely unnecessary. Also, since we never check the return of pdsc_get_pf_struct() I'd guess we take a segfault referencing off of pds_vfio->pdsc should the driver bind to something other than expected. Validating the PF drvdata before anything else seems like a good first stop in pds_vfio_pci_probe(). It's also a little curious why we're storing the pdev at all in the pds_vfio_pci_device when it's readily available from the embedded vfio_pci_core_device. Thanks, Alex > + > + dev = &pds_vfio->pdev->dev; > + cmd.client_reg.opcode = PDS_AQ_CMD_CLIENT_REG; > + snprintf(cmd.client_reg.devname, sizeof(cmd.client_reg.devname), > + "%s.%d-%u", PDS_LM_DEV_NAME, > + pci_domain_nr(pds_vfio->pdev->bus), id); > + > + err = pdsc_adminq_post(pds_vfio->pdsc, &cmd, &comp, false); > + if (err) { > + dev_info(dev, "register with DSC failed, status %d: %pe\n", > + comp.status, ERR_PTR(err)); > + return err; > + } > + > + ci = le16_to_cpu(comp.client_reg.client_id); > + if (!ci) { > + dev_err(dev, "%s: device returned null client_id\n", __func__); > + return -EIO; > + } > + pds_vfio->client_id = ci; > + > + return 0; > +} > + > +void > +pds_vfio_unregister_client_cmd(struct pds_vfio_pci_device *pds_vfio) > +{ > + union pds_core_adminq_comp comp = { 0 }; > + union pds_core_adminq_cmd cmd = { 0 }; > + struct device *dev; > + int err; > + > + dev = &pds_vfio->pdev->dev; > + cmd.client_unreg.opcode = PDS_AQ_CMD_CLIENT_UNREG; > + cmd.client_unreg.client_id = cpu_to_le16(pds_vfio->client_id); > + > + err = pdsc_adminq_post(pds_vfio->pdsc, &cmd, &comp, false); > + if (err) > + dev_info(dev, "unregister from DSC failed, status %d: %pe\n", > + comp.status, ERR_PTR(err)); > + > + pds_vfio->client_id = 0; > +} > diff --git a/drivers/vfio/pci/pds/cmds.h b/drivers/vfio/pci/pds/cmds.h > new file mode 100644 > index 000000000000..4c592afccf89 > --- /dev/null > +++ b/drivers/vfio/pci/pds/cmds.h > @@ -0,0 +1,10 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Copyright(c) 2023 Advanced Micro Devices, Inc. */ > + > +#ifndef _CMDS_H_ > +#define _CMDS_H_ > + > +int pds_vfio_register_client_cmd(struct pds_vfio_pci_device *pds_vfio); > +void pds_vfio_unregister_client_cmd(struct pds_vfio_pci_device *pds_vfio); > + > +#endif /* _CMDS_H_ */ > diff --git a/drivers/vfio/pci/pds/pci_drv.c b/drivers/vfio/pci/pds/pci_drv.c > index 5e554420792e..46537afdee2d 100644 > --- a/drivers/vfio/pci/pds/pci_drv.c > +++ b/drivers/vfio/pci/pds/pci_drv.c > @@ -8,9 +8,13 @@ > #include <linux/types.h> > #include <linux/vfio.h> > > +#include <linux/pds/pds_common.h> > #include <linux/pds/pds_core_if.h> > +#include <linux/pds/pds_adminq.h> > > #include "vfio_dev.h" > +#include "pci_drv.h" > +#include "cmds.h" > > #define PDS_VFIO_DRV_NAME "pds_vfio" > #define PDS_VFIO_DRV_DESCRIPTION "AMD/Pensando VFIO Device Driver" > @@ -30,13 +34,23 @@ pds_vfio_pci_probe(struct pci_dev *pdev, > > dev_set_drvdata(&pdev->dev, &pds_vfio->vfio_coredev); > pds_vfio->pdev = pdev; > + pds_vfio->pdsc = pdsc_get_pf_struct(pdev); > + > + err = pds_vfio_register_client_cmd(pds_vfio); > + if (err) { > + dev_err(&pdev->dev, "failed to register as client: %pe\n", > + ERR_PTR(err)); > + goto out_put_vdev; > + } > > err = vfio_pci_core_register_device(&pds_vfio->vfio_coredev); > if (err) > - goto out_put_vdev; > + goto out_unreg_client; > > return 0; > > +out_unreg_client: > + pds_vfio_unregister_client_cmd(pds_vfio); > out_put_vdev: > vfio_put_device(&pds_vfio->vfio_coredev.vdev); > return err; > diff --git a/drivers/vfio/pci/pds/pci_drv.h b/drivers/vfio/pci/pds/pci_drv.h > new file mode 100644 > index 000000000000..e79bed12ed14 > --- /dev/null > +++ b/drivers/vfio/pci/pds/pci_drv.h > @@ -0,0 +1,9 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Copyright(c) 2023 Advanced Micro Devices, Inc. */ > + > +#ifndef _PCI_DRV_H > +#define _PCI_DRV_H > + > +#include <linux/pci.h> > + > +#endif /* _PCI_DRV_H */ > diff --git a/drivers/vfio/pci/pds/vfio_dev.c b/drivers/vfio/pci/pds/vfio_dev.c > index 0f70efec01e1..056715dea512 100644 > --- a/drivers/vfio/pci/pds/vfio_dev.c > +++ b/drivers/vfio/pci/pds/vfio_dev.c > @@ -31,6 +31,11 @@ pds_vfio_init_device(struct vfio_device *vdev) > pds_vfio->vf_id = pci_iov_vf_id(pdev); > pds_vfio->pci_id = PCI_DEVID(pdev->bus->number, pdev->devfn); > > + dev_dbg(&pdev->dev, "%s: PF %#04x VF %#04x (%d) vf_id %d domain %d pds_vfio %p\n", > + __func__, pci_dev_id(pdev->physfn), > + pds_vfio->pci_id, pds_vfio->pci_id, pds_vfio->vf_id, > + pci_domain_nr(pdev->bus), pds_vfio); > + > return 0; > } > > diff --git a/drivers/vfio/pci/pds/vfio_dev.h b/drivers/vfio/pci/pds/vfio_dev.h > index a66f8069b88c..10557e8dc829 100644 > --- a/drivers/vfio/pci/pds/vfio_dev.h > +++ b/drivers/vfio/pci/pds/vfio_dev.h > @@ -10,9 +10,11 @@ > struct pds_vfio_pci_device { > struct vfio_pci_core_device vfio_coredev; > struct pci_dev *pdev; > + void *pdsc; > > int vf_id; > int pci_id; > + u16 client_id; > }; > > const struct vfio_device_ops *pds_vfio_ops_info(void); > diff --git a/include/linux/pds/pds_lm.h b/include/linux/pds/pds_lm.h > new file mode 100644 > index 000000000000..2bc2bf79426e > --- /dev/null > +++ b/include/linux/pds/pds_lm.h > @@ -0,0 +1,12 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Copyright(c) 2022 Pensando Systems, Inc */ > + > +#ifndef _PDS_LM_H_ > +#define _PDS_LM_H_ > + > +#include "pds_common.h" > + > +#define PDS_DEV_TYPE_LM_STR "LM" > +#define PDS_LM_DEV_NAME PDS_CORE_DRV_NAME "." PDS_DEV_TYPE_LM_STR > + > +#endif /* _PDS_LM_H_ */