| Message ID | 20230404190141.57762-4-brett.creeley@amd.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | pds_vfio driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Tue, 4 Apr 2023 12:01:37 -0700 Brett Creeley <brett.creeley@amd.com> 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@amd.com> > Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> > --- > 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_ */
On 4/10/2023 1:41 PM, Alex Williamson wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > On Tue, 4 Apr 2023 12:01:37 -0700 > Brett Creeley <brett.creeley@amd.com> 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@amd.com> >> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com> >> --- >> 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 > Yeah, these are all good catches and I'm not sure how I didn't notice them before. I will rework these bits based on your feedback. Thanks, Brett >> + >> + 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_ */ >
On Tue, Apr 04, 2023 at 12:01:37PM -0700, Brett Creeley wrote: > @@ -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); This should not be a void *, it has a type, looks like it is 'struct pdsc *' - comment applies to all the places in both series that dropped the type here. Jason
On 4/14/23 5:43 AM, Jason Gunthorpe wrote: > > On Tue, Apr 04, 2023 at 12:01:37PM -0700, Brett Creeley wrote: >> @@ -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); > > This should not be a void *, it has a type, looks like it is 'struct > pdsc *' - comment applies to all the places in both series that > dropped the type here. Thanks, Jason. I'll fix this up with a proper type in the pds_core patchset so that the pds_vfio can follow. sln
On 4/14/2023 5:43 AM, Jason Gunthorpe wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > On Tue, Apr 04, 2023 at 12:01:37PM -0700, Brett Creeley wrote: >> @@ -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); > > This should not be a void *, it has a type, looks like it is 'struct > pdsc *' - comment applies to all the places in both series that > dropped the type here. Will fix. > > Jason Hey Jason, Thanks for the responses/feedback. For some reason Shannon and I didn't get any of your recent responses in our inboxes except this one. We're not really sure why... Due to this, I'm replying to all of your responses in this thread. >> + union pds_core_adminq_cmd cmd = { 0 }; > These should all be = {}, adding the 0 is a subtly different thing in Will fix. >> +int >> +pds_vfio_suspend_device_cmd(struct pds_vfio_pci_device *pds_vfio) >> +{ >> + struct pds_lm_suspend_cmd cmd = { >> + .opcode = PDS_LM_CMD_SUSPEND, >> + .vf_id = cpu_to_le16(pds_vfio->vf_id), >> + }; >> + struct pds_lm_suspend_comp comp = {0}; >> + struct pci_dev *pdev = pds_vfio->pdev; >> + int err; >> + >> + dev_dbg(&pdev->dev, "vf%u: Suspend device\n", pds_vfio->vf_id); >> + >> + err = pds_client_adminq_cmd(pds_vfio, >> + (union pds_core_adminq_cmd *)&cmd, >> + sizeof(cmd), >> + (union pds_core_adminq_comp *)&comp, >> + PDS_AQ_FLAG_FASTPOLL); > These casts to a union are really weird, why isn't the union the type > on the stack? Yeah, this is an artifact of initial development that allowed us to completely de-couple pds_lm.h from pds_adminq.h, but there don't seem to be any conflicts including inluding pds_lm.h in pds_adminq.h. So, it will be fixed in the next revision. >> + >> + /* alloc sgl */ >> + sgl = dma_alloc_coherent(dev, lm_file->num_sge * >> + sizeof(struct pds_lm_sg_elem), >> + &lm_file->sgl_addr, GFP_KERNEL); > Do you really need a coherent allocation for this? I don't think it is needed. I will look into this and fix it if dma_alloc_coherent() isn't needed. >> +#define PDS_VFIO_LM_FILENAME "pds_vfio_lm" > This doesn't need a define, it is typical to write the pseudo filename > in the only anon_inode_getfile() Yeah, we technically only use it in one spot, so it makes sense to just have the string inlined to the anon_inode_getfile() call. This also allows us to get rid of the const char *name argument in pds_vfio_get_lm_file(). Will fix in the next revision. >> +static struct pds_vfio_lm_file * >> +pds_vfio_get_lm_file(const char *name, const struct file_operations *fops, >> + int flags, u64 size) >> +{ >> + struct pds_vfio_lm_file *lm_file = NULL; >> + unsigned long long npages; >> + struct page **pages; >> + int err = 0; >> + >> + if (!size) >> + return NULL; >> + >> + /* Alloc file structure */ >> + lm_file = kzalloc(sizeof(*lm_file), GFP_KERNEL); >> + if (!lm_file) >> + return NULL; >> + >> + /* Create file */ >> + lm_file->filep = anon_inode_getfile(name, fops, lm_file, flags); >> + if (!lm_file->filep) >> + goto err_get_file; >> + >> + stream_open(lm_file->filep->f_inode, lm_file->filep); >> + mutex_init(&lm_file->lock); >> + >> + lm_file->size = size; >> + >> + /* Allocate memory for file pages */ >> + npages = DIV_ROUND_UP_ULL(lm_file->size, PAGE_SIZE); >> + >> + pages = kcalloc(npages, sizeof(*pages), GFP_KERNEL); >> + if (!pages) >> + goto err_alloc_pages; >> + >> + for (unsigned long long i = 0; i < npages; i++) { >> + pages[i] = alloc_page(GFP_KERNEL); >> + if (!pages[i]) >> + goto err_alloc_page; >> + } >> + >> + lm_file->pages = pages; >> + lm_file->npages = npages; >> + lm_file->alloc_size = npages * PAGE_SIZE; >> + >> + /* Create scatterlist of file pages to use for DMA mapping later */ >> + err = sg_alloc_table_from_pages(&lm_file->sg_table, pages, npages, >> + 0, size, GFP_KERNEL); >> + if (err) >> + goto err_alloc_sg_table; > This is the same basic thing the mlx5 driver does, you should move the > mlx5 code into some common place and just re-use it here. I looked at the mlx5 code and even though the two drivers are doing the same basic thing, IMHO it doesn't seem like a straight forward task as the mlx5 code seems to have some device/driver specifics mixed in. I'd prefer not trying to refactor/commonize this bit of code at this point in time. However, it does seem like a good future improvement once things quiet down after getting this initial series merged. >> diff --git a/drivers/vfio/pci/pds/vfio_dev.h b/drivers/vfio/pci/pds/vfio_dev.h >> index 10557e8dc829..3f55861ffc7c 100644 >> --- a/drivers/vfio/pci/pds/vfio_dev.h >> +++ b/drivers/vfio/pci/pds/vfio_dev.h >> @@ -7,10 +7,20 @@ >> #include <linux/pci.h> >> #include <linux/vfio_pci_core.h> >> >> +#include "lm.h" >> + >> struct pds_vfio_pci_device { >> struct vfio_pci_core_device vfio_coredev; >> struct pci_dev *pdev; >> void *pdsc; >> + struct device *coredev; > Why? If this is just &pdev->dev it it doesn't need to be in the struct > And pdev is just vfio_coredev->pdev, don't need to duplicate it either This was actually the pds_core's device structure. I have removed this in my local tree and instead use the pci_physfn() to get pds_core's struct device. Will be fixed in the next revision. >> +static void >> +pds_vfio_recovery_work(struct work_struct *work) >> +{ >> + struct pds_vfio_pci_device *pds_vfio = >> + container_of(work, struct pds_vfio_pci_device, work); >> + bool deferred_reset_needed = false; >> + >> + /* Documentation states that the kernel migration driver must not >> + * generate asynchronous device state transitions outside of >> + * manipulation by the user or the VFIO_DEVICE_RESET ioctl. >> + * >> + * Since recovery is an asynchronous event received from the device, >> + * initiate a deferred reset. Only issue the deferred reset if a >> + * migration is in progress, which will cause the next step of the >> + * migration to fail. Also, if the device is in a state that will >> + * be set to VFIO_DEVICE_STATE_RUNNING on the next action (i.e. VM is >> + * shutdown and device is in VFIO_DEVICE_STATE_STOP) as that will clear >> + * the VFIO_DEVICE_STATE_ERROR when the VM starts back up. >> + */ >> + mutex_lock(&pds_vfio->state_mutex); >> + if ((pds_vfio->state != VFIO_DEVICE_STATE_RUNNING && >> + pds_vfio->state != VFIO_DEVICE_STATE_ERROR) || >> + (pds_vfio->state == VFIO_DEVICE_STATE_RUNNING && >> + pds_vfio_dirty_is_enabled(pds_vfio))) >> + deferred_reset_needed = true; >> + mutex_unlock(&pds_vfio->state_mutex); >> + >> + /* On the next user initiated state transition, the device will >> + * transition to the VFIO_DEVICE_STATE_ERROR. At this point it's the user's >> + * responsibility to reset the device. >> + * >> + * If a VFIO_DEVICE_RESET is requested post recovery and before the next >> + * state transition, then the deferred reset state will be set to >> + * VFIO_DEVICE_STATE_RUNNING. >> + */ >> + if (deferred_reset_needed) >> + pds_vfio_deferred_reset(pds_vfio, VFIO_DEVICE_STATE_ERROR); >> +} > Why is this a work? it is threaded on a blocking_notifier_chain so it > can call the mutex? I think the work item can be dropped and the contents of the work function can be moved in the notifier callback. I will fix this in the next revision. > Why is the locking like this, can't you just call > pds_vfio_deferred_reset() under the mutex? It was done to avoid any lock ordering issues with pds_vfio_state_mutex_unlock() or pds_vfio_reset(). >> Add Kconfig entries and pds_vfio.rst. Also, add an entry in the >> MAINTAINERS file for this new driver. >> >> It's not clear where documentation for vendor specific VFIO >> drivers should live, so just re-use the current amd >> ethernet location. > It would be nice to make a kdoc section for vfio. It seems like there are already vfio docs in Documentation/driver-api/, but the kdoc added in this patch is slightly different since it's vendor specific. Which of the following locations make the most sense? [1] Documentation/vfio/<vendor>/<vendor_kdoc> - Documentation/vfio/amd/pds_vfio.rst [2] Documentation/vfio/vendor-drivers/<vendor_kdoc> - Documentation/vfio/vendor-drivers/pds_vfio.rst Thanks again for the time and feedback, Brett
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)); + + 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_ */