Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
On Tue, 27 Aug 2019 15:35:10 +0200 Cornelia Huck wrote: > On Tue, 27 Aug 2019 11:57:07 + > Parav Pandit wrote: > > > > -Original Message- > > > From: Cornelia Huck > > > Sent: Tuesday, August 27, 2019 5:11 PM > > > To: Parav Pandit > > > Cc: alex.william...@redhat.com; Jiri Pirko ; > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux- > > > ker...@vger.kernel.org; net...@vger.kernel.org > > > Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias > > > > > > On Tue, 27 Aug 2019 11:33:54 + > > > Parav Pandit wrote: > > > > > > > > -Original Message- > > > > > From: Cornelia Huck > > > > > Sent: Tuesday, August 27, 2019 4:54 PM > > > > > To: Parav Pandit > > > > > Cc: alex.william...@redhat.com; Jiri Pirko ; > > > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; > > > > > linux- ker...@vger.kernel.org; net...@vger.kernel.org > > > > > Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias > > > > > > > > > > On Tue, 27 Aug 2019 11:12:23 + > > > > > Parav Pandit wrote: > > > > > > > > > > > > -Original Message- > > > > > > > From: Cornelia Huck > > > > > > > Sent: Tuesday, August 27, 2019 3:54 PM > > > > > > > To: Parav Pandit > > > > > > > Cc: alex.william...@redhat.com; Jiri Pirko ; > > > > > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; > > > > > > > linux- ker...@vger.kernel.org; net...@vger.kernel.org > > > > > > > Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias > > > > > > > > > > > > > > > > > What about: > > > > > > > > > > > > > > * @get_alias_length: optional callback to specify length of the > > > > > > > alias to > > > > > create > > > > > > > *Returns unsigned integer: length of the > > > > > > > alias to be created, > > > > > > > * 0 to not create an > > > > > > > alias > > > > > > > > > > > > > Ack. > > > > > > > > > > > > > I also think it might be beneficial to add a device parameter > > > > > > > here now (rather than later); that seems to be something that > > > > > > > makes > > > sense. > > > > > > > > > > > > > Without showing the use, it shouldn't be added. > > > > > > > > > > It just feels like an omission: Why should the vendor driver only be > > > > > able to return one value here, without knowing which device it is for? > > > > > If a driver supports different devices, it may have different > > > > > requirements for them. > > > > > > > > > Sure. Lets first have this requirement to add it. > > > > I am against adding this length field itself without an actual vendor > > > > use case, > > > which is adding some complexity in code today. > > > > But it was ok to have length field instead of bool. > > > > > > > > Lets not further add "no-requirement futuristic knobs" which hasn't > > > > shown its > > > need yet. > > > > When a vendor driver needs it, there is nothing prevents such addition. > > > > > > > > > > Frankly, I do not see how it adds complexity; the other callbacks have > > > device > > > arguments already, > > Other ioctls such as create, remove, mmap, likely need to access the parent. > > Hence it make sense to have parent pointer in there. > > > > I am not against complexity, I am just saying, at present there is no > > use-case. Let have use case and we add it. > > > > > and the vendor driver is free to ignore it if it does not have > > > a use for it. I'd rather add the argument before a possible future user > > > tries > > > weird hacks to allow multiple values, but I'll leave the decision to the > > > maintainers. > > Why would a possible future user tries a weird hack? > > If
Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
On Tue, 27 Aug 2019 11:57:07 + Parav Pandit wrote: > > -Original Message- > > From: Cornelia Huck > > Sent: Tuesday, August 27, 2019 5:11 PM > > To: Parav Pandit > > Cc: alex.william...@redhat.com; Jiri Pirko ; > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux- > > ker...@vger.kernel.org; net...@vger.kernel.org > > Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias > > > > On Tue, 27 Aug 2019 11:33:54 + > > Parav Pandit wrote: > > > > > > -Original Message- > > > > From: Cornelia Huck > > > > Sent: Tuesday, August 27, 2019 4:54 PM > > > > To: Parav Pandit > > > > Cc: alex.william...@redhat.com; Jiri Pirko ; > > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; > > > > linux- ker...@vger.kernel.org; net...@vger.kernel.org > > > > Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias > > > > > > > > On Tue, 27 Aug 2019 11:12:23 + > > > > Parav Pandit wrote: > > > > > > > > > > -Original Message- > > > > > > From: Cornelia Huck > > > > > > Sent: Tuesday, August 27, 2019 3:54 PM > > > > > > To: Parav Pandit > > > > > > Cc: alex.william...@redhat.com; Jiri Pirko ; > > > > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; > > > > > > linux- ker...@vger.kernel.org; net...@vger.kernel.org > > > > > > Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias > > > > > > > > > > > > > > What about: > > > > > > > > > > > > * @get_alias_length: optional callback to specify length of the > > > > > > alias to > > > > create > > > > > > *Returns unsigned integer: length of the alias > > > > > > to be created, > > > > > > * 0 to not create an > > > > > > alias > > > > > > > > > > > Ack. > > > > > > > > > > > I also think it might be beneficial to add a device parameter > > > > > > here now (rather than later); that seems to be something that makes > > > > > > > > sense. > > > > > > > > > > > Without showing the use, it shouldn't be added. > > > > > > > > It just feels like an omission: Why should the vendor driver only be > > > > able to return one value here, without knowing which device it is for? > > > > If a driver supports different devices, it may have different > > > > requirements for them. > > > > > > > Sure. Lets first have this requirement to add it. > > > I am against adding this length field itself without an actual vendor use > > > case, > > which is adding some complexity in code today. > > > But it was ok to have length field instead of bool. > > > > > > Lets not further add "no-requirement futuristic knobs" which hasn't shown > > > its > > need yet. > > > When a vendor driver needs it, there is nothing prevents such addition. > > > > Frankly, I do not see how it adds complexity; the other callbacks have > > device > > arguments already, > Other ioctls such as create, remove, mmap, likely need to access the parent. > Hence it make sense to have parent pointer in there. > > I am not against complexity, I am just saying, at present there is no > use-case. Let have use case and we add it. > > > and the vendor driver is free to ignore it if it does not have > > a use for it. I'd rather add the argument before a possible future user > > tries > > weird hacks to allow multiple values, but I'll leave the decision to the > > maintainers. > Why would a possible future user tries a weird hack? > If user needs to access parent device, that driver maintainer should ask for > it. I've seen the situation often enough that folks tried to do hacks instead of enhancing the interface. Again, let's get a maintainer opinion.
RE: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
> -Original Message- > From: Cornelia Huck > Sent: Tuesday, August 27, 2019 5:11 PM > To: Parav Pandit > Cc: alex.william...@redhat.com; Jiri Pirko ; > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux- > ker...@vger.kernel.org; net...@vger.kernel.org > Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias > > On Tue, 27 Aug 2019 11:33:54 + > Parav Pandit wrote: > > > > -Original Message- > > > From: Cornelia Huck > > > Sent: Tuesday, August 27, 2019 4:54 PM > > > To: Parav Pandit > > > Cc: alex.william...@redhat.com; Jiri Pirko ; > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; > > > linux- ker...@vger.kernel.org; net...@vger.kernel.org > > > Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias > > > > > > On Tue, 27 Aug 2019 11:12:23 + > > > Parav Pandit wrote: > > > > > > > > -Original Message- > > > > > From: Cornelia Huck > > > > > Sent: Tuesday, August 27, 2019 3:54 PM > > > > > To: Parav Pandit > > > > > Cc: alex.william...@redhat.com; Jiri Pirko ; > > > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; > > > > > linux- ker...@vger.kernel.org; net...@vger.kernel.org > > > > > Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias > > > > > > > > > > > What about: > > > > > > > > > > * @get_alias_length: optional callback to specify length of the > > > > > alias to > > > create > > > > > *Returns unsigned integer: length of the alias to > > > > > be created, > > > > > * 0 to not create an > > > > > alias > > > > > > > > > Ack. > > > > > > > > > I also think it might be beneficial to add a device parameter > > > > > here now (rather than later); that seems to be something that makes > sense. > > > > > > > > > Without showing the use, it shouldn't be added. > > > > > > It just feels like an omission: Why should the vendor driver only be > > > able to return one value here, without knowing which device it is for? > > > If a driver supports different devices, it may have different > > > requirements for them. > > > > > Sure. Lets first have this requirement to add it. > > I am against adding this length field itself without an actual vendor use > > case, > which is adding some complexity in code today. > > But it was ok to have length field instead of bool. > > > > Lets not further add "no-requirement futuristic knobs" which hasn't shown > > its > need yet. > > When a vendor driver needs it, there is nothing prevents such addition. > > Frankly, I do not see how it adds complexity; the other callbacks have device > arguments already, Other ioctls such as create, remove, mmap, likely need to access the parent. Hence it make sense to have parent pointer in there. I am not against complexity, I am just saying, at present there is no use-case. Let have use case and we add it. > and the vendor driver is free to ignore it if it does not have > a use for it. I'd rather add the argument before a possible future user tries > weird hacks to allow multiple values, but I'll leave the decision to the > maintainers. Why would a possible future user tries a weird hack? If user needs to access parent device, that driver maintainer should ask for it.
Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
On Tue, 27 Aug 2019 11:33:54 + Parav Pandit wrote: > > -Original Message- > > From: Cornelia Huck > > Sent: Tuesday, August 27, 2019 4:54 PM > > To: Parav Pandit > > Cc: alex.william...@redhat.com; Jiri Pirko ; > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux- > > ker...@vger.kernel.org; net...@vger.kernel.org > > Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias > > > > On Tue, 27 Aug 2019 11:12:23 + > > Parav Pandit wrote: > > > > > > -Original Message- > > > > From: Cornelia Huck > > > > Sent: Tuesday, August 27, 2019 3:54 PM > > > > To: Parav Pandit > > > > Cc: alex.william...@redhat.com; Jiri Pirko ; > > > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; > > > > linux- ker...@vger.kernel.org; net...@vger.kernel.org > > > > Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias > > > > > > > > What about: > > > > > > > > * @get_alias_length: optional callback to specify length of the alias > > > > to > > create > > > > *Returns unsigned integer: length of the alias to > > > > be created, > > > > * 0 to not create an alias > > > > > > > Ack. > > > > > > > I also think it might be beneficial to add a device parameter here > > > > now (rather than later); that seems to be something that makes sense. > > > > > > > Without showing the use, it shouldn't be added. > > > > It just feels like an omission: Why should the vendor driver only be able to > > return one value here, without knowing which device it is for? > > If a driver supports different devices, it may have different requirements > > for > > them. > > > Sure. Lets first have this requirement to add it. > I am against adding this length field itself without an actual vendor use > case, which is adding some complexity in code today. > But it was ok to have length field instead of bool. > > Lets not further add "no-requirement futuristic knobs" which hasn't shown its > need yet. > When a vendor driver needs it, there is nothing prevents such addition. Frankly, I do not see how it adds complexity; the other callbacks have device arguments already, and the vendor driver is free to ignore it if it does not have a use for it. I'd rather add the argument before a possible future user tries weird hacks to allow multiple values, but I'll leave the decision to the maintainers.
RE: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
> -Original Message-
> From: Cornelia Huck
> Sent: Tuesday, August 27, 2019 4:54 PM
> To: Parav Pandit
> Cc: alex.william...@redhat.com; Jiri Pirko ;
> kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
>
> On Tue, 27 Aug 2019 11:12:23 +
> Parav Pandit wrote:
>
> > > -Original Message-
> > > From: Cornelia Huck
> > > Sent: Tuesday, August 27, 2019 3:54 PM
> > > To: Parav Pandit
> > > Cc: alex.william...@redhat.com; Jiri Pirko ;
> > > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org;
> > > linux- ker...@vger.kernel.org; net...@vger.kernel.org
> > > Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
> > >
> > > On Mon, 26 Aug 2019 15:41:16 -0500
> > > Parav Pandit wrote:
> > >
> > > > Whenever a parent requests to generate mdev alias, generate a mdev
> > > > alias.
> > > > It is an optional attribute that parent can request to generate
> > > > for each of its child mdev.
> > > > mdev alias is generated using sha1 from the mdev name.
> > >
> > > Maybe add some motivation here as well?
> > >
> > > "Some vendor drivers want an identifier for an mdev device that is
> > > shorter than the uuid, due to length restrictions in the consumers of that
> identifier.
> > >
> > > Add a callback that allows a vendor driver to request an alias of a
> > > specified length to be generated (via sha1) for an mdev device. If
> > > generated, that alias is checked for collisions."
> > >
> > I did described the motivation in the cover letter with example and this
> design discussion thread.
>
> Yes, but adding it to the patch description makes it available in the git
> history.
>
Ok.
> > I will include above summary in v1.
> >
> > > What about:
> > >
> > > * @get_alias_length: optional callback to specify length of the alias to
> create
> > > *Returns unsigned integer: length of the alias to be
> > > created,
> > > * 0 to not create an alias
> > >
> > Ack.
> >
> > > I also think it might be beneficial to add a device parameter here
> > > now (rather than later); that seems to be something that makes sense.
> > >
> > Without showing the use, it shouldn't be added.
>
> It just feels like an omission: Why should the vendor driver only be able to
> return one value here, without knowing which device it is for?
> If a driver supports different devices, it may have different requirements for
> them.
>
Sure. Lets first have this requirement to add it.
I am against adding this length field itself without an actual vendor use case,
which is adding some complexity in code today.
But it was ok to have length field instead of bool.
Lets not further add "no-requirement futuristic knobs" which hasn't shown its
need yet.
When a vendor driver needs it, there is nothing prevents such addition.
> >
> > > > * Parent device that support mediated device should be
> > > > registered with
> > > mdev
> > > > * module with mdev_parent_ops structure.
> > > > **/
> > > > @@ -92,6 +95,7 @@ struct mdev_parent_ops {
> > > > long(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
> > > > unsigned long arg);
> > > > int (*mmap)(struct mdev_device *mdev, struct vm_area_struct
> > > *vma);
> > > > + unsigned int (*get_alias_length)(void);
> > > > };
> > > >
> > > > /* interface for exporting mdev supported type attributes */
> >
Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
On Tue, 27 Aug 2019 11:12:23 +
Parav Pandit wrote:
> > -Original Message-
> > From: Cornelia Huck
> > Sent: Tuesday, August 27, 2019 3:54 PM
> > To: Parav Pandit
> > Cc: alex.william...@redhat.com; Jiri Pirko ;
> > kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> > ker...@vger.kernel.org; net...@vger.kernel.org
> > Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
> >
> > On Mon, 26 Aug 2019 15:41:16 -0500
> > Parav Pandit wrote:
> >
> > > Whenever a parent requests to generate mdev alias, generate a mdev
> > > alias.
> > > It is an optional attribute that parent can request to generate for
> > > each of its child mdev.
> > > mdev alias is generated using sha1 from the mdev name.
> >
> > Maybe add some motivation here as well?
> >
> > "Some vendor drivers want an identifier for an mdev device that is shorter
> > than
> > the uuid, due to length restrictions in the consumers of that identifier.
> >
> > Add a callback that allows a vendor driver to request an alias of a
> > specified
> > length to be generated (via sha1) for an mdev device. If generated, that
> > alias is
> > checked for collisions."
> >
> I did described the motivation in the cover letter with example and this
> design discussion thread.
Yes, but adding it to the patch description makes it available in the
git history.
> I will include above summary in v1.
>
> > What about:
> >
> > * @get_alias_length: optional callback to specify length of the alias to
> > create
> > *Returns unsigned integer: length of the alias to be
> > created,
> > * 0 to not create an alias
> >
> Ack.
>
> > I also think it might be beneficial to add a device parameter here now
> > (rather
> > than later); that seems to be something that makes sense.
> >
> Without showing the use, it shouldn't be added.
It just feels like an omission: Why should the vendor driver only be
able to return one value here, without knowing which device it is for?
If a driver supports different devices, it may have different
requirements for them.
>
> > > * Parent device that support mediated device should be registered with
> > mdev
> > > * module with mdev_parent_ops structure.
> > > **/
> > > @@ -92,6 +95,7 @@ struct mdev_parent_ops {
> > > long(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
> > >unsigned long arg);
> > > int (*mmap)(struct mdev_device *mdev, struct vm_area_struct
> > *vma);
> > > + unsigned int (*get_alias_length)(void);
> > > };
> > >
> > > /* interface for exporting mdev supported type attributes */
>
RE: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
> -Original Message-
> From: Cornelia Huck
> Sent: Tuesday, August 27, 2019 3:54 PM
> To: Parav Pandit
> Cc: alex.william...@redhat.com; Jiri Pirko ;
> kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
>
> On Mon, 26 Aug 2019 15:41:16 -0500
> Parav Pandit wrote:
> >
> > static int __init mdev_init(void)
> > {
> > + alias_hash = crypto_alloc_shash("sha1", 0, 0);
> > + if (!alias_hash)
> > + return -ENOMEM;
> > +
> > return mdev_bus_register();
>
> Don't you need to call crypto_free_shash() if mdev_bus_register() fails?
>
Missed to answer this in previous reply.
Yes, took care of it in v1.
Mark Bloch also pointed it to me.
RE: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
> -Original Message-
> From: Cornelia Huck
> Sent: Tuesday, August 27, 2019 3:54 PM
> To: Parav Pandit
> Cc: alex.william...@redhat.com; Jiri Pirko ;
> kwankh...@nvidia.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
>
> On Mon, 26 Aug 2019 15:41:16 -0500
> Parav Pandit wrote:
>
> > Whenever a parent requests to generate mdev alias, generate a mdev
> > alias.
> > It is an optional attribute that parent can request to generate for
> > each of its child mdev.
> > mdev alias is generated using sha1 from the mdev name.
>
> Maybe add some motivation here as well?
>
> "Some vendor drivers want an identifier for an mdev device that is shorter
> than
> the uuid, due to length restrictions in the consumers of that identifier.
>
> Add a callback that allows a vendor driver to request an alias of a specified
> length to be generated (via sha1) for an mdev device. If generated, that
> alias is
> checked for collisions."
>
I did described the motivation in the cover letter with example and this design
discussion thread.
I will include above summary in v1.
> What about:
>
> * @get_alias_length: optional callback to specify length of the alias to
> create
> *Returns unsigned integer: length of the alias to be
> created,
> * 0 to not create an alias
>
Ack.
> I also think it might be beneficial to add a device parameter here now (rather
> than later); that seems to be something that makes sense.
>
Without showing the use, it shouldn't be added.
> > * Parent device that support mediated device should be registered with
> mdev
> > * module with mdev_parent_ops structure.
> > **/
> > @@ -92,6 +95,7 @@ struct mdev_parent_ops {
> > long(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
> > unsigned long arg);
> > int (*mmap)(struct mdev_device *mdev, struct vm_area_struct
> *vma);
> > + unsigned int (*get_alias_length)(void);
> > };
> >
> > /* interface for exporting mdev supported type attributes */
Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
On Mon, 26 Aug 2019 15:41:16 -0500
Parav Pandit wrote:
> Whenever a parent requests to generate mdev alias, generate a mdev
> alias.
> It is an optional attribute that parent can request to generate
> for each of its child mdev.
> mdev alias is generated using sha1 from the mdev name.
Maybe add some motivation here as well?
"Some vendor drivers want an identifier for an mdev device that is
shorter than the uuid, due to length restrictions in the consumers of
that identifier.
Add a callback that allows a vendor driver to request an alias of a
specified length to be generated (via sha1) for an mdev device. If
generated, that alias is checked for collisions."
>
> Signed-off-by: Parav Pandit
> ---
> drivers/vfio/mdev/mdev_core.c| 98 +++-
> drivers/vfio/mdev/mdev_private.h | 5 +-
> drivers/vfio/mdev/mdev_sysfs.c | 13 +++--
> include/linux/mdev.h | 4 ++
> 4 files changed, 111 insertions(+), 9 deletions(-)
>
(...)
> @@ -406,6 +495,10 @@ EXPORT_SYMBOL(mdev_get_iommu_device);
>
> static int __init mdev_init(void)
> {
> + alias_hash = crypto_alloc_shash("sha1", 0, 0);
> + if (!alias_hash)
> + return -ENOMEM;
> +
> return mdev_bus_register();
Don't you need to call crypto_free_shash() if mdev_bus_register() fails?
> }
>
> @@ -415,6 +508,7 @@ static void __exit mdev_exit(void)
> class_compat_unregister(mdev_bus_compat_class);
>
> mdev_bus_unregister();
> + crypto_free_shash(alias_hash);
> }
>
> module_init(mdev_init)
(...)
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index 0ce30ca78db0..f036fe9854ee 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -72,6 +72,9 @@ struct device *mdev_get_iommu_device(struct device *dev);
> * @mmap:mmap callback
> * @mdev: mediated device structure
> * @vma: vma structure
> + * @get_alias_length:Generate alias for the mdevs of this parent
> based on the
> + * mdev device name when it returns non zero alias length.
> + * It is optional.
What about:
* @get_alias_length: optional callback to specify length of the alias to create
*Returns unsigned integer: length of the alias to be
created,
* 0 to not create an alias
I also think it might be beneficial to add a device parameter here now
(rather than later); that seems to be something that makes sense.
> * Parent device that support mediated device should be registered with mdev
> * module with mdev_parent_ops structure.
> **/
> @@ -92,6 +95,7 @@ struct mdev_parent_ops {
> long(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
>unsigned long arg);
> int (*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma);
> + unsigned int (*get_alias_length)(void);
> };
>
> /* interface for exporting mdev supported type attributes */
RE: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
> -Original Message-
> From: Alex Williamson
> Sent: Tuesday, August 27, 2019 7:15 AM
> To: Parav Pandit
> Cc: Jiri Pirko ; kwankh...@nvidia.com;
> coh...@redhat.com; da...@davemloft.net; k...@vger.kernel.org; linux-
> ker...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
>
> On Mon, 26 Aug 2019 15:41:16 -0500
> Parav Pandit wrote:
>
> > Whenever a parent requests to generate mdev alias, generate a mdev
> > alias.
> > It is an optional attribute that parent can request to generate for
> > each of its child mdev.
> > mdev alias is generated using sha1 from the mdev name.
> >
> > Signed-off-by: Parav Pandit
> > ---
> > drivers/vfio/mdev/mdev_core.c| 98
> +++-
> > drivers/vfio/mdev/mdev_private.h | 5 +-
> > drivers/vfio/mdev/mdev_sysfs.c | 13 +++--
> > include/linux/mdev.h | 4 ++
> > 4 files changed, 111 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/vfio/mdev/mdev_core.c
> > b/drivers/vfio/mdev/mdev_core.c index b558d4cfd082..e825ff38b037
> > 100644
> > --- a/drivers/vfio/mdev/mdev_core.c
> > +++ b/drivers/vfio/mdev/mdev_core.c
> > @@ -10,9 +10,11 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "mdev_private.h"
> >
> > @@ -27,6 +29,8 @@ static struct class_compat *mdev_bus_compat_class;
> > static LIST_HEAD(mdev_list); static DEFINE_MUTEX(mdev_list_lock);
> >
> > +static struct crypto_shash *alias_hash;
> > +
> > struct device *mdev_parent_dev(struct mdev_device *mdev) {
> > return mdev->parent->dev;
> > @@ -164,6 +168,18 @@ int mdev_register_device(struct device *dev, const
> struct mdev_parent_ops *ops)
> > goto add_dev_err;
> > }
> >
> > + if (ops->get_alias_length) {
> > + unsigned int digest_size;
> > + unsigned int aligned_len;
> > +
> > + aligned_len = roundup(ops->get_alias_length(), 2);
> > + digest_size = crypto_shash_digestsize(alias_hash);
> > + if (aligned_len / 2 > digest_size) {
> > + ret = -EINVAL;
> > + goto add_dev_err;
> > + }
> > + }
>
> This looks like a sanity check, it could be done outside of the
> parent_list_lock, even before we get a parent device reference.
>
Yes.
> I think we're using a callback for get_alias_length() rather than a fixed
> field
> to support the mtty module option added in patch 4, right?
Right.
I will move the check outside.
> Its utility is rather limited with no args. I could imagine that if a parent
> wanted to generate an alias that could be incorporated into a string with the
> parent device name that it would be useful to call this with the parent
> device as an arg. I guess we can save that until a user comes along though.
>
Right. We save until user arrives.
I suggest you review the extra complexity I added here for vendor driven alias
length, which I think we should do when an actual user comes along.
> There doesn't seem to be anything serializing use of alias_hash.
>
Each sha1 calculation is happening on the new descriptor allocated and
initialized using crypto_shash_init().
So it appears to me that each hash calculation can occur in parallel on the
individual desc.
> > +
> > parent = kzalloc(sizeof(*parent), GFP_KERNEL);
> > if (!parent) {
> > ret = -ENOMEM;
> > @@ -259,6 +275,7 @@ static void mdev_device_free(struct mdev_device
> *mdev)
> > mutex_unlock(_list_lock);
> >
> > dev_dbg(>dev, "MDEV: destroying\n");
> > + kvfree(mdev->alias);
> > kfree(mdev);
> > }
> >
> > @@ -269,18 +286,86 @@ static void mdev_device_release(struct device
> *dev)
> > mdev_device_free(mdev);
> > }
> >
> > -int mdev_device_create(struct kobject *kobj,
> > - struct device *dev, const guid_t *uuid)
> > +static const char *
> > +generate_alias(const char *uuid, unsigned int max_alias_len) {
> > + struct shash_desc *hash_desc;
> > + unsigned int digest_size;
> > + unsigned char *digest;
> > + unsigned int alias_len;
> > + char *alias;
> > + int ret = 0;
> > +
> > + /* Align to multiple of 2 as bin2hex will generate
> > +* even number of bytes.
> > +*/
>
> Comment style for non-networking code please
Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
On Mon, 26 Aug 2019 19:44:56 -0600
Alex Williamson wrote:
> On Mon, 26 Aug 2019 15:41:16 -0500
> Parav Pandit wrote:
>
> > Whenever a parent requests to generate mdev alias, generate a mdev
> > alias.
> > It is an optional attribute that parent can request to generate
> > for each of its child mdev.
> > mdev alias is generated using sha1 from the mdev name.
> >
> > Signed-off-by: Parav Pandit
> > ---
> > drivers/vfio/mdev/mdev_core.c| 98 +++-
> > drivers/vfio/mdev/mdev_private.h | 5 +-
> > drivers/vfio/mdev/mdev_sysfs.c | 13 +++--
> > include/linux/mdev.h | 4 ++
> > 4 files changed, 111 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> > index b558d4cfd082..e825ff38b037 100644
> > --- a/drivers/vfio/mdev/mdev_core.c
> > +++ b/drivers/vfio/mdev/mdev_core.c
> > @@ -10,9 +10,11 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "mdev_private.h"
> >
> > @@ -27,6 +29,8 @@ static struct class_compat *mdev_bus_compat_class;
> > static LIST_HEAD(mdev_list);
> > static DEFINE_MUTEX(mdev_list_lock);
> >
> > +static struct crypto_shash *alias_hash;
> > +
> > struct device *mdev_parent_dev(struct mdev_device *mdev)
> > {
> > return mdev->parent->dev;
> > @@ -164,6 +168,18 @@ int mdev_register_device(struct device *dev, const
> > struct mdev_parent_ops *ops)
> > goto add_dev_err;
> > }
> >
> > + if (ops->get_alias_length) {
> > + unsigned int digest_size;
> > + unsigned int aligned_len;
> > +
> > + aligned_len = roundup(ops->get_alias_length(), 2);
> > + digest_size = crypto_shash_digestsize(alias_hash);
> > + if (aligned_len / 2 > digest_size) {
> > + ret = -EINVAL;
> > + goto add_dev_err;
> > + }
> > + }
>
> This looks like a sanity check, it could be done outside of the
> parent_list_lock, even before we get a parent device reference.
>
> I think we're using a callback for get_alias_length() rather than a
> fixed field to support the mtty module option added in patch 4, right?
> Its utility is rather limited with no args. I could imagine that if a
> parent wanted to generate an alias that could be incorporated into a
> string with the parent device name that it would be useful to call this
> with the parent device as an arg. I guess we can save that until a
> user comes along though.
>
> There doesn't seem to be anything serializing use of alias_hash.
>
> > +
> > parent = kzalloc(sizeof(*parent), GFP_KERNEL);
> > if (!parent) {
> > ret = -ENOMEM;
> > @@ -259,6 +275,7 @@ static void mdev_device_free(struct mdev_device *mdev)
> > mutex_unlock(_list_lock);
> >
> > dev_dbg(>dev, "MDEV: destroying\n");
> > + kvfree(mdev->alias);
> > kfree(mdev);
> > }
> >
> > @@ -269,18 +286,86 @@ static void mdev_device_release(struct device *dev)
> > mdev_device_free(mdev);
> > }
> >
> > -int mdev_device_create(struct kobject *kobj,
> > - struct device *dev, const guid_t *uuid)
> > +static const char *
> > +generate_alias(const char *uuid, unsigned int max_alias_len)
> > +{
> > + struct shash_desc *hash_desc;
> > + unsigned int digest_size;
> > + unsigned char *digest;
> > + unsigned int alias_len;
> > + char *alias;
> > + int ret = 0;
> > +
> > + /* Align to multiple of 2 as bin2hex will generate
> > +* even number of bytes.
> > +*/
>
> Comment style for non-networking code please.
>
> > + alias_len = roundup(max_alias_len, 2);
> > + alias = kvzalloc(alias_len + 1, GFP_KERNEL);
Oops, here's the null termination of alias for the even case (+ 1),
ignore the comment below about odd/even. Thanks,
Alex
>
> The size we're generating here should be small enough to just use
> kzalloc(), probably below too.
>
> > + if (!alias)
> > + return NULL;
> > +
> > + /* Allocate and init descriptor */
> > + hash_desc = kvzalloc(sizeof(*hash_desc) +
> > +crypto_shash_descsize(alias_hash),
> > +GFP_KERNEL);
> > + if (!hash_desc)
> > + goto desc_err;
> > +
> > + hash_desc->tfm = alias_hash;
> > +
> > + digest_size = crypto_shash_digestsize(alias_hash);
> > +
> > + digest = kvzalloc(digest_size, GFP_KERNEL);
> > + if (!digest) {
> > + ret = -ENOMEM;
> > + goto digest_err;
> > + }
> > + crypto_shash_init(hash_desc);
> > + crypto_shash_update(hash_desc, uuid, UUID_STRING_LEN);
> > + crypto_shash_final(hash_desc, digest);
> > + bin2hex([0], digest,
>
> [0], ie. alias
>
> > + min_t(unsigned int, digest_size, alias_len / 2));
> > + /* When alias length is odd, zero out and additional last byte
> > +* that bin2hex has copied.
> > +*/
> > + if (max_alias_len % 2)
> > +
Re: [PATCH 1/4] mdev: Introduce sha1 based mdev alias
On Mon, 26 Aug 2019 15:41:16 -0500
Parav Pandit wrote:
> Whenever a parent requests to generate mdev alias, generate a mdev
> alias.
> It is an optional attribute that parent can request to generate
> for each of its child mdev.
> mdev alias is generated using sha1 from the mdev name.
>
> Signed-off-by: Parav Pandit
> ---
> drivers/vfio/mdev/mdev_core.c| 98 +++-
> drivers/vfio/mdev/mdev_private.h | 5 +-
> drivers/vfio/mdev/mdev_sysfs.c | 13 +++--
> include/linux/mdev.h | 4 ++
> 4 files changed, 111 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> index b558d4cfd082..e825ff38b037 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -10,9 +10,11 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> +#include
>
> #include "mdev_private.h"
>
> @@ -27,6 +29,8 @@ static struct class_compat *mdev_bus_compat_class;
> static LIST_HEAD(mdev_list);
> static DEFINE_MUTEX(mdev_list_lock);
>
> +static struct crypto_shash *alias_hash;
> +
> struct device *mdev_parent_dev(struct mdev_device *mdev)
> {
> return mdev->parent->dev;
> @@ -164,6 +168,18 @@ int mdev_register_device(struct device *dev, const
> struct mdev_parent_ops *ops)
> goto add_dev_err;
> }
>
> + if (ops->get_alias_length) {
> + unsigned int digest_size;
> + unsigned int aligned_len;
> +
> + aligned_len = roundup(ops->get_alias_length(), 2);
> + digest_size = crypto_shash_digestsize(alias_hash);
> + if (aligned_len / 2 > digest_size) {
> + ret = -EINVAL;
> + goto add_dev_err;
> + }
> + }
This looks like a sanity check, it could be done outside of the
parent_list_lock, even before we get a parent device reference.
I think we're using a callback for get_alias_length() rather than a
fixed field to support the mtty module option added in patch 4, right?
Its utility is rather limited with no args. I could imagine that if a
parent wanted to generate an alias that could be incorporated into a
string with the parent device name that it would be useful to call this
with the parent device as an arg. I guess we can save that until a
user comes along though.
There doesn't seem to be anything serializing use of alias_hash.
> +
> parent = kzalloc(sizeof(*parent), GFP_KERNEL);
> if (!parent) {
> ret = -ENOMEM;
> @@ -259,6 +275,7 @@ static void mdev_device_free(struct mdev_device *mdev)
> mutex_unlock(_list_lock);
>
> dev_dbg(>dev, "MDEV: destroying\n");
> + kvfree(mdev->alias);
> kfree(mdev);
> }
>
> @@ -269,18 +286,86 @@ static void mdev_device_release(struct device *dev)
> mdev_device_free(mdev);
> }
>
> -int mdev_device_create(struct kobject *kobj,
> -struct device *dev, const guid_t *uuid)
> +static const char *
> +generate_alias(const char *uuid, unsigned int max_alias_len)
> +{
> + struct shash_desc *hash_desc;
> + unsigned int digest_size;
> + unsigned char *digest;
> + unsigned int alias_len;
> + char *alias;
> + int ret = 0;
> +
> + /* Align to multiple of 2 as bin2hex will generate
> + * even number of bytes.
> + */
Comment style for non-networking code please.
> + alias_len = roundup(max_alias_len, 2);
> + alias = kvzalloc(alias_len + 1, GFP_KERNEL);
The size we're generating here should be small enough to just use
kzalloc(), probably below too.
> + if (!alias)
> + return NULL;
> +
> + /* Allocate and init descriptor */
> + hash_desc = kvzalloc(sizeof(*hash_desc) +
> + crypto_shash_descsize(alias_hash),
> + GFP_KERNEL);
> + if (!hash_desc)
> + goto desc_err;
> +
> + hash_desc->tfm = alias_hash;
> +
> + digest_size = crypto_shash_digestsize(alias_hash);
> +
> + digest = kvzalloc(digest_size, GFP_KERNEL);
> + if (!digest) {
> + ret = -ENOMEM;
> + goto digest_err;
> + }
> + crypto_shash_init(hash_desc);
> + crypto_shash_update(hash_desc, uuid, UUID_STRING_LEN);
> + crypto_shash_final(hash_desc, digest);
> + bin2hex([0], digest,
[0], ie. alias
> + min_t(unsigned int, digest_size, alias_len / 2));
> + /* When alias length is odd, zero out and additional last byte
> + * that bin2hex has copied.
> + */
> + if (max_alias_len % 2)
> + alias[max_alias_len] = 0;
Doesn't this give us a null terminated string for odd numbers but not
even numbers? Probably best to define that we always provide a null
terminated string then we could do this unconditionally.
> +
> + kvfree(digest);
> + kvfree(hash_desc);
> + return alias;
> +
> +digest_err:
> + kvfree(hash_desc);
> +desc_err:
> +
