| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20210210234639.GI4247@nvidia.com>
Date: Wed, 10 Feb 2021 19:46:39 -0400
From: Jason Gunthorpe <jgg@...dia.com>
To: Dave Jiang <dave.jiang@...el.com>
CC: <alex.williamson@...hat.com>, <kwankhede@...dia.com>,
<tglx@...utronix.de>, <vkoul@...nel.org>, <megha.dey@...el.com>,
<jacob.jun.pan@...el.com>, <ashok.raj@...el.com>,
<yi.l.liu@...el.com>, <baolu.lu@...el.com>, <kevin.tian@...el.com>,
<sanjay.k.kumar@...el.com>, <tony.luck@...el.com>,
<dan.j.williams@...el.com>, <eric.auger@...hat.com>,
<parav@...lanox.com>, <netanelg@...lanox.com>,
<shahafs@...lanox.com>, <pbonzini@...hat.com>,
<dmaengine@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<kvm@...r.kernel.org>
Subject: Re: [PATCH v5 04/14] vfio/mdev: idxd: Add auxialary device plumbing
for idxd mdev support
On Fri, Feb 05, 2021 at 01:53:18PM -0700, Dave Jiang wrote:
> diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
> index a2438b3166db..f02c96164515 100644
> +++ b/drivers/dma/idxd/idxd.h
> @@ -8,6 +8,7 @@
> #include <linux/percpu-rwsem.h>
> #include <linux/wait.h>
> #include <linux/cdev.h>
> +#include <linux/auxiliary_bus.h>
> #include "registers.h"
>
> #define IDXD_DRIVER_VERSION "1.00"
> @@ -221,6 +222,8 @@ struct idxd_device {
> struct work_struct work;
>
> int *int_handles;
> +
> + struct auxiliary_device *mdev_auxdev;
> };
If there is only one aux device there not much reason to make it a
dedicated allocation.
> /* IDXD software descriptor */
> @@ -282,6 +285,10 @@ enum idxd_interrupt_type {
> IDXD_IRQ_IMS,
> };
>
> +struct idxd_mdev_aux_drv {
> + struct auxiliary_driver auxiliary_drv;
> +};
Wrong indent. What is this even for?
> +
> static inline int idxd_get_wq_portal_offset(enum idxd_portal_prot prot,
> enum idxd_interrupt_type irq_type)
> {
> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index ee56b92108d8..fd57f39e4b7d 100644
> +++ b/drivers/dma/idxd/init.c
> @@ -382,6 +382,74 @@ static void idxd_disable_system_pasid(struct idxd_device *idxd)
> idxd->sva = NULL;
> }
>
> +static void idxd_remove_mdev_auxdev(struct idxd_device *idxd)
> +{
> + if (!IS_ENABLED(CONFIG_VFIO_MDEV_IDXD))
> + return;
> +
> + auxiliary_device_delete(idxd->mdev_auxdev);
> + auxiliary_device_uninit(idxd->mdev_auxdev);
> +}
> +
> +static void idxd_auxdev_release(struct device *dev)
> +{
> + struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
> + struct idxd_device *idxd = dev_get_drvdata(dev);
Nope, where did you see drvdata being used like this? You need to use
container_of.
If put the mdev_auxdev as a non pointer member then this is just:
struct idxd_device *idxd = container_of(dev, struct idxd_device, mdev_auxdev)
put_device(&idxd->conf_dev);
And fix the 'setup' to match this design
> + kfree(auxdev->name);
This is weird, the name shouldn't be allocated, it is supposed to be a
fixed string to make it easy to find the driver name in the code base.
> +static int idxd_setup_mdev_auxdev(struct idxd_device *idxd)
> +{
> + struct auxiliary_device *auxdev;
> + struct device *dev = &idxd->pdev->dev;
> + int rc;
> +
> + if (!IS_ENABLED(CONFIG_VFIO_MDEV_IDXD))
> + return 0;
> +
> + auxdev = kzalloc(sizeof(*auxdev), GFP_KERNEL);
> + if (!auxdev)
> + return -ENOMEM;
> +
> + auxdev->name = kasprintf(GFP_KERNEL, "mdev-%s", idxd_name[idxd->type]);
> + if (!auxdev->name) {
> + rc = -ENOMEM;
> + goto err_name;
> + }
> +
> + dev_dbg(&idxd->pdev->dev, "aux dev mdev: %s\n", auxdev->name);
> +
> + auxdev->dev.parent = dev;
> + auxdev->dev.release = idxd_auxdev_release;
> + auxdev->id = idxd->id;
> +
> + rc = auxiliary_device_init(auxdev);
> + if (rc < 0) {
> + dev_err(dev, "Failed to init aux dev: %d\n", rc);
> + goto err_auxdev;
> + }
Put the init earlier so it can handle the error unwinds
> + rc = auxiliary_device_add(auxdev);
> + if (rc < 0) {
> + dev_err(dev, "Failed to add aux dev: %d\n", rc);
> + goto err_auxdev;
> + }
> +
> + idxd->mdev_auxdev = auxdev;
> + dev_set_drvdata(&auxdev->dev, idxd);
No to using drvdata, and this is in the wrong order anyhow.
> + return 0;
> +
> + err_auxdev:
> + kfree(auxdev->name);
> + err_name:
> + kfree(auxdev);
> + return rc;
> +}
> +
> static int idxd_probe(struct idxd_device *idxd)
> {
> struct pci_dev *pdev = idxd->pdev;
> @@ -434,11 +502,19 @@ static int idxd_probe(struct idxd_device *idxd)
> goto err_idr_fail;
> }
>
> + rc = idxd_setup_mdev_auxdev(idxd);
> + if (rc < 0)
> + goto err_auxdev_fail;
> +
> idxd->major = idxd_cdev_get_major(idxd);
>
> dev_dbg(dev, "IDXD device %d probed successfully\n", idxd->id);
> return 0;
>
> + err_auxdev_fail:
> + mutex_lock(&idxd_idr_lock);
> + idr_remove(&idxd_idrs[idxd->type], idxd->id);
> + mutex_unlock(&idxd_idr_lock);
Probably wrong to order things like this..
Also somehow this has a
idxd = devm_kzalloc(dev, sizeof(struct idxd_device), GFP_KERNEL);
but the idxd has a kref'd struct device in it:
struct idxd_device {
enum idxd_type type;
struct device conf_dev;
So that's not right either
You'll need to fix the lifetime model for idxd_device before you get
to adding auxdevices
> +static int idxd_mdev_host_init(struct idxd_device *idxd)
> +{
> + /* FIXME: Fill in later */
> + return 0;
> +}
> +
> +static int idxd_mdev_host_release(struct idxd_device *idxd)
> +{
> + /* FIXME: Fill in later */
> + return 0;
> +}
Don't leave empty stubs like this, just provide the whole driver in
the next patch
> +static int idxd_mdev_aux_probe(struct auxiliary_device *auxdev,
> + const struct auxiliary_device_id *id)
> +{
> + struct idxd_device *idxd = dev_get_drvdata(&auxdev->dev);
Continuing no to using drvdata, must use container_of
> + int rc;
> +
> + rc = idxd_mdev_host_init(idxd);
And why add this indirection? Just write what it here
> +static struct idxd_mdev_aux_drv idxd_mdev_aux_drv = {
> + .auxiliary_drv = {
> + .id_table = idxd_mdev_auxbus_id_table,
> + .probe = idxd_mdev_aux_probe,
> + .remove = idxd_mdev_aux_remove,
> + },
> +};
Why idxd_mdev_aux_drv ? Does a later patch add something here?
> +static int idxd_mdev_auxdev_drv_register(struct idxd_mdev_aux_drv *drv)
> +{
> + return auxiliary_driver_register(&drv->auxiliary_drv);
> +}
> +
> +static void idxd_mdev_auxdev_drv_unregister(struct idxd_mdev_aux_drv *drv)
> +{
> + auxiliary_driver_unregister(&drv->auxiliary_drv);
> +}
> +
> +module_driver(idxd_mdev_aux_drv, idxd_mdev_auxdev_drv_register, idxd_mdev_auxdev_drv_unregister);
There is some auxillary driver macro that does this boilerplate
Jason
Powered by blists - more mailing lists