To have consistent naming for the netdevice of a mdev and to have
consistent naming of the devlink port [1] of a mdev, which is formed using
phys_port_name of the devlink port, current UUID is not usable because
UUID is too long.
UUID in string format is 36-characters long and in binary 128-bit.
Both formats are not able to fit within 15 characters limit of netdev
name.
It is desired to have mdev device naming consistent using UUID.
So that widely used user space framework such as ovs [2] can make use
of mdev representor in similar way as PCIe SR-IOV VF and PF representors.
Hence,
(a) mdev alias is created which is derived using sha1 from the mdev name.
(b) Vendor driver describes how long an alias should be for the child mdev
created for a given parent.
(c) Mdev aliases are unique at system level.
(d) alias is created optionally whenever parent requested.
This ensures that non networking mdev parents can function without alias
creation overhead.
This design is discussed at [3].
An example systemd/udev extension will have,
1. netdev name created using mdev alias available in sysfs.
mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001
mdev 12 character alias=cd5b146a80a5
netdev name of this mdev = enmcd5b146a80a5
Here en = Ethernet link
m = mediated device
2. devlink port phys_port_name created using mdev alias.
devlink phys_port_name=pcd5b146a80a5
This patchset enables mdev core to maintain unique alias for a mdev.
Patch-1 Introduces mdev alias using sha1.
Patch-2 Ensures that mdev alias is unique in a system.
Patch-3 Exposes mdev alias in a sysfs hirerchy.
Patch-4 Extends mtty driver to optionally provide alias generation.
This also enables to test UUID based sha1 collision and trigger
error handling for duplicate sha1 results.
In future when networking driver wants to use mdev alias, mdev_alias()
API will be added to derive devlink port name.
[1] http://man7.org/linux/man-pages/man8/devlink-port.8.html
[2] https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html
[3] https://patchwork.kernel.org/cover/11084231/
Parav Pandit (4):
mdev: Introduce sha1 based mdev alias
mdev: Make mdev alias unique among all mdevs
mdev: Expose mdev alias in sysfs tree
mtty: Optionally support mtty alias
drivers/vfio/mdev/mdev_core.c | 103 ++++++++++++++++++++++++++++++-
drivers/vfio/mdev/mdev_private.h | 5 +-
drivers/vfio/mdev/mdev_sysfs.c | 26 ++++++--
include/linux/mdev.h | 4 ++
samples/vfio-mdev/mtty.c | 10 +++
5 files changed, 139 insertions(+), 9 deletions(-)
--
2.19.2
Hi Alex, Cornelia,
> -----Original Message-----
> From: [email protected] <[email protected]> On Behalf
> Of Parav Pandit
> Sent: Tuesday, August 27, 2019 2:11 AM
> To: [email protected]; Jiri Pirko <[email protected]>;
> [email protected]; [email protected]; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; Parav Pandit <[email protected]>
> Subject: [PATCH 0/4] Introduce variable length mdev alias
>
> To have consistent naming for the netdevice of a mdev and to have consistent
> naming of the devlink port [1] of a mdev, which is formed using
> phys_port_name of the devlink port, current UUID is not usable because UUID
> is too long.
>
> UUID in string format is 36-characters long and in binary 128-bit.
> Both formats are not able to fit within 15 characters limit of netdev name.
>
> It is desired to have mdev device naming consistent using UUID.
> So that widely used user space framework such as ovs [2] can make use of
> mdev representor in similar way as PCIe SR-IOV VF and PF representors.
>
> Hence,
> (a) mdev alias is created which is derived using sha1 from the mdev name.
> (b) Vendor driver describes how long an alias should be for the child mdev
> created for a given parent.
> (c) Mdev aliases are unique at system level.
> (d) alias is created optionally whenever parent requested.
> This ensures that non networking mdev parents can function without alias
> creation overhead.
>
> This design is discussed at [3].
>
> An example systemd/udev extension will have,
>
> 1. netdev name created using mdev alias available in sysfs.
>
> mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001
> mdev 12 character alias=cd5b146a80a5
>
> netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link m =
> mediated device
>
> 2. devlink port phys_port_name created using mdev alias.
> devlink phys_port_name=pcd5b146a80a5
>
> This patchset enables mdev core to maintain unique alias for a mdev.
>
> Patch-1 Introduces mdev alias using sha1.
> Patch-2 Ensures that mdev alias is unique in a system.
> Patch-3 Exposes mdev alias in a sysfs hirerchy.
> Patch-4 Extends mtty driver to optionally provide alias generation.
> This also enables to test UUID based sha1 collision and trigger error handling
> for duplicate sha1 results.
>
> In future when networking driver wants to use mdev alias, mdev_alias() API will
> be added to derive devlink port name.
>
Now that majority of above patches looks in shape and I addressed all comments,
In next v1 post, I was considering to include mdev_alias() and have example use in mtty driver.
This way, subsequent series of mlx5_core who intents to use mdev_alias() API makes it easy to review and merge through Dave M, netdev tree.
Is that ok with you?
On Tue, 27 Aug 2019 13:11:17 +0000
Parav Pandit <[email protected]> wrote:
> Hi Alex, Cornelia,
>
> > -----Original Message-----
> > From: [email protected] <[email protected]> On Behalf
> > Of Parav Pandit
> > Sent: Tuesday, August 27, 2019 2:11 AM
> > To: [email protected]; Jiri Pirko <[email protected]>;
> > [email protected]; [email protected]; [email protected]
> > Cc: [email protected]; [email protected];
> > [email protected]; Parav Pandit <[email protected]>
> > Subject: [PATCH 0/4] Introduce variable length mdev alias
> >
> > To have consistent naming for the netdevice of a mdev and to have consistent
> > naming of the devlink port [1] of a mdev, which is formed using
> > phys_port_name of the devlink port, current UUID is not usable because UUID
> > is too long.
> >
> > UUID in string format is 36-characters long and in binary 128-bit.
> > Both formats are not able to fit within 15 characters limit of netdev name.
> >
> > It is desired to have mdev device naming consistent using UUID.
> > So that widely used user space framework such as ovs [2] can make use of
> > mdev representor in similar way as PCIe SR-IOV VF and PF representors.
> >
> > Hence,
> > (a) mdev alias is created which is derived using sha1 from the mdev name.
> > (b) Vendor driver describes how long an alias should be for the child mdev
> > created for a given parent.
> > (c) Mdev aliases are unique at system level.
> > (d) alias is created optionally whenever parent requested.
> > This ensures that non networking mdev parents can function without alias
> > creation overhead.
> >
> > This design is discussed at [3].
> >
> > An example systemd/udev extension will have,
> >
> > 1. netdev name created using mdev alias available in sysfs.
> >
> > mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001
> > mdev 12 character alias=cd5b146a80a5
> >
> > netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link m =
> > mediated device
> >
> > 2. devlink port phys_port_name created using mdev alias.
> > devlink phys_port_name=pcd5b146a80a5
> >
> > This patchset enables mdev core to maintain unique alias for a mdev.
> >
> > Patch-1 Introduces mdev alias using sha1.
> > Patch-2 Ensures that mdev alias is unique in a system.
> > Patch-3 Exposes mdev alias in a sysfs hirerchy.
> > Patch-4 Extends mtty driver to optionally provide alias generation.
> > This also enables to test UUID based sha1 collision and trigger error handling
> > for duplicate sha1 results.
> >
> > In future when networking driver wants to use mdev alias, mdev_alias() API will
> > be added to derive devlink port name.
> >
> Now that majority of above patches looks in shape and I addressed all comments,
I think the discussion of what to do with the attribute if no alias is
available is still unresolved; waiting for maintainer opinion.
> In next v1 post, I was considering to include mdev_alias() and have example use in mtty driver.
>
> This way, subsequent series of mlx5_core who intents to use mdev_alias() API makes it easy to review and merge through Dave M, netdev tree.
> Is that ok with you?
On Tue, 27 Aug 2019 13:11:17 +0000
Parav Pandit <[email protected]> wrote:
> Hi Alex, Cornelia,
>
> > -----Original Message-----
> > From: [email protected] <[email protected]> On Behalf
> > Of Parav Pandit
> > Sent: Tuesday, August 27, 2019 2:11 AM
> > To: [email protected]; Jiri Pirko <[email protected]>;
> > [email protected]; [email protected]; [email protected]
> > Cc: [email protected]; [email protected];
> > [email protected]; Parav Pandit <[email protected]>
> > Subject: [PATCH 0/4] Introduce variable length mdev alias
> >
> > To have consistent naming for the netdevice of a mdev and to have consistent
> > naming of the devlink port [1] of a mdev, which is formed using
> > phys_port_name of the devlink port, current UUID is not usable because UUID
> > is too long.
> >
> > UUID in string format is 36-characters long and in binary 128-bit.
> > Both formats are not able to fit within 15 characters limit of netdev name.
> >
> > It is desired to have mdev device naming consistent using UUID.
> > So that widely used user space framework such as ovs [2] can make use of
> > mdev representor in similar way as PCIe SR-IOV VF and PF representors.
> >
> > Hence,
> > (a) mdev alias is created which is derived using sha1 from the mdev name.
> > (b) Vendor driver describes how long an alias should be for the child mdev
> > created for a given parent.
> > (c) Mdev aliases are unique at system level.
> > (d) alias is created optionally whenever parent requested.
> > This ensures that non networking mdev parents can function without alias
> > creation overhead.
> >
> > This design is discussed at [3].
> >
> > An example systemd/udev extension will have,
> >
> > 1. netdev name created using mdev alias available in sysfs.
> >
> > mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001
> > mdev 12 character alias=cd5b146a80a5
> >
> > netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link m =
> > mediated device
> >
> > 2. devlink port phys_port_name created using mdev alias.
> > devlink phys_port_name=pcd5b146a80a5
> >
> > This patchset enables mdev core to maintain unique alias for a mdev.
> >
> > Patch-1 Introduces mdev alias using sha1.
> > Patch-2 Ensures that mdev alias is unique in a system.
> > Patch-3 Exposes mdev alias in a sysfs hirerchy.
> > Patch-4 Extends mtty driver to optionally provide alias generation.
> > This also enables to test UUID based sha1 collision and trigger error handling
> > for duplicate sha1 results.
> >
> > In future when networking driver wants to use mdev alias, mdev_alias() API will
> > be added to derive devlink port name.
> >
> Now that majority of above patches looks in shape and I addressed all comments,
> In next v1 post, I was considering to include mdev_alias() and have
> example use in mtty driver.
>
> This way, subsequent series of mlx5_core who intents to use
> mdev_alias() API makes it easy to review and merge through Dave M,
> netdev tree. Is that ok with you?
What would be the timing for the mlx5_core use case? Can we coordinate
within the same development cycle? I wouldn't want someone to come
clean up the sample driver and remove the API ;) Thanks,
Alex
> -----Original Message-----
> From: Alex Williamson <[email protected]>
> Sent: Tuesday, August 27, 2019 11:19 PM
> To: Parav Pandit <[email protected]>
> Cc: Jiri Pirko <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH 0/4] Introduce variable length mdev alias
>
> On Tue, 27 Aug 2019 13:11:17 +0000
> Parav Pandit <[email protected]> wrote:
>
> > Hi Alex, Cornelia,
> >
> > > -----Original Message-----
> > > From: [email protected] <[email protected]> On
> > > Behalf Of Parav Pandit
> > > Sent: Tuesday, August 27, 2019 2:11 AM
> > > To: [email protected]; Jiri Pirko <[email protected]>;
> > > [email protected]; [email protected]; [email protected]
> > > Cc: [email protected]; [email protected];
> > > [email protected]; Parav Pandit <[email protected]>
> > > Subject: [PATCH 0/4] Introduce variable length mdev alias
> > >
> > > To have consistent naming for the netdevice of a mdev and to have
> > > consistent naming of the devlink port [1] of a mdev, which is formed
> > > using phys_port_name of the devlink port, current UUID is not usable
> > > because UUID is too long.
> > >
> > > UUID in string format is 36-characters long and in binary 128-bit.
> > > Both formats are not able to fit within 15 characters limit of netdev
> name.
> > >
> > > It is desired to have mdev device naming consistent using UUID.
> > > So that widely used user space framework such as ovs [2] can make
> > > use of mdev representor in similar way as PCIe SR-IOV VF and PF
> representors.
> > >
> > > Hence,
> > > (a) mdev alias is created which is derived using sha1 from the mdev
> name.
> > > (b) Vendor driver describes how long an alias should be for the
> > > child mdev created for a given parent.
> > > (c) Mdev aliases are unique at system level.
> > > (d) alias is created optionally whenever parent requested.
> > > This ensures that non networking mdev parents can function without
> > > alias creation overhead.
> > >
> > > This design is discussed at [3].
> > >
> > > An example systemd/udev extension will have,
> > >
> > > 1. netdev name created using mdev alias available in sysfs.
> > >
> > > mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001
> > > mdev 12 character alias=cd5b146a80a5
> > >
> > > netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link m
> > > = mediated device
> > >
> > > 2. devlink port phys_port_name created using mdev alias.
> > > devlink phys_port_name=pcd5b146a80a5
> > >
> > > This patchset enables mdev core to maintain unique alias for a mdev.
> > >
> > > Patch-1 Introduces mdev alias using sha1.
> > > Patch-2 Ensures that mdev alias is unique in a system.
> > > Patch-3 Exposes mdev alias in a sysfs hirerchy.
> > > Patch-4 Extends mtty driver to optionally provide alias generation.
> > > This also enables to test UUID based sha1 collision and trigger
> > > error handling for duplicate sha1 results.
> > >
> > > In future when networking driver wants to use mdev alias,
> > > mdev_alias() API will be added to derive devlink port name.
> > >
> > Now that majority of above patches looks in shape and I addressed all
> > comments, In next v1 post, I was considering to include mdev_alias()
> > and have example use in mtty driver.
> >
> > This way, subsequent series of mlx5_core who intents to use
> > mdev_alias() API makes it easy to review and merge through Dave M,
> > netdev tree. Is that ok with you?
>
> What would be the timing for the mlx5_core use case? Can we coordinate
> within the same development cycle? I wouldn't want someone to come
> clean up the sample driver and remove the API ;) Thanks,
>
We targeted it for 5.4. mdev_alias was the only known user interface issue, which is resolved.
Some more internal reviews are in progress.
It might be tight for 5.4, if not 5.4, it should happen in 5.5.
I agree, that is why I was holding up to be part of this series.
Since its very small API, even if there is any merge conflict, it is easy to resolve.
If this change can be merged through netdev tree, its better to include it as part of mlx5_core's mdev series.
So both options are fine, a direction from you is better to have.
To have consistent naming for the netdevice of a mdev and to have
consistent naming of the devlink port [1] of a mdev, which is formed using
phys_port_name of the devlink port, current UUID is not usable because
UUID is too long.
UUID in string format is 36-characters long and in binary 128-bit.
Both formats are not able to fit within 15 characters limit of netdev
name.
It is desired to have mdev device naming consistent using UUID.
So that widely used user space framework such as ovs [2] can make use
of mdev representor in similar way as PCIe SR-IOV VF and PF representors.
Hence,
(a) mdev alias is created which is derived using sha1 from the mdev name.
(b) Vendor driver describes how long an alias should be for the child mdev
created for a given parent.
(c) Mdev aliases are unique at system level.
(d) alias is created optionally whenever parent requested.
This ensures that non networking mdev parents can function without alias
creation overhead.
This design is discussed at [3].
An example systemd/udev extension will have,
1. netdev name created using mdev alias available in sysfs.
mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001
mdev 12 character alias=cd5b146a80a5
netdev name of this mdev = enmcd5b146a80a5
Here en = Ethernet link
m = mediated device
2. devlink port phys_port_name created using mdev alias.
devlink phys_port_name=pcd5b146a80a5
This patchset enables mdev core to maintain unique alias for a mdev.
Patch-1 Introduces mdev alias using sha1.
Patch-2 Ensures that mdev alias is unique in a system.
Patch-3 Exposes mdev alias in a sysfs hirerchy.
Patch-4 Extends mtty driver to optionally provide alias generation.
This also enables to test UUID based sha1 collision and trigger
error handling for duplicate sha1 results.
In future when networking driver wants to use mdev alias, mdev_alias()
API will be added to derive devlink port name.
[1] http://man7.org/linux/man-pages/man8/devlink-port.8.html
[2] https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html
[3] https://patchwork.kernel.org/cover/11084231/
---
Changelog:
v0->v1:
- Addressed comments from Alex Williamson, Cornelia Hunk and Mark Bloch
- Moved alias length check outside of the parent lock
- Moved alias and digest allocation from kvzalloc to kzalloc
- &alias[0] changed to alias
- alias_length check is nested under get_alias_length callback check
- Changed comments to start with an empty line
- Added comment where alias memory ownership is handed over to mdev device
- Fixed cleaunup of hash if mdev_bus_register() fails
- Updated documentation for new sysfs alias file
- Improved commit logs to make description more clear
- Fixed inclusiong of alias for NULL check
- Added ratelimited debug print for sha1 hash collision error
Parav Pandit (5):
mdev: Introduce sha1 based mdev alias
mdev: Make mdev alias unique among all mdevs
mdev: Expose mdev alias in sysfs tree
mdev: Update sysfs documentation
mtty: Optionally support mtty alias
.../driver-api/vfio-mediated-device.rst | 5 +
drivers/vfio/mdev/mdev_core.c | 117 +++++++++++++++++-
drivers/vfio/mdev/mdev_private.h | 5 +-
drivers/vfio/mdev/mdev_sysfs.c | 26 +++-
include/linux/mdev.h | 4 +
samples/vfio-mdev/mtty.c | 10 ++
6 files changed, 157 insertions(+), 10 deletions(-)
--
2.19.2
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 for an mdev device. If generated,
that alias is checked for collisions.
It is an optional attribute.
mdev alias is generated using sha1 from the mdev name.
Signed-off-by: Parav Pandit <[email protected]>
---
Changelog:
v0->v1:
- Moved alias length check outside of the parent lock
- Moved alias and digest allocation from kvzalloc to kzalloc
- &alias[0] changed to alias
- alias_length check is nested under get_alias_length callback check
- Changed comments to start with an empty line
- Fixed cleaunup of hash if mdev_bus_register() fails
- Added comment where alias memory ownership is handed over to mdev device
- Updated commit log to indicate motivation for this feature
---
drivers/vfio/mdev/mdev_core.c | 110 ++++++++++++++++++++++++++++++-
drivers/vfio/mdev/mdev_private.h | 5 +-
drivers/vfio/mdev/mdev_sysfs.c | 13 ++--
include/linux/mdev.h | 4 ++
4 files changed, 122 insertions(+), 10 deletions(-)
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index b558d4cfd082..62d29f57fe0c 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -10,9 +10,11 @@
#include <linux/module.h>
#include <linux/device.h>
#include <linux/slab.h>
+#include <linux/mm.h>
#include <linux/uuid.h>
#include <linux/sysfs.h>
#include <linux/mdev.h>
+#include <crypto/hash.h>
#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;
@@ -150,6 +154,16 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups)
return -EINVAL;
+ 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)
+ return -EINVAL;
+ }
+
dev = get_device(dev);
if (!dev)
return -EINVAL;
@@ -259,6 +273,7 @@ static void mdev_device_free(struct mdev_device *mdev)
mutex_unlock(&mdev_list_lock);
dev_dbg(&mdev->dev, "MDEV: destroying\n");
+ kfree(mdev->alias);
kfree(mdev);
}
@@ -269,18 +284,88 @@ 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.
+ */
+ alias_len = roundup(max_alias_len, 2);
+ alias = kzalloc(alias_len + 1, GFP_KERNEL);
+ 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 = kzalloc(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(alias, digest, 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;
+
+ kfree(digest);
+ kvfree(hash_desc);
+ return alias;
+
+digest_err:
+ kvfree(hash_desc);
+desc_err:
+ kfree(alias);
+ return NULL;
+}
+
+int mdev_device_create(struct kobject *kobj, struct device *dev,
+ const char *uuid_str, const guid_t *uuid)
{
int ret;
struct mdev_device *mdev, *tmp;
struct mdev_parent *parent;
struct mdev_type *type = to_mdev_type(kobj);
+ const char *alias = NULL;
parent = mdev_get_parent(type->parent);
if (!parent)
return -EINVAL;
+ if (parent->ops->get_alias_length) {
+ unsigned int alias_len;
+
+ alias_len = parent->ops->get_alias_length();
+ if (alias_len) {
+ alias = generate_alias(uuid_str, alias_len);
+ if (!alias) {
+ ret = -ENOMEM;
+ goto alias_fail;
+ }
+ }
+ }
mutex_lock(&mdev_list_lock);
/* Check for duplicate */
@@ -300,6 +385,12 @@ int mdev_device_create(struct kobject *kobj,
}
guid_copy(&mdev->uuid, uuid);
+ mdev->alias = alias;
+ /*
+ * At this point alias memory is owned by the mdev.
+ * Mark it NULL, so that only mdev can free it.
+ */
+ alias = NULL;
list_add(&mdev->next, &mdev_list);
mutex_unlock(&mdev_list_lock);
@@ -346,6 +437,8 @@ int mdev_device_create(struct kobject *kobj,
up_read(&parent->unreg_sem);
put_device(&mdev->dev);
mdev_fail:
+ kfree(alias);
+alias_fail:
mdev_put_parent(parent);
return ret;
}
@@ -406,7 +499,17 @@ EXPORT_SYMBOL(mdev_get_iommu_device);
static int __init mdev_init(void)
{
- return mdev_bus_register();
+ int ret;
+
+ alias_hash = crypto_alloc_shash("sha1", 0, 0);
+ if (!alias_hash)
+ return -ENOMEM;
+
+ ret = mdev_bus_register();
+ if (ret)
+ crypto_free_shash(alias_hash);
+
+ return ret;
}
static void __exit mdev_exit(void)
@@ -415,6 +518,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/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
index 7d922950caaf..cf1c0d9842c6 100644
--- a/drivers/vfio/mdev/mdev_private.h
+++ b/drivers/vfio/mdev/mdev_private.h
@@ -33,6 +33,7 @@ struct mdev_device {
struct kobject *type_kobj;
struct device *iommu_device;
bool active;
+ const char *alias;
};
#define to_mdev_device(dev) container_of(dev, struct mdev_device, dev)
@@ -57,8 +58,8 @@ void parent_remove_sysfs_files(struct mdev_parent *parent);
int mdev_create_sysfs_files(struct device *dev, struct mdev_type *type);
void mdev_remove_sysfs_files(struct device *dev, struct mdev_type *type);
-int mdev_device_create(struct kobject *kobj,
- struct device *dev, const guid_t *uuid);
+int mdev_device_create(struct kobject *kobj, struct device *dev,
+ const char *uuid_str, const guid_t *uuid);
int mdev_device_remove(struct device *dev);
#endif /* MDEV_PRIVATE_H */
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
index 7570c7602ab4..43afe0e80b76 100644
--- a/drivers/vfio/mdev/mdev_sysfs.c
+++ b/drivers/vfio/mdev/mdev_sysfs.c
@@ -63,15 +63,18 @@ static ssize_t create_store(struct kobject *kobj, struct device *dev,
return -ENOMEM;
ret = guid_parse(str, &uuid);
- kfree(str);
if (ret)
- return ret;
+ goto err;
- ret = mdev_device_create(kobj, dev, &uuid);
+ ret = mdev_device_create(kobj, dev, str, &uuid);
if (ret)
- return ret;
+ goto err;
- return count;
+ ret = count;
+
+err:
+ kfree(str);
+ return ret;
}
MDEV_TYPE_ATTR_WO(create);
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.
* 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 */
--
2.19.2
Updated documentation for optional read only sysfs attribute.
Signed-off-by: Parav Pandit <[email protected]>
---
Documentation/driver-api/vfio-mediated-device.rst | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
index 25eb7d5b834b..0ab03d3f5629 100644
--- a/Documentation/driver-api/vfio-mediated-device.rst
+++ b/Documentation/driver-api/vfio-mediated-device.rst
@@ -270,6 +270,7 @@ Directories and Files Under the sysfs for Each mdev Device
|--- remove
|--- mdev_type {link to its type}
|--- vendor-specific-attributes [optional]
+ |--- alias [optional]
* remove (write only)
@@ -281,6 +282,10 @@ Example::
# echo 1 > /sys/bus/mdev/devices/$mdev_UUID/remove
+* alias (read only)
+Whenever a parent requested to generate an alias, each mdev is assigned a unique
+alias by the mdev core. This file shows the alias of the mdev device.
+
Mediated device Hot plug
------------------------
--
2.19.2
Expose the optional alias for an mdev device as a sysfs attribute.
This way, userspace tools such as udev may make use of the alias, for
example to create a netdevice name for the mdev.
Signed-off-by: Parav Pandit <[email protected]>
---
Changelog:
v0->v1:
- Addressed comments from Cornelia Huck
- Updated commit description
---
drivers/vfio/mdev/mdev_sysfs.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
index 43afe0e80b76..59f4e3cc5233 100644
--- a/drivers/vfio/mdev/mdev_sysfs.c
+++ b/drivers/vfio/mdev/mdev_sysfs.c
@@ -246,7 +246,20 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr,
static DEVICE_ATTR_WO(remove);
+static ssize_t alias_show(struct device *device,
+ struct device_attribute *attr, char *buf)
+{
+ struct mdev_device *dev = mdev_from_dev(device);
+
+ if (!dev->alias)
+ return -EOPNOTSUPP;
+
+ return sprintf(buf, "%s\n", dev->alias);
+}
+static DEVICE_ATTR_RO(alias);
+
static const struct attribute *mdev_device_attrs[] = {
+ &dev_attr_alias.attr,
&dev_attr_remove.attr,
NULL,
};
--
2.19.2
Mdev alias should be unique among all the mdevs, so that when such alias
is used by the mdev users to derive other objects, there is no
collision in a given system.
Signed-off-by: Parav Pandit <[email protected]>
---
Changelog:
v0->v1:
- Fixed inclusiong of alias for NULL check
- Added ratelimited debug print for sha1 hash collision error
---
drivers/vfio/mdev/mdev_core.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index 62d29f57fe0c..4b9899e40665 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -375,6 +375,13 @@ int mdev_device_create(struct kobject *kobj, struct device *dev,
ret = -EEXIST;
goto mdev_fail;
}
+ if (tmp->alias && alias && strcmp(tmp->alias, alias) == 0) {
+ mutex_unlock(&mdev_list_lock);
+ ret = -EEXIST;
+ dev_dbg_ratelimited(dev, "Hash collision in alias creation for UUID %pUl\n",
+ uuid);
+ goto mdev_fail;
+ }
}
mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
--
2.19.2
Provide a module parameter to set alias length to optionally generate
mdev alias.
Example to request mdev alias.
$ modprobe mtty alias_length=12
Signed-off-by: Parav Pandit <[email protected]>
---
samples/vfio-mdev/mtty.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
index 92e770a06ea2..92208245b057 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -1410,6 +1410,15 @@ static struct attribute_group *mdev_type_groups[] = {
NULL,
};
+static unsigned int mtty_alias_length;
+module_param_named(alias_length, mtty_alias_length, uint, 0444);
+MODULE_PARM_DESC(alias_length, "mdev alias length; default=0");
+
+static unsigned int mtty_get_alias_length(void)
+{
+ return mtty_alias_length;
+}
+
static const struct mdev_parent_ops mdev_fops = {
.owner = THIS_MODULE,
.dev_attr_groups = mtty_dev_groups,
@@ -1422,6 +1431,7 @@ static const struct mdev_parent_ops mdev_fops = {
.read = mtty_read,
.write = mtty_write,
.ioctl = mtty_ioctl,
+ .get_alias_length = mtty_get_alias_length
};
static void mtty_device_release(struct device *dev)
--
2.19.2
On Tue, 27 Aug 2019 14:16:50 -0500
Parav Pandit <[email protected]> wrote:
> 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 for an mdev device. If generated,
> that alias is checked for collisions.
>
> It is an optional attribute.
> mdev alias is generated using sha1 from the mdev name.
>
> Signed-off-by: Parav Pandit <[email protected]>
>
> ---
> Changelog:
>
> v0->v1:
> - Moved alias length check outside of the parent lock
> - Moved alias and digest allocation from kvzalloc to kzalloc
> - &alias[0] changed to alias
> - alias_length check is nested under get_alias_length callback check
> - Changed comments to start with an empty line
> - Fixed cleaunup of hash if mdev_bus_register() fails
> - Added comment where alias memory ownership is handed over to mdev device
> - Updated commit log to indicate motivation for this feature
> ---
> drivers/vfio/mdev/mdev_core.c | 110 ++++++++++++++++++++++++++++++-
> drivers/vfio/mdev/mdev_private.h | 5 +-
> drivers/vfio/mdev/mdev_sysfs.c | 13 ++--
> include/linux/mdev.h | 4 ++
> 4 files changed, 122 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> index b558d4cfd082..62d29f57fe0c 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -10,9 +10,11 @@
> #include <linux/module.h>
> #include <linux/device.h>
> #include <linux/slab.h>
> +#include <linux/mm.h>
> #include <linux/uuid.h>
> #include <linux/sysfs.h>
> #include <linux/mdev.h>
> +#include <crypto/hash.h>
>
> #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;
> @@ -150,6 +154,16 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
> if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups)
> return -EINVAL;
>
> + 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)
> + return -EINVAL;
> + }
> +
> dev = get_device(dev);
> if (!dev)
> return -EINVAL;
> @@ -259,6 +273,7 @@ static void mdev_device_free(struct mdev_device *mdev)
> mutex_unlock(&mdev_list_lock);
>
> dev_dbg(&mdev->dev, "MDEV: destroying\n");
> + kfree(mdev->alias);
> kfree(mdev);
> }
>
> @@ -269,18 +284,88 @@ 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.
> + */
> + alias_len = roundup(max_alias_len, 2);
> + alias = kzalloc(alias_len + 1, GFP_KERNEL);
> + 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 = kzalloc(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);
All of these can fail and many, if not most, of the callers appear
that they might test the return value. Thanks,
Alex
> + bin2hex(alias, digest, 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;
> +
> + kfree(digest);
> + kvfree(hash_desc);
> + return alias;
> +
> +digest_err:
> + kvfree(hash_desc);
> +desc_err:
> + kfree(alias);
> + return NULL;
> +}
> +
> +int mdev_device_create(struct kobject *kobj, struct device *dev,
> + const char *uuid_str, const guid_t *uuid)
> {
> int ret;
> struct mdev_device *mdev, *tmp;
> struct mdev_parent *parent;
> struct mdev_type *type = to_mdev_type(kobj);
> + const char *alias = NULL;
>
> parent = mdev_get_parent(type->parent);
> if (!parent)
> return -EINVAL;
>
> + if (parent->ops->get_alias_length) {
> + unsigned int alias_len;
> +
> + alias_len = parent->ops->get_alias_length();
> + if (alias_len) {
> + alias = generate_alias(uuid_str, alias_len);
> + if (!alias) {
> + ret = -ENOMEM;
> + goto alias_fail;
> + }
> + }
> + }
> mutex_lock(&mdev_list_lock);
>
> /* Check for duplicate */
> @@ -300,6 +385,12 @@ int mdev_device_create(struct kobject *kobj,
> }
>
> guid_copy(&mdev->uuid, uuid);
> + mdev->alias = alias;
> + /*
> + * At this point alias memory is owned by the mdev.
> + * Mark it NULL, so that only mdev can free it.
> + */
> + alias = NULL;
> list_add(&mdev->next, &mdev_list);
> mutex_unlock(&mdev_list_lock);
>
> @@ -346,6 +437,8 @@ int mdev_device_create(struct kobject *kobj,
> up_read(&parent->unreg_sem);
> put_device(&mdev->dev);
> mdev_fail:
> + kfree(alias);
> +alias_fail:
> mdev_put_parent(parent);
> return ret;
> }
> @@ -406,7 +499,17 @@ EXPORT_SYMBOL(mdev_get_iommu_device);
>
> static int __init mdev_init(void)
> {
> - return mdev_bus_register();
> + int ret;
> +
> + alias_hash = crypto_alloc_shash("sha1", 0, 0);
> + if (!alias_hash)
> + return -ENOMEM;
> +
> + ret = mdev_bus_register();
> + if (ret)
> + crypto_free_shash(alias_hash);
> +
> + return ret;
> }
>
> static void __exit mdev_exit(void)
> @@ -415,6 +518,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/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
> index 7d922950caaf..cf1c0d9842c6 100644
> --- a/drivers/vfio/mdev/mdev_private.h
> +++ b/drivers/vfio/mdev/mdev_private.h
> @@ -33,6 +33,7 @@ struct mdev_device {
> struct kobject *type_kobj;
> struct device *iommu_device;
> bool active;
> + const char *alias;
> };
>
> #define to_mdev_device(dev) container_of(dev, struct mdev_device, dev)
> @@ -57,8 +58,8 @@ void parent_remove_sysfs_files(struct mdev_parent *parent);
> int mdev_create_sysfs_files(struct device *dev, struct mdev_type *type);
> void mdev_remove_sysfs_files(struct device *dev, struct mdev_type *type);
>
> -int mdev_device_create(struct kobject *kobj,
> - struct device *dev, const guid_t *uuid);
> +int mdev_device_create(struct kobject *kobj, struct device *dev,
> + const char *uuid_str, const guid_t *uuid);
> int mdev_device_remove(struct device *dev);
>
> #endif /* MDEV_PRIVATE_H */
> diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
> index 7570c7602ab4..43afe0e80b76 100644
> --- a/drivers/vfio/mdev/mdev_sysfs.c
> +++ b/drivers/vfio/mdev/mdev_sysfs.c
> @@ -63,15 +63,18 @@ static ssize_t create_store(struct kobject *kobj, struct device *dev,
> return -ENOMEM;
>
> ret = guid_parse(str, &uuid);
> - kfree(str);
> if (ret)
> - return ret;
> + goto err;
>
> - ret = mdev_device_create(kobj, dev, &uuid);
> + ret = mdev_device_create(kobj, dev, str, &uuid);
> if (ret)
> - return ret;
> + goto err;
>
> - return count;
> + ret = count;
> +
> +err:
> + kfree(str);
> + return ret;
> }
>
> MDEV_TYPE_ATTR_WO(create);
> 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.
> * 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 */
On Wed, 28 Aug 2019 15:25:44 -0600
Alex Williamson <[email protected]> wrote:
> On Tue, 27 Aug 2019 14:16:50 -0500
> Parav Pandit <[email protected]> wrote:
> > module_init(mdev_init)
> > diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
> > index 7d922950caaf..cf1c0d9842c6 100644
> > --- a/drivers/vfio/mdev/mdev_private.h
> > +++ b/drivers/vfio/mdev/mdev_private.h
> > @@ -33,6 +33,7 @@ struct mdev_device {
> > struct kobject *type_kobj;
> > struct device *iommu_device;
> > bool active;
> > + const char *alias;
Nit, put this above active to avoid creating a hole in the structure.
Thanks,
Alex
On Tue, 27 Aug 2019 14:16:51 -0500
Parav Pandit <[email protected]> wrote:
> Mdev alias should be unique among all the mdevs, so that when such alias
> is used by the mdev users to derive other objects, there is no
> collision in a given system.
>
> Signed-off-by: Parav Pandit <[email protected]>
>
> ---
> Changelog:
> v0->v1:
> - Fixed inclusiong of alias for NULL check
> - Added ratelimited debug print for sha1 hash collision error
> ---
> drivers/vfio/mdev/mdev_core.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> index 62d29f57fe0c..4b9899e40665 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -375,6 +375,13 @@ int mdev_device_create(struct kobject *kobj, struct device *dev,
> ret = -EEXIST;
> goto mdev_fail;
> }
> + if (tmp->alias && alias && strcmp(tmp->alias, alias) == 0) {
Nit, test if the device we adding has an alias before the device we're
testing against. The compiler can better optimize keeping alias hot.
Thanks,
Alex
> + mutex_unlock(&mdev_list_lock);
> + ret = -EEXIST;
> + dev_dbg_ratelimited(dev, "Hash collision in alias creation for UUID %pUl\n",
> + uuid);
> + goto mdev_fail;
> + }
> }
>
> mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
> -----Original Message-----
> From: Alex Williamson <[email protected]>
> Sent: Thursday, August 29, 2019 2:56 AM
> To: Parav Pandit <[email protected]>
> Cc: Jiri Pirko <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH v1 1/5] mdev: Introduce sha1 based mdev alias
>
> > + /* 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 = kzalloc(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);
>
> All of these can fail and many, if not most, of the callers appear that they might
> test the return value. Thanks,
Right. Changing the signature and honoring return value in v2.
>
> Alex
> -----Original Message-----
> From: Alex Williamson <[email protected]>
> Sent: Thursday, August 29, 2019 3:05 AM
> To: Parav Pandit <[email protected]>
> Cc: Jiri Pirko <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH v1 1/5] mdev: Introduce sha1 based mdev alias
>
> On Wed, 28 Aug 2019 15:25:44 -0600
> Alex Williamson <[email protected]> wrote:
>
> > On Tue, 27 Aug 2019 14:16:50 -0500
> > Parav Pandit <[email protected]> wrote:
> > > module_init(mdev_init)
> > > diff --git a/drivers/vfio/mdev/mdev_private.h
> b/drivers/vfio/mdev/mdev_private.h
> > > index 7d922950caaf..cf1c0d9842c6 100644
> > > --- a/drivers/vfio/mdev/mdev_private.h
> > > +++ b/drivers/vfio/mdev/mdev_private.h
> > > @@ -33,6 +33,7 @@ struct mdev_device {
> > > struct kobject *type_kobj;
> > > struct device *iommu_device;
> > > bool active;
> > > + const char *alias;
>
> Nit, put this above active to avoid creating a hole in the structure.
> Thanks,
>
Ack.
> Alex
> -----Original Message-----
> From: Alex Williamson <[email protected]>
> Sent: Thursday, August 29, 2019 3:07 AM
> To: Parav Pandit <[email protected]>
> Cc: Jiri Pirko <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH v1 2/5] mdev: Make mdev alias unique among all mdevs
>
> On Tue, 27 Aug 2019 14:16:51 -0500
> Parav Pandit <[email protected]> wrote:
>
> > Mdev alias should be unique among all the mdevs, so that when such
> > alias is used by the mdev users to derive other objects, there is no
> > collision in a given system.
> >
> > Signed-off-by: Parav Pandit <[email protected]>
> >
> > ---
> > Changelog:
> > v0->v1:
> > - Fixed inclusiong of alias for NULL check
> > - Added ratelimited debug print for sha1 hash collision error
> > ---
> > drivers/vfio/mdev/mdev_core.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/vfio/mdev/mdev_core.c
> > b/drivers/vfio/mdev/mdev_core.c index 62d29f57fe0c..4b9899e40665
> > 100644
> > --- a/drivers/vfio/mdev/mdev_core.c
> > +++ b/drivers/vfio/mdev/mdev_core.c
> > @@ -375,6 +375,13 @@ int mdev_device_create(struct kobject *kobj, struct
> device *dev,
> > ret = -EEXIST;
> > goto mdev_fail;
> > }
> > + if (tmp->alias && alias && strcmp(tmp->alias, alias) == 0) {
>
> Nit, test if the device we adding has an alias before the device we're testing
> against. The compiler can better optimize keeping alias hot.
> Thanks,
>
Ok. will do.
> Alex
To have consistent naming for the netdevice of a mdev and to have
consistent naming of the devlink port [1] of a mdev, which is formed using
phys_port_name of the devlink port, current UUID is not usable because
UUID is too long.
UUID in string format is 36-characters long and in binary 128-bit.
Both formats are not able to fit within 15 characters limit of netdev
name.
It is desired to have mdev device naming consistent using UUID.
So that widely used user space framework such as ovs [2] can make use
of mdev representor in similar way as PCIe SR-IOV VF and PF representors.
Hence,
(a) mdev alias is created which is derived using sha1 from the mdev name.
(b) Vendor driver describes how long an alias should be for the child mdev
created for a given parent.
(c) Mdev aliases are unique at system level.
(d) alias is created optionally whenever parent requested.
This ensures that non networking mdev parents can function without alias
creation overhead.
This design is discussed at [3].
An example systemd/udev extension will have,
1. netdev name created using mdev alias available in sysfs.
mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001
mdev 12 character alias=cd5b146a80a5
netdev name of this mdev = enmcd5b146a80a5
Here en = Ethernet link
m = mediated device
2. devlink port phys_port_name created using mdev alias.
devlink phys_port_name=pcd5b146a80a5
This patchset enables mdev core to maintain unique alias for a mdev.
Patch-1 Introduces mdev alias using sha1.
Patch-2 Ensures that mdev alias is unique in a system.
Patch-3 Exposes mdev alias in a sysfs hirerchy.
Patch-4 Introduces mdev_alias() API.
Patch-5 Updated sysfs documentation
Patch-6 Extends mtty driver to optionally provide alias generation.
This also enables to test UUID based sha1 collision and trigger
error handling for duplicate sha1 results.
[1] http://man7.org/linux/man-pages/man8/devlink-port.8.html
[2] https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html
[3] https://patchwork.kernel.org/cover/11084231/
---
Changelog:
v1->v2:
- Corrected a typo from 'and' to 'an'
- Addressed comments from Alex Williamson
- Kept mdev_device naturally aligned
- Added error checking for crypt_*() calls
- Moved alias NULL check at beginning
- Added mdev_alias() API
- Updated mtty driver to show example mdev_alias() usage
- Changed return type of generate_alias() from int to char*
v0->v1:
- Addressed comments from Alex Williamson, Cornelia Hunk and Mark Bloch
- Moved alias length check outside of the parent lock
- Moved alias and digest allocation from kvzalloc to kzalloc
- &alias[0] changed to alias
- alias_length check is nested under get_alias_length callback check
- Changed comments to start with an empty line
- Added comment where alias memory ownership is handed over to mdev device
- Fixed cleaunup of hash if mdev_bus_register() fails
- Updated documentation for new sysfs alias file
- Improved commit logs to make description more clear
- Fixed inclusiong of alias for NULL check
- Added ratelimited debug print for sha1 hash collision error
Parav Pandit (6):
mdev: Introduce sha1 based mdev alias
mdev: Make mdev alias unique among all mdevs
mdev: Expose mdev alias in sysfs tree
mdev: Introduce an API mdev_alias
mdev: Update sysfs documentation
mtty: Optionally support mtty alias
.../driver-api/vfio-mediated-device.rst | 5 +
drivers/vfio/mdev/mdev_core.c | 142 +++++++++++++++++-
drivers/vfio/mdev/mdev_private.h | 5 +-
drivers/vfio/mdev/mdev_sysfs.c | 26 +++-
include/linux/mdev.h | 5 +
samples/vfio-mdev/mtty.c | 13 ++
6 files changed, 186 insertions(+), 10 deletions(-)
--
2.19.2
Provide a module parameter to set alias length to optionally generate
mdev alias.
Example to request mdev alias.
$ modprobe mtty alias_length=12
Make use of mtty_alias() API when alias_length module parameter is set.
Signed-off-by: Parav Pandit <[email protected]>
---
Changelog:
v1->v2:
- Added mdev_alias() usage sample
---
samples/vfio-mdev/mtty.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
index 92e770a06ea2..075d65440bc0 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -150,6 +150,10 @@ static const struct file_operations vd_fops = {
.owner = THIS_MODULE,
};
+static unsigned int mtty_alias_length;
+module_param_named(alias_length, mtty_alias_length, uint, 0444);
+MODULE_PARM_DESC(alias_length, "mdev alias length; default=0");
+
/* function prototypes */
static int mtty_trigger_interrupt(const guid_t *uuid);
@@ -770,6 +774,9 @@ static int mtty_create(struct kobject *kobj, struct mdev_device *mdev)
list_add(&mdev_state->next, &mdev_devices_list);
mutex_unlock(&mdev_list_lock);
+ if (mtty_alias_length)
+ dev_dbg(mdev_dev(mdev), "alias is %s\n", mdev_alias(mdev));
+
return 0;
}
@@ -1410,6 +1417,11 @@ static struct attribute_group *mdev_type_groups[] = {
NULL,
};
+static unsigned int mtty_get_alias_length(void)
+{
+ return mtty_alias_length;
+}
+
static const struct mdev_parent_ops mdev_fops = {
.owner = THIS_MODULE,
.dev_attr_groups = mtty_dev_groups,
@@ -1422,6 +1434,7 @@ static const struct mdev_parent_ops mdev_fops = {
.read = mtty_read,
.write = mtty_write,
.ioctl = mtty_ioctl,
+ .get_alias_length = mtty_get_alias_length
};
static void mtty_device_release(struct device *dev)
--
2.19.2
To have consistent naming for the netdevice of a mdev and to have
consistent naming of the devlink port [1] of a mdev, which is formed using
phys_port_name of the devlink port, current UUID is not usable because
UUID is too long.
UUID in string format is 36-characters long and in binary 128-bit.
Both formats are not able to fit within 15 characters limit of netdev
name.
It is desired to have mdev device naming consistent using UUID.
So that widely used user space framework such as ovs [2] can make use
of mdev representor in similar way as PCIe SR-IOV VF and PF representors.
Hence,
(a) mdev alias is created which is derived using sha1 from the mdev name.
(b) Vendor driver describes how long an alias should be for the child mdev
created for a given parent.
(c) Mdev aliases are unique at system level.
(d) alias is created optionally whenever parent requested.
This ensures that non networking mdev parents can function without alias
creation overhead.
This design is discussed at [3].
An example systemd/udev extension will have,
1. netdev name created using mdev alias available in sysfs.
mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001
mdev 12 character alias=cd5b146a80a5
netdev name of this mdev = enmcd5b146a80a5
Here en = Ethernet link
m = mediated device
2. devlink port phys_port_name created using mdev alias.
devlink phys_port_name=pcd5b146a80a5
This patchset enables mdev core to maintain unique alias for a mdev.
Patch-1 Introduces mdev alias using sha1.
Patch-2 Ensures that mdev alias is unique in a system.
Patch-3 Exposes mdev alias in a sysfs hirerchy, update Documentation
Patch-4 Introduces mdev_alias() API.
Patch-5 Extends mtty driver to optionally provide alias generation.
This also enables to test UUID based sha1 collision and trigger
error handling for duplicate sha1 results.
[1] http://man7.org/linux/man-pages/man8/devlink-port.8.html
[2] https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html
[3] https://patchwork.kernel.org/cover/11084231/
---
Changelog:
v2->v3:
- Addressed comment from Yunsheng Lin
- Changed strcmp() ==0 to !strcmp()
- Addressed comment from Cornelia Hunk
- Merged sysfs Documentation patch with syfs patch
- Added more description for alias return value
v1->v2:
- Corrected a typo from 'and' to 'an'
- Addressed comments from Alex Williamson
- Kept mdev_device naturally aligned
- Added error checking for crypt_*() calls
- Moved alias NULL check at beginning
- Added mdev_alias() API
- Updated mtty driver to show example mdev_alias() usage
- Changed return type of generate_alias() from int to char*
v0->v1:
- Addressed comments from Alex Williamson, Cornelia Hunk and Mark Bloch
- Moved alias length check outside of the parent lock
- Moved alias and digest allocation from kvzalloc to kzalloc
- &alias[0] changed to alias
- alias_length check is nested under get_alias_length callback check
- Changed comments to start with an empty line
- Added comment where alias memory ownership is handed over to mdev device
- Fixed cleaunup of hash if mdev_bus_register() fails
- Updated documentation for new sysfs alias file
- Improved commit logs to make description more clear
- Fixed inclusiong of alias for NULL check
- Added ratelimited debug print for sha1 hash collision error
Parav Pandit (5):
mdev: Introduce sha1 based mdev alias
mdev: Make mdev alias unique among all mdevs
mdev: Expose mdev alias in sysfs tree
mdev: Introduce an API mdev_alias
mtty: Optionally support mtty alias
.../driver-api/vfio-mediated-device.rst | 9 ++
drivers/vfio/mdev/mdev_core.c | 142 +++++++++++++++++-
drivers/vfio/mdev/mdev_private.h | 5 +-
drivers/vfio/mdev/mdev_sysfs.c | 26 +++-
include/linux/mdev.h | 5 +
samples/vfio-mdev/mtty.c | 13 ++
6 files changed, 190 insertions(+), 10 deletions(-)
--
2.19.2
Mdev alias should be unique among all the mdevs, so that when such alias
is used by the mdev users to derive other objects, there is no
collision in a given system.
Signed-off-by: Parav Pandit <[email protected]>
---
Changelog:
v2->v3:
- Changed strcmp() ==0 to !strcmp()
v1->v2:
- Moved alias NULL check at beginning
v0->v1:
- Fixed inclusiong of alias for NULL check
- Added ratelimited debug print for sha1 hash collision error
---
drivers/vfio/mdev/mdev_core.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index 3bdff0469607..c8cd40366783 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -388,6 +388,13 @@ int mdev_device_create(struct kobject *kobj, struct device *dev,
ret = -EEXIST;
goto mdev_fail;
}
+ if (alias && tmp->alias && !strcmp(alias, tmp->alias)) {
+ mutex_unlock(&mdev_list_lock);
+ ret = -EEXIST;
+ dev_dbg_ratelimited(dev, "Hash collision in alias creation for UUID %pUl\n",
+ uuid);
+ goto mdev_fail;
+ }
}
mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
--
2.19.2
Hi Alex,
> -----Original Message-----
> From: Parav Pandit <[email protected]>
> Sent: Sunday, September 1, 2019 11:25 PM
> To: [email protected]; Jiri Pirko <[email protected]>;
> [email protected]; [email protected]; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; Parav Pandit <[email protected]>
> Subject: [PATCH v3 0/5] Introduce variable length mdev alias
>
> To have consistent naming for the netdevice of a mdev and to have consistent
> naming of the devlink port [1] of a mdev, which is formed using
> phys_port_name of the devlink port, current UUID is not usable because UUID
> is too long.
>
> UUID in string format is 36-characters long and in binary 128-bit.
> Both formats are not able to fit within 15 characters limit of netdev name.
>
> It is desired to have mdev device naming consistent using UUID.
> So that widely used user space framework such as ovs [2] can make use of
> mdev representor in similar way as PCIe SR-IOV VF and PF representors.
>
> Hence,
> (a) mdev alias is created which is derived using sha1 from the mdev name.
> (b) Vendor driver describes how long an alias should be for the child mdev
> created for a given parent.
> (c) Mdev aliases are unique at system level.
> (d) alias is created optionally whenever parent requested.
> This ensures that non networking mdev parents can function without alias
> creation overhead.
>
> This design is discussed at [3].
>
> An example systemd/udev extension will have,
>
> 1. netdev name created using mdev alias available in sysfs.
>
> mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001
> mdev 12 character alias=cd5b146a80a5
>
> netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link m =
> mediated device
>
> 2. devlink port phys_port_name created using mdev alias.
> devlink phys_port_name=pcd5b146a80a5
>
> This patchset enables mdev core to maintain unique alias for a mdev.
>
> Patch-1 Introduces mdev alias using sha1.
> Patch-2 Ensures that mdev alias is unique in a system.
> Patch-3 Exposes mdev alias in a sysfs hirerchy, update Documentation
> Patch-4 Introduces mdev_alias() API.
> Patch-5 Extends mtty driver to optionally provide alias generation.
> This also enables to test UUID based sha1 collision and trigger error handling
> for duplicate sha1 results.
>
> [1] http://man7.org/linux/man-pages/man8/devlink-port.8.html
> [2] https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html
> [3] https://patchwork.kernel.org/cover/11084231/
>
> ---
> Changelog:
> v2->v3:
> - Addressed comment from Yunsheng Lin
> - Changed strcmp() ==0 to !strcmp()
> - Addressed comment from Cornelia Hunk
> - Merged sysfs Documentation patch with syfs patch
> - Added more description for alias return value
Did you get a chance review this updated series?
I addressed Cornelia's and yours comment.
I do not think allocating alias memory twice, once for comparison and once for storing is good idea or moving alias generation logic inside the mdev_list_lock(). So I didn't address that suggestion of Cornelia.
> v1->v2:
> - Corrected a typo from 'and' to 'an'
> - Addressed comments from Alex Williamson
> - Kept mdev_device naturally aligned
> - Added error checking for crypt_*() calls
> - Moved alias NULL check at beginning
> - Added mdev_alias() API
> - Updated mtty driver to show example mdev_alias() usage
> - Changed return type of generate_alias() from int to char*
> v0->v1:
> - Addressed comments from Alex Williamson, Cornelia Hunk and Mark Bloch
> - Moved alias length check outside of the parent lock
> - Moved alias and digest allocation from kvzalloc to kzalloc
> - &alias[0] changed to alias
> - alias_length check is nested under get_alias_length callback check
> - Changed comments to start with an empty line
> - Added comment where alias memory ownership is handed over to mdev
> device
> - Fixed cleaunup of hash if mdev_bus_register() fails
> - Updated documentation for new sysfs alias file
> - Improved commit logs to make description more clear
> - Fixed inclusiong of alias for NULL check
> - Added ratelimited debug print for sha1 hash collision error
>
> Parav Pandit (5):
> mdev: Introduce sha1 based mdev alias
> mdev: Make mdev alias unique among all mdevs
> mdev: Expose mdev alias in sysfs tree
> mdev: Introduce an API mdev_alias
> mtty: Optionally support mtty alias
>
> .../driver-api/vfio-mediated-device.rst | 9 ++
> drivers/vfio/mdev/mdev_core.c | 142 +++++++++++++++++-
> drivers/vfio/mdev/mdev_private.h | 5 +-
> drivers/vfio/mdev/mdev_sysfs.c | 26 +++-
> include/linux/mdev.h | 5 +
> samples/vfio-mdev/mtty.c | 13 ++
> 6 files changed, 190 insertions(+), 10 deletions(-)
>
> --
> 2.19.2
On Mon, 9 Sep 2019 20:42:32 +0000
Parav Pandit <[email protected]> wrote:
> Hi Alex,
>
> > -----Original Message-----
> > From: Parav Pandit <[email protected]>
> > Sent: Sunday, September 1, 2019 11:25 PM
> > To: [email protected]; Jiri Pirko <[email protected]>;
> > [email protected]; [email protected]; [email protected]
> > Cc: [email protected]; [email protected];
> > [email protected]; Parav Pandit <[email protected]>
> > Subject: [PATCH v3 0/5] Introduce variable length mdev alias
> >
> > To have consistent naming for the netdevice of a mdev and to have consistent
> > naming of the devlink port [1] of a mdev, which is formed using
> > phys_port_name of the devlink port, current UUID is not usable because UUID
> > is too long.
> >
> > UUID in string format is 36-characters long and in binary 128-bit.
> > Both formats are not able to fit within 15 characters limit of netdev name.
> >
> > It is desired to have mdev device naming consistent using UUID.
> > So that widely used user space framework such as ovs [2] can make use of
> > mdev representor in similar way as PCIe SR-IOV VF and PF representors.
> >
> > Hence,
> > (a) mdev alias is created which is derived using sha1 from the mdev name.
> > (b) Vendor driver describes how long an alias should be for the child mdev
> > created for a given parent.
> > (c) Mdev aliases are unique at system level.
> > (d) alias is created optionally whenever parent requested.
> > This ensures that non networking mdev parents can function without alias
> > creation overhead.
> >
> > This design is discussed at [3].
> >
> > An example systemd/udev extension will have,
> >
> > 1. netdev name created using mdev alias available in sysfs.
> >
> > mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001
> > mdev 12 character alias=cd5b146a80a5
> >
> > netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link m =
> > mediated device
> >
> > 2. devlink port phys_port_name created using mdev alias.
> > devlink phys_port_name=pcd5b146a80a5
> >
> > This patchset enables mdev core to maintain unique alias for a mdev.
> >
> > Patch-1 Introduces mdev alias using sha1.
> > Patch-2 Ensures that mdev alias is unique in a system.
> > Patch-3 Exposes mdev alias in a sysfs hirerchy, update Documentation
> > Patch-4 Introduces mdev_alias() API.
> > Patch-5 Extends mtty driver to optionally provide alias generation.
> > This also enables to test UUID based sha1 collision and trigger error handling
> > for duplicate sha1 results.
> >
> > [1] http://man7.org/linux/man-pages/man8/devlink-port.8.html
> > [2] https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html
> > [3] https://patchwork.kernel.org/cover/11084231/
> >
> > ---
> > Changelog:
> > v2->v3:
> > - Addressed comment from Yunsheng Lin
> > - Changed strcmp() ==0 to !strcmp()
> > - Addressed comment from Cornelia Hunk
> > - Merged sysfs Documentation patch with syfs patch
> > - Added more description for alias return value
>
> Did you get a chance review this updated series?
> I addressed Cornelia's and yours comment.
> I do not think allocating alias memory twice, once for comparison and
> once for storing is good idea or moving alias generation logic inside
> the mdev_list_lock(). So I didn't address that suggestion of
> Cornelia.
Sorry, I'm at LPC this week. I agree, I don't think the double
allocation is necessary, I thought the comment was sufficient to
clarify null'ing the variable. It's awkward, but seems correct.
I'm not sure what we do with this patch series though, has the real
consumer of this even been proposed? It feels optimistic to include at
this point. We've used the sample driver as a placeholder in the past
for mdev_uuid(), but we arrived at that via a conversion rather than
explicitly adding the API. Please let me know where the consumer
patches stand, perhaps it would make more sense for them to go in
together rather than risk adding an unused API. Thanks,
Alex
> > v1->v2:
> > - Corrected a typo from 'and' to 'an'
> > - Addressed comments from Alex Williamson
> > - Kept mdev_device naturally aligned
> > - Added error checking for crypt_*() calls
> > - Moved alias NULL check at beginning
> > - Added mdev_alias() API
> > - Updated mtty driver to show example mdev_alias() usage
> > - Changed return type of generate_alias() from int to char*
> > v0->v1:
> > - Addressed comments from Alex Williamson, Cornelia Hunk and Mark
> > Bloch
> > - Moved alias length check outside of the parent lock
> > - Moved alias and digest allocation from kvzalloc to kzalloc
> > - &alias[0] changed to alias
> > - alias_length check is nested under get_alias_length callback
> > check
> > - Changed comments to start with an empty line
> > - Added comment where alias memory ownership is handed over to mdev
> > device
> > - Fixed cleaunup of hash if mdev_bus_register() fails
> > - Updated documentation for new sysfs alias file
> > - Improved commit logs to make description more clear
> > - Fixed inclusiong of alias for NULL check
> > - Added ratelimited debug print for sha1 hash collision error
> >
> > Parav Pandit (5):
> > mdev: Introduce sha1 based mdev alias
> > mdev: Make mdev alias unique among all mdevs
> > mdev: Expose mdev alias in sysfs tree
> > mdev: Introduce an API mdev_alias
> > mtty: Optionally support mtty alias
> >
> > .../driver-api/vfio-mediated-device.rst | 9 ++
> > drivers/vfio/mdev/mdev_core.c | 142
> > +++++++++++++++++- drivers/vfio/mdev/mdev_private.h
> > | 5 +- drivers/vfio/mdev/mdev_sysfs.c | 26 +++-
> > include/linux/mdev.h | 5 +
> > samples/vfio-mdev/mtty.c | 13 ++
> > 6 files changed, 190 insertions(+), 10 deletions(-)
> >
> > --
> > 2.19.2
>
Hi Alex,
> -----Original Message-----
> From: Alex Williamson <[email protected]>
> Sent: Wednesday, September 11, 2019 8:56 AM
> To: Parav Pandit <[email protected]>
> Cc: Jiri Pirko <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH v3 0/5] Introduce variable length mdev alias
>
> On Mon, 9 Sep 2019 20:42:32 +0000
> Parav Pandit <[email protected]> wrote:
>
> > Hi Alex,
> >
> > > -----Original Message-----
> > > From: Parav Pandit <[email protected]>
> > > Sent: Sunday, September 1, 2019 11:25 PM
> > > To: [email protected]; Jiri Pirko <[email protected]>;
> > > [email protected]; [email protected]; [email protected]
> > > Cc: [email protected]; [email protected];
> > > [email protected]; Parav Pandit <[email protected]>
> > > Subject: [PATCH v3 0/5] Introduce variable length mdev alias
> > >
> > > To have consistent naming for the netdevice of a mdev and to have
> > > consistent naming of the devlink port [1] of a mdev, which is formed
> > > using phys_port_name of the devlink port, current UUID is not usable
> > > because UUID is too long.
> > >
> > > UUID in string format is 36-characters long and in binary 128-bit.
> > > Both formats are not able to fit within 15 characters limit of netdev
> name.
> > >
> > > It is desired to have mdev device naming consistent using UUID.
> > > So that widely used user space framework such as ovs [2] can make
> > > use of mdev representor in similar way as PCIe SR-IOV VF and PF
> representors.
> > >
> > > Hence,
> > > (a) mdev alias is created which is derived using sha1 from the mdev
> name.
> > > (b) Vendor driver describes how long an alias should be for the
> > > child mdev created for a given parent.
> > > (c) Mdev aliases are unique at system level.
> > > (d) alias is created optionally whenever parent requested.
> > > This ensures that non networking mdev parents can function without
> > > alias creation overhead.
> > >
> > > This design is discussed at [3].
> > >
> > > An example systemd/udev extension will have,
> > >
> > > 1. netdev name created using mdev alias available in sysfs.
> > >
> > > mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001
> > > mdev 12 character alias=cd5b146a80a5
> > >
> > > netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link m
> > > = mediated device
> > >
> > > 2. devlink port phys_port_name created using mdev alias.
> > > devlink phys_port_name=pcd5b146a80a5
> > >
> > > This patchset enables mdev core to maintain unique alias for a mdev.
> > >
> > > Patch-1 Introduces mdev alias using sha1.
> > > Patch-2 Ensures that mdev alias is unique in a system.
> > > Patch-3 Exposes mdev alias in a sysfs hirerchy, update Documentation
> > > Patch-4 Introduces mdev_alias() API.
> > > Patch-5 Extends mtty driver to optionally provide alias generation.
> > > This also enables to test UUID based sha1 collision and trigger
> > > error handling for duplicate sha1 results.
> > >
> > > [1] http://man7.org/linux/man-pages/man8/devlink-port.8.html
> > > [2] https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html
> > > [3] https://patchwork.kernel.org/cover/11084231/
> > >
> > > ---
> > > Changelog:
> > > v2->v3:
> > > - Addressed comment from Yunsheng Lin
> > > - Changed strcmp() ==0 to !strcmp()
> > > - Addressed comment from Cornelia Hunk
> > > - Merged sysfs Documentation patch with syfs patch
> > > - Added more description for alias return value
> >
> > Did you get a chance review this updated series?
> > I addressed Cornelia's and yours comment.
> > I do not think allocating alias memory twice, once for comparison and
> > once for storing is good idea or moving alias generation logic inside
> > the mdev_list_lock(). So I didn't address that suggestion of Cornelia.
>
> Sorry, I'm at LPC this week. I agree, I don't think the double allocation is
> necessary, I thought the comment was sufficient to clarify null'ing the
> variable. It's awkward, but seems correct.
>
> I'm not sure what we do with this patch series though, has the real
> consumer of this even been proposed? It feels optimistic to include at this
> point. We've used the sample driver as a placeholder in the past for
> mdev_uuid(), but we arrived at that via a conversion rather than explicitly
> adding the API. Please let me know where the consumer patches stand,
> perhaps it would make more sense for them to go in together rather than
> risk adding an unused API. Thanks,
>
Given that consumer patch series is relatively large (around 15+ patches), I was considering to merge this one as pre-series to it.
Its ok to combine this with consumer patch series.
But wanted to have it reviewed beforehand, so that churn is less in actual consumer series which is more mlx5_core and devlink/netdev centric.
So if you can add Review-by, it will be easier to combine with consumer series.
And if we merge it with consumer series, it will come through Dave Miller's tree instead of your tree.
Would that work for you?
On Wed, 11 Sep 2019 15:30:40 +0000
Parav Pandit <[email protected]> wrote:
> Hi Alex,
>
> > -----Original Message-----
> > From: Alex Williamson <[email protected]>
> > Sent: Wednesday, September 11, 2019 8:56 AM
> > To: Parav Pandit <[email protected]>
> > Cc: Jiri Pirko <[email protected]>; [email protected];
> > [email protected]; [email protected]; [email protected]; linux-
> > [email protected]; [email protected]
> > Subject: Re: [PATCH v3 0/5] Introduce variable length mdev alias
> >
> > On Mon, 9 Sep 2019 20:42:32 +0000
> > Parav Pandit <[email protected]> wrote:
> >
> > > Hi Alex,
> > >
> > > > -----Original Message-----
> > > > From: Parav Pandit <[email protected]>
> > > > Sent: Sunday, September 1, 2019 11:25 PM
> > > > To: [email protected]; Jiri Pirko <[email protected]>;
> > > > [email protected]; [email protected]; [email protected]
> > > > Cc: [email protected]; [email protected];
> > > > [email protected]; Parav Pandit <[email protected]>
> > > > Subject: [PATCH v3 0/5] Introduce variable length mdev alias
> > > >
> > > > To have consistent naming for the netdevice of a mdev and to have
> > > > consistent naming of the devlink port [1] of a mdev, which is formed
> > > > using phys_port_name of the devlink port, current UUID is not usable
> > > > because UUID is too long.
> > > >
> > > > UUID in string format is 36-characters long and in binary 128-bit.
> > > > Both formats are not able to fit within 15 characters limit of netdev
> > name.
> > > >
> > > > It is desired to have mdev device naming consistent using UUID.
> > > > So that widely used user space framework such as ovs [2] can make
> > > > use of mdev representor in similar way as PCIe SR-IOV VF and PF
> > representors.
> > > >
> > > > Hence,
> > > > (a) mdev alias is created which is derived using sha1 from the mdev
> > name.
> > > > (b) Vendor driver describes how long an alias should be for the
> > > > child mdev created for a given parent.
> > > > (c) Mdev aliases are unique at system level.
> > > > (d) alias is created optionally whenever parent requested.
> > > > This ensures that non networking mdev parents can function without
> > > > alias creation overhead.
> > > >
> > > > This design is discussed at [3].
> > > >
> > > > An example systemd/udev extension will have,
> > > >
> > > > 1. netdev name created using mdev alias available in sysfs.
> > > >
> > > > mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001
> > > > mdev 12 character alias=cd5b146a80a5
> > > >
> > > > netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link m
> > > > = mediated device
> > > >
> > > > 2. devlink port phys_port_name created using mdev alias.
> > > > devlink phys_port_name=pcd5b146a80a5
> > > >
> > > > This patchset enables mdev core to maintain unique alias for a mdev.
> > > >
> > > > Patch-1 Introduces mdev alias using sha1.
> > > > Patch-2 Ensures that mdev alias is unique in a system.
> > > > Patch-3 Exposes mdev alias in a sysfs hirerchy, update Documentation
> > > > Patch-4 Introduces mdev_alias() API.
> > > > Patch-5 Extends mtty driver to optionally provide alias generation.
> > > > This also enables to test UUID based sha1 collision and trigger
> > > > error handling for duplicate sha1 results.
> > > >
> > > > [1] http://man7.org/linux/man-pages/man8/devlink-port.8.html
> > > > [2] https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html
> > > > [3] https://patchwork.kernel.org/cover/11084231/
> > > >
> > > > ---
> > > > Changelog:
> > > > v2->v3:
> > > > - Addressed comment from Yunsheng Lin
> > > > - Changed strcmp() ==0 to !strcmp()
> > > > - Addressed comment from Cornelia Hunk
> > > > - Merged sysfs Documentation patch with syfs patch
> > > > - Added more description for alias return value
> > >
> > > Did you get a chance review this updated series?
> > > I addressed Cornelia's and yours comment.
> > > I do not think allocating alias memory twice, once for comparison and
> > > once for storing is good idea or moving alias generation logic inside
> > > the mdev_list_lock(). So I didn't address that suggestion of Cornelia.
> >
> > Sorry, I'm at LPC this week. I agree, I don't think the double allocation is
> > necessary, I thought the comment was sufficient to clarify null'ing the
> > variable. It's awkward, but seems correct.
Not hot about it, but no real complaints.
However, please give me some more time, as I'm at LPC as well.
> >
> > I'm not sure what we do with this patch series though, has the real
> > consumer of this even been proposed? It feels optimistic to include at this
> > point. We've used the sample driver as a placeholder in the past for
> > mdev_uuid(), but we arrived at that via a conversion rather than explicitly
> > adding the API. Please let me know where the consumer patches stand,
> > perhaps it would make more sense for them to go in together rather than
> > risk adding an unused API. Thanks,
> >
> Given that consumer patch series is relatively large (around 15+ patches), I was considering to merge this one as pre-series to it.
> Its ok to combine this with consumer patch series.
> But wanted to have it reviewed beforehand, so that churn is less in actual consumer series which is more mlx5_core and devlink/netdev centric.
> So if you can add Review-by, it will be easier to combine with consumer series.
>
> And if we merge it with consumer series, it will come through Dave Miller's tree instead of your tree.
> Would that work for you?
It would be easier to see what to do here if we could see the consumer
for this. If those patches are fine, we could maybe queue this series
via both trees?
> -----Original Message-----
> From: [email protected] <linux-kernel-
> [email protected]> On Behalf Of Parav Pandit
> Sent: Wednesday, September 11, 2019 10:31 AM
> To: Alex Williamson <[email protected]>
> Cc: Jiri Pirko <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: RE: [PATCH v3 0/5] Introduce variable length mdev alias
>
> Hi Alex,
>
> > -----Original Message-----
> > From: Alex Williamson <[email protected]>
> > Sent: Wednesday, September 11, 2019 8:56 AM
> > To: Parav Pandit <[email protected]>
> > Cc: Jiri Pirko <[email protected]>; [email protected];
> > [email protected]; [email protected]; [email protected]; linux-
> > [email protected]; [email protected]
> > Subject: Re: [PATCH v3 0/5] Introduce variable length mdev alias
> >
> > On Mon, 9 Sep 2019 20:42:32 +0000
> > Parav Pandit <[email protected]> wrote:
> >
> > > Hi Alex,
> > >
> > > > -----Original Message-----
> > > > From: Parav Pandit <[email protected]>
> > > > Sent: Sunday, September 1, 2019 11:25 PM
> > > > To: [email protected]; Jiri Pirko <[email protected]>;
> > > > [email protected]; [email protected]; [email protected]
> > > > Cc: [email protected]; [email protected];
> > > > [email protected]; Parav Pandit <[email protected]>
> > > > Subject: [PATCH v3 0/5] Introduce variable length mdev alias
> > > >
> > > > To have consistent naming for the netdevice of a mdev and to have
> > > > consistent naming of the devlink port [1] of a mdev, which is
> > > > formed using phys_port_name of the devlink port, current UUID is
> > > > not usable because UUID is too long.
> > > >
> > > > UUID in string format is 36-characters long and in binary 128-bit.
> > > > Both formats are not able to fit within 15 characters limit of
> > > > netdev
> > name.
> > > >
> > > > It is desired to have mdev device naming consistent using UUID.
> > > > So that widely used user space framework such as ovs [2] can make
> > > > use of mdev representor in similar way as PCIe SR-IOV VF and PF
> > representors.
> > > >
> > > > Hence,
> > > > (a) mdev alias is created which is derived using sha1 from the
> > > > mdev
> > name.
> > > > (b) Vendor driver describes how long an alias should be for the
> > > > child mdev created for a given parent.
> > > > (c) Mdev aliases are unique at system level.
> > > > (d) alias is created optionally whenever parent requested.
> > > > This ensures that non networking mdev parents can function without
> > > > alias creation overhead.
> > > >
> > > > This design is discussed at [3].
> > > >
> > > > An example systemd/udev extension will have,
> > > >
> > > > 1. netdev name created using mdev alias available in sysfs.
> > > >
> > > > mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001
> > > > mdev 12 character alias=cd5b146a80a5
> > > >
> > > > netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link
> > > > m = mediated device
> > > >
> > > > 2. devlink port phys_port_name created using mdev alias.
> > > > devlink phys_port_name=pcd5b146a80a5
> > > >
> > > > This patchset enables mdev core to maintain unique alias for a mdev.
> > > >
> > > > Patch-1 Introduces mdev alias using sha1.
> > > > Patch-2 Ensures that mdev alias is unique in a system.
> > > > Patch-3 Exposes mdev alias in a sysfs hirerchy, update
> > > > Documentation
> > > > Patch-4 Introduces mdev_alias() API.
> > > > Patch-5 Extends mtty driver to optionally provide alias generation.
> > > > This also enables to test UUID based sha1 collision and trigger
> > > > error handling for duplicate sha1 results.
> > > >
> > > > [1] http://man7.org/linux/man-pages/man8/devlink-port.8.html
> > > > [2] https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html
> > > > [3] https://patchwork.kernel.org/cover/11084231/
> > > >
> > > > ---
> > > > Changelog:
> > > > v2->v3:
> > > > - Addressed comment from Yunsheng Lin
> > > > - Changed strcmp() ==0 to !strcmp()
> > > > - Addressed comment from Cornelia Hunk
> > > > - Merged sysfs Documentation patch with syfs patch
> > > > - Added more description for alias return value
> > >
> > > Did you get a chance review this updated series?
> > > I addressed Cornelia's and yours comment.
> > > I do not think allocating alias memory twice, once for comparison
> > > and once for storing is good idea or moving alias generation logic
> > > inside the mdev_list_lock(). So I didn't address that suggestion of
> Cornelia.
> >
> > Sorry, I'm at LPC this week. I agree, I don't think the double
> > allocation is necessary, I thought the comment was sufficient to
> > clarify null'ing the variable. It's awkward, but seems correct.
> >
> > I'm not sure what we do with this patch series though, has the real
> > consumer of this even been proposed?
Jiri already acked to use mdev_alias() to generate phys_port_name several days back in the discussion we had in [1].
After concluding in the thread [1], I proceed with mdev_alias().
mlx5_core patches are not yet present on netdev mailing list, but we all agree to use it in mdev_alias() in devlink phys_port_name generation.
So we have collective agreement on how to proceed forward.
I wasn't probably clear enough in previous email reply about it, so adding link here.
[1] https://patchwork.kernel.org/cover/11084231/#22838955
> It feels optimistic to include
> > at this point. We've used the sample driver as a placeholder in the
> > past for mdev_uuid(), but we arrived at that via a conversion rather
> > than explicitly adding the API. Please let me know where the consumer
> > patches stand, perhaps it would make more sense for them to go in
> > together rather than risk adding an unused API. Thanks,
> >
> Given that consumer patch series is relatively large (around 15+ patches), I
> was considering to merge this one as pre-series to it.
> Its ok to combine this with consumer patch series.
> But wanted to have it reviewed beforehand, so that churn is less in actual
> consumer series which is more mlx5_core and devlink/netdev centric.
> So if you can add Review-by, it will be easier to combine with consumer
> series.
>
> And if we merge it with consumer series, it will come through Dave Miller's
> tree instead of your tree.
> Would that work for you?
On Wed, 11 Sep 2019 16:38:49 +0000
Parav Pandit <[email protected]> wrote:
> > -----Original Message-----
> > From: [email protected] <linux-kernel-
> > [email protected]> On Behalf Of Parav Pandit
> > Sent: Wednesday, September 11, 2019 10:31 AM
> > To: Alex Williamson <[email protected]>
> > Cc: Jiri Pirko <[email protected]>; [email protected];
> > [email protected]; [email protected]; [email protected]; linux-
> > [email protected]; [email protected]
> > Subject: RE: [PATCH v3 0/5] Introduce variable length mdev alias
> >
> > Hi Alex,
> >
> > > -----Original Message-----
> > > From: Alex Williamson <[email protected]>
> > > Sent: Wednesday, September 11, 2019 8:56 AM
> > > To: Parav Pandit <[email protected]>
> > > Cc: Jiri Pirko <[email protected]>; [email protected];
> > > [email protected]; [email protected]; [email protected]; linux-
> > > [email protected]; [email protected]
> > > Subject: Re: [PATCH v3 0/5] Introduce variable length mdev alias
> > >
> > > On Mon, 9 Sep 2019 20:42:32 +0000
> > > Parav Pandit <[email protected]> wrote:
> > >
> > > > Hi Alex,
> > > >
> > > > > -----Original Message-----
> > > > > From: Parav Pandit <[email protected]>
> > > > > Sent: Sunday, September 1, 2019 11:25 PM
> > > > > To: [email protected]; Jiri Pirko <[email protected]>;
> > > > > [email protected]; [email protected]; [email protected]
> > > > > Cc: [email protected]; [email protected];
> > > > > [email protected]; Parav Pandit <[email protected]>
> > > > > Subject: [PATCH v3 0/5] Introduce variable length mdev alias
> > > > >
> > > > > To have consistent naming for the netdevice of a mdev and to have
> > > > > consistent naming of the devlink port [1] of a mdev, which is
> > > > > formed using phys_port_name of the devlink port, current UUID is
> > > > > not usable because UUID is too long.
> > > > >
> > > > > UUID in string format is 36-characters long and in binary 128-bit.
> > > > > Both formats are not able to fit within 15 characters limit of
> > > > > netdev
> > > name.
> > > > >
> > > > > It is desired to have mdev device naming consistent using UUID.
> > > > > So that widely used user space framework such as ovs [2] can make
> > > > > use of mdev representor in similar way as PCIe SR-IOV VF and PF
> > > representors.
> > > > >
> > > > > Hence,
> > > > > (a) mdev alias is created which is derived using sha1 from the
> > > > > mdev
> > > name.
> > > > > (b) Vendor driver describes how long an alias should be for the
> > > > > child mdev created for a given parent.
> > > > > (c) Mdev aliases are unique at system level.
> > > > > (d) alias is created optionally whenever parent requested.
> > > > > This ensures that non networking mdev parents can function without
> > > > > alias creation overhead.
> > > > >
> > > > > This design is discussed at [3].
> > > > >
> > > > > An example systemd/udev extension will have,
> > > > >
> > > > > 1. netdev name created using mdev alias available in sysfs.
> > > > >
> > > > > mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001
> > > > > mdev 12 character alias=cd5b146a80a5
> > > > >
> > > > > netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link
> > > > > m = mediated device
> > > > >
> > > > > 2. devlink port phys_port_name created using mdev alias.
> > > > > devlink phys_port_name=pcd5b146a80a5
> > > > >
> > > > > This patchset enables mdev core to maintain unique alias for a mdev.
> > > > >
> > > > > Patch-1 Introduces mdev alias using sha1.
> > > > > Patch-2 Ensures that mdev alias is unique in a system.
> > > > > Patch-3 Exposes mdev alias in a sysfs hirerchy, update
> > > > > Documentation
> > > > > Patch-4 Introduces mdev_alias() API.
> > > > > Patch-5 Extends mtty driver to optionally provide alias generation.
> > > > > This also enables to test UUID based sha1 collision and trigger
> > > > > error handling for duplicate sha1 results.
> > > > >
> > > > > [1] http://man7.org/linux/man-pages/man8/devlink-port.8.html
> > > > > [2] https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html
> > > > > [3] https://patchwork.kernel.org/cover/11084231/
> > > > >
> > > > > ---
> > > > > Changelog:
> > > > > v2->v3:
> > > > > - Addressed comment from Yunsheng Lin
> > > > > - Changed strcmp() ==0 to !strcmp()
> > > > > - Addressed comment from Cornelia Hunk
> > > > > - Merged sysfs Documentation patch with syfs patch
> > > > > - Added more description for alias return value
> > > >
> > > > Did you get a chance review this updated series?
> > > > I addressed Cornelia's and yours comment.
> > > > I do not think allocating alias memory twice, once for comparison
> > > > and once for storing is good idea or moving alias generation logic
> > > > inside the mdev_list_lock(). So I didn't address that suggestion of
> > Cornelia.
> > >
> > > Sorry, I'm at LPC this week. I agree, I don't think the double
> > > allocation is necessary, I thought the comment was sufficient to
> > > clarify null'ing the variable. It's awkward, but seems correct.
> > >
> > > I'm not sure what we do with this patch series though, has the real
> > > consumer of this even been proposed?
>
> Jiri already acked to use mdev_alias() to generate phys_port_name several days back in the discussion we had in [1].
> After concluding in the thread [1], I proceed with mdev_alias().
> mlx5_core patches are not yet present on netdev mailing list, but we
> all agree to use it in mdev_alias() in devlink phys_port_name
> generation. So we have collective agreement on how to proceed
> forward. I wasn't probably clear enough in previous email reply about
> it, so adding link here.
>
> [1] https://patchwork.kernel.org/cover/11084231/#22838955
Jiri may have agreed to the concept, but without patches on the list
proving an end to end solution, I think it's too early for us to commit
to this by preemptively adding it to our API. "Acked" and "collective
agreement" seem like they overstate something that seems not to have
seen the light of day yet. Instead I'll say, it looks reasonable, come
back when the real consumer has actually been proposed upstream and has
more buy-in from the community and we'll see if it still looks like the
right approach from an mdev perspective then. Thanks,
Alex
Hi Alex,
> -----Original Message-----
> From: Alex Williamson <[email protected]>
> Sent: Friday, September 13, 2019 4:33 PM
> To: Parav Pandit <[email protected]>
> Cc: Jiri Pirko <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH v3 0/5] Introduce variable length mdev alias
>
> On Wed, 11 Sep 2019 16:38:49 +0000
> Parav Pandit <[email protected]> wrote:
>
> > > -----Original Message-----
> > > From: [email protected] <linux-kernel-
> > > [email protected]> On Behalf Of Parav Pandit
> > > Sent: Wednesday, September 11, 2019 10:31 AM
> > > To: Alex Williamson <[email protected]>
> > > Cc: Jiri Pirko <[email protected]>; [email protected];
> > > [email protected]; [email protected]; [email protected]; linux-
> > > [email protected]; [email protected]
> > > Subject: RE: [PATCH v3 0/5] Introduce variable length mdev alias
> > >
> > > Hi Alex,
> > >
> > > > -----Original Message-----
> > > > From: Alex Williamson <[email protected]>
> > > > Sent: Wednesday, September 11, 2019 8:56 AM
> > > > To: Parav Pandit <[email protected]>
> > > > Cc: Jiri Pirko <[email protected]>; [email protected];
> > > > [email protected]; [email protected]; [email protected];
> > > > linux- [email protected]; [email protected]
> > > > Subject: Re: [PATCH v3 0/5] Introduce variable length mdev alias
> > > >
> > > > On Mon, 9 Sep 2019 20:42:32 +0000
> > > > Parav Pandit <[email protected]> wrote:
> > > >
> > > > > Hi Alex,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Parav Pandit <[email protected]>
> > > > > > Sent: Sunday, September 1, 2019 11:25 PM
> > > > > > To: [email protected]; Jiri Pirko
> > > > > > <[email protected]>; [email protected]; [email protected];
> > > > > > [email protected]
> > > > > > Cc: [email protected]; [email protected];
> > > > > > [email protected]; Parav Pandit <[email protected]>
> > > > > > Subject: [PATCH v3 0/5] Introduce variable length mdev alias
> > > > > >
> > > > > > To have consistent naming for the netdevice of a mdev and to
> > > > > > have consistent naming of the devlink port [1] of a mdev,
> > > > > > which is formed using phys_port_name of the devlink port,
> > > > > > current UUID is not usable because UUID is too long.
> > > > > >
> > > > > > UUID in string format is 36-characters long and in binary 128-bit.
> > > > > > Both formats are not able to fit within 15 characters limit of
> > > > > > netdev
> > > > name.
> > > > > >
> > > > > > It is desired to have mdev device naming consistent using UUID.
> > > > > > So that widely used user space framework such as ovs [2] can
> > > > > > make use of mdev representor in similar way as PCIe SR-IOV VF
> > > > > > and PF
> > > > representors.
> > > > > >
> > > > > > Hence,
> > > > > > (a) mdev alias is created which is derived using sha1 from the
> > > > > > mdev
> > > > name.
> > > > > > (b) Vendor driver describes how long an alias should be for
> > > > > > the child mdev created for a given parent.
> > > > > > (c) Mdev aliases are unique at system level.
> > > > > > (d) alias is created optionally whenever parent requested.
> > > > > > This ensures that non networking mdev parents can function
> > > > > > without alias creation overhead.
> > > > > >
> > > > > > This design is discussed at [3].
> > > > > >
> > > > > > An example systemd/udev extension will have,
> > > > > >
> > > > > > 1. netdev name created using mdev alias available in sysfs.
> > > > > >
> > > > > > mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001
> > > > > > mdev 12 character alias=cd5b146a80a5
> > > > > >
> > > > > > netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet
> > > > > > link m = mediated device
> > > > > >
> > > > > > 2. devlink port phys_port_name created using mdev alias.
> > > > > > devlink phys_port_name=pcd5b146a80a5
> > > > > >
> > > > > > This patchset enables mdev core to maintain unique alias for a mdev.
> > > > > >
> > > > > > Patch-1 Introduces mdev alias using sha1.
> > > > > > Patch-2 Ensures that mdev alias is unique in a system.
> > > > > > Patch-3 Exposes mdev alias in a sysfs hirerchy, update
> > > > > > Documentation
> > > > > > Patch-4 Introduces mdev_alias() API.
> > > > > > Patch-5 Extends mtty driver to optionally provide alias generation.
> > > > > > This also enables to test UUID based sha1 collision and
> > > > > > trigger error handling for duplicate sha1 results.
> > > > > >
> > > > > > [1] http://man7.org/linux/man-pages/man8/devlink-port.8.html
> > > > > > [2]
> > > > > > https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html
> > > > > > [3] https://patchwork.kernel.org/cover/11084231/
> > > > > >
> > > > > > ---
> > > > > > Changelog:
> > > > > > v2->v3:
> > > > > > - Addressed comment from Yunsheng Lin
> > > > > > - Changed strcmp() ==0 to !strcmp()
> > > > > > - Addressed comment from Cornelia Hunk
> > > > > > - Merged sysfs Documentation patch with syfs patch
> > > > > > - Added more description for alias return value
> > > > >
> > > > > Did you get a chance review this updated series?
> > > > > I addressed Cornelia's and yours comment.
> > > > > I do not think allocating alias memory twice, once for
> > > > > comparison and once for storing is good idea or moving alias
> > > > > generation logic inside the mdev_list_lock(). So I didn't
> > > > > address that suggestion of
> > > Cornelia.
> > > >
> > > > Sorry, I'm at LPC this week. I agree, I don't think the double
> > > > allocation is necessary, I thought the comment was sufficient to
> > > > clarify null'ing the variable. It's awkward, but seems correct.
> > > >
> > > > I'm not sure what we do with this patch series though, has the real
> > > > consumer of this even been proposed?
> >
> > Jiri already acked to use mdev_alias() to generate phys_port_name several
> days back in the discussion we had in [1].
> > After concluding in the thread [1], I proceed with mdev_alias().
> > mlx5_core patches are not yet present on netdev mailing list, but we
> > all agree to use it in mdev_alias() in devlink phys_port_name
> > generation. So we have collective agreement on how to proceed forward.
> > I wasn't probably clear enough in previous email reply about it, so
> > adding link here.
> >
> > [1] https://patchwork.kernel.org/cover/11084231/#22838955
>
> Jiri may have agreed to the concept, but without patches on the list proving an
> end to end solution, I think it's too early for us to commit to this by
> preemptively adding it to our API. "Acked" and "collective agreement" seem
> like they overstate something that seems not to have seen the light of day yet.
> Instead I'll say, it looks reasonable, come back when the real consumer has
> actually been proposed upstream and has more buy-in from the community
> and we'll see if it still looks like the right approach from an mdev perspective
> then. Thanks,
>
Ok. I will combine these patches with the actual consumer patches of mdev_alias().
Thanks.
> Alex
On Sun, 1 Sep 2019 23:24:31 -0500
Parav Pandit <[email protected]> wrote:
> To have consistent naming for the netdevice of a mdev and to have
> consistent naming of the devlink port [1] of a mdev, which is formed using
> phys_port_name of the devlink port, current UUID is not usable because
> UUID is too long.
>
> UUID in string format is 36-characters long and in binary 128-bit.
> Both formats are not able to fit within 15 characters limit of netdev
> name.
>
> It is desired to have mdev device naming consistent using UUID.
> So that widely used user space framework such as ovs [2] can make use
> of mdev representor in similar way as PCIe SR-IOV VF and PF representors.
>
> Hence,
> (a) mdev alias is created which is derived using sha1 from the mdev name.
> (b) Vendor driver describes how long an alias should be for the child mdev
> created for a given parent.
> (c) Mdev aliases are unique at system level.
> (d) alias is created optionally whenever parent requested.
> This ensures that non networking mdev parents can function without alias
> creation overhead.
>
> This design is discussed at [3].
>
> An example systemd/udev extension will have,
>
> 1. netdev name created using mdev alias available in sysfs.
>
> mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001
> mdev 12 character alias=cd5b146a80a5
>
> netdev name of this mdev = enmcd5b146a80a5
> Here en = Ethernet link
> m = mediated device
>
> 2. devlink port phys_port_name created using mdev alias.
> devlink phys_port_name=pcd5b146a80a5
>
> This patchset enables mdev core to maintain unique alias for a mdev.
>
> Patch-1 Introduces mdev alias using sha1.
> Patch-2 Ensures that mdev alias is unique in a system.
> Patch-3 Exposes mdev alias in a sysfs hirerchy, update Documentation
> Patch-4 Introduces mdev_alias() API.
> Patch-5 Extends mtty driver to optionally provide alias generation.
> This also enables to test UUID based sha1 collision and trigger
> error handling for duplicate sha1 results.
>
> [1] http://man7.org/linux/man-pages/man8/devlink-port.8.html
> [2] https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html
> [3] https://patchwork.kernel.org/cover/11084231/
>
> ---
> Changelog:
> v2->v3:
> - Addressed comment from Yunsheng Lin
> - Changed strcmp() ==0 to !strcmp()
> - Addressed comment from Cornelia Hunk
> - Merged sysfs Documentation patch with syfs patch
> - Added more description for alias return value
> v1->v2:
> - Corrected a typo from 'and' to 'an'
> - Addressed comments from Alex Williamson
> - Kept mdev_device naturally aligned
> - Added error checking for crypt_*() calls
> - Moved alias NULL check at beginning
> - Added mdev_alias() API
> - Updated mtty driver to show example mdev_alias() usage
> - Changed return type of generate_alias() from int to char*
> v0->v1:
> - Addressed comments from Alex Williamson, Cornelia Hunk and Mark Bloch
> - Moved alias length check outside of the parent lock
> - Moved alias and digest allocation from kvzalloc to kzalloc
> - &alias[0] changed to alias
> - alias_length check is nested under get_alias_length callback check
> - Changed comments to start with an empty line
> - Added comment where alias memory ownership is handed over to mdev device
> - Fixed cleaunup of hash if mdev_bus_register() fails
> - Updated documentation for new sysfs alias file
> - Improved commit logs to make description more clear
> - Fixed inclusiong of alias for NULL check
> - Added ratelimited debug print for sha1 hash collision error
>
> Parav Pandit (5):
> mdev: Introduce sha1 based mdev alias
> mdev: Make mdev alias unique among all mdevs
> mdev: Expose mdev alias in sysfs tree
> mdev: Introduce an API mdev_alias
> mtty: Optionally support mtty alias
>
> .../driver-api/vfio-mediated-device.rst | 9 ++
> drivers/vfio/mdev/mdev_core.c | 142 +++++++++++++++++-
> drivers/vfio/mdev/mdev_private.h | 5 +-
> drivers/vfio/mdev/mdev_sysfs.c | 26 +++-
> include/linux/mdev.h | 5 +
> samples/vfio-mdev/mtty.c | 13 ++
> 6 files changed, 190 insertions(+), 10 deletions(-)
>
The patches on their own look sane (and I gave my R-b), but the
consumer of this new API should be ready before this is merged, as
already discussed below.
On Sun, 1 Sep 2019 23:24:33 -0500
Parav Pandit <[email protected]> wrote:
> Mdev alias should be unique among all the mdevs, so that when such alias
> is used by the mdev users to derive other objects, there is no
> collision in a given system.
>
> Signed-off-by: Parav Pandit <[email protected]>
>
> ---
> Changelog:
> v2->v3:
> - Changed strcmp() ==0 to !strcmp()
> v1->v2:
> - Moved alias NULL check at beginning
> v0->v1:
> - Fixed inclusiong of alias for NULL check
> - Added ratelimited debug print for sha1 hash collision error
> ---
> drivers/vfio/mdev/mdev_core.c | 7 +++++++
> 1 file changed, 7 insertions(+)
Reviewed-by: Cornelia Huck <[email protected]>
Hi Cornelia,
> -----Original Message-----
> From: Cornelia Huck <[email protected]>
> Sent: Tuesday, September 17, 2019 5:14 AM
> To: Parav Pandit <[email protected]>
> Cc: [email protected]; Jiri Pirko <[email protected]>;
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH v3 0/5] Introduce variable length mdev alias
>
> On Sun, 1 Sep 2019 23:24:31 -0500
> Parav Pandit <[email protected]> wrote:
>
> > To have consistent naming for the netdevice of a mdev and to have
> > consistent naming of the devlink port [1] of a mdev, which is formed
> > using phys_port_name of the devlink port, current UUID is not usable
> > because UUID is too long.
> >
> > UUID in string format is 36-characters long and in binary 128-bit.
> > Both formats are not able to fit within 15 characters limit of netdev
> > name.
> >
> > It is desired to have mdev device naming consistent using UUID.
> > So that widely used user space framework such as ovs [2] can make use
> > of mdev representor in similar way as PCIe SR-IOV VF and PF representors.
> >
> > Hence,
> > (a) mdev alias is created which is derived using sha1 from the mdev name.
> > (b) Vendor driver describes how long an alias should be for the child
> > mdev created for a given parent.
> > (c) Mdev aliases are unique at system level.
> > (d) alias is created optionally whenever parent requested.
> > This ensures that non networking mdev parents can function without
> > alias creation overhead.
> >
> > This design is discussed at [3].
> >
> > An example systemd/udev extension will have,
> >
> > 1. netdev name created using mdev alias available in sysfs.
> >
> > mdev UUID=83b8f4f2-509f-382f-3c1e-e6bfe0fa1001
> > mdev 12 character alias=cd5b146a80a5
> >
> > netdev name of this mdev = enmcd5b146a80a5 Here en = Ethernet link m =
> > mediated device
> >
> > 2. devlink port phys_port_name created using mdev alias.
> > devlink phys_port_name=pcd5b146a80a5
> >
> > This patchset enables mdev core to maintain unique alias for a mdev.
> >
> > Patch-1 Introduces mdev alias using sha1.
> > Patch-2 Ensures that mdev alias is unique in a system.
> > Patch-3 Exposes mdev alias in a sysfs hirerchy, update Documentation
> > Patch-4 Introduces mdev_alias() API.
> > Patch-5 Extends mtty driver to optionally provide alias generation.
> > This also enables to test UUID based sha1 collision and trigger error
> > handling for duplicate sha1 results.
> >
> > [1] http://man7.org/linux/man-pages/man8/devlink-port.8.html
> > [2] https://docs.openstack.org/os-vif/latest/user/plugins/ovs.html
> > [3] https://patchwork.kernel.org/cover/11084231/
> >
> > ---
> > Changelog:
> > v2->v3:
> > - Addressed comment from Yunsheng Lin
> > - Changed strcmp() ==0 to !strcmp()
> > - Addressed comment from Cornelia Hunk
> > - Merged sysfs Documentation patch with syfs patch
> > - Added more description for alias return value
> > v1->v2:
> > - Corrected a typo from 'and' to 'an'
> > - Addressed comments from Alex Williamson
> > - Kept mdev_device naturally aligned
> > - Added error checking for crypt_*() calls
> > - Moved alias NULL check at beginning
> > - Added mdev_alias() API
> > - Updated mtty driver to show example mdev_alias() usage
> > - Changed return type of generate_alias() from int to char*
> > v0->v1:
> > - Addressed comments from Alex Williamson, Cornelia Hunk and Mark
> > Bloch
> > - Moved alias length check outside of the parent lock
> > - Moved alias and digest allocation from kvzalloc to kzalloc
> > - &alias[0] changed to alias
> > - alias_length check is nested under get_alias_length callback check
> > - Changed comments to start with an empty line
> > - Added comment where alias memory ownership is handed over to mdev
> > device
> > - Fixed cleaunup of hash if mdev_bus_register() fails
> > - Updated documentation for new sysfs alias file
> > - Improved commit logs to make description more clear
> > - Fixed inclusiong of alias for NULL check
> > - Added ratelimited debug print for sha1 hash collision error
> >
> > Parav Pandit (5):
> > mdev: Introduce sha1 based mdev alias
> > mdev: Make mdev alias unique among all mdevs
> > mdev: Expose mdev alias in sysfs tree
> > mdev: Introduce an API mdev_alias
> > mtty: Optionally support mtty alias
> >
> > .../driver-api/vfio-mediated-device.rst | 9 ++
> > drivers/vfio/mdev/mdev_core.c | 142 +++++++++++++++++-
> > drivers/vfio/mdev/mdev_private.h | 5 +-
> > drivers/vfio/mdev/mdev_sysfs.c | 26 +++-
> > include/linux/mdev.h | 5 +
> > samples/vfio-mdev/mtty.c | 13 ++
> > 6 files changed, 190 insertions(+), 10 deletions(-)
> >
>
> The patches on their own look sane (and I gave my R-b), but the consumer of
> this new API should be ready before this is merged, as already discussed below.
Thanks for the review. I will send v4 here to address all comments and to add your R-b tag.
I am waiting for Saeed to post other prep series of mlx5_core to be merged before I post actual consumer series, as it depends on it.
I will also drop the mtty sample patch and change-log to avoid confusion with versions when I combine them with consumer series.