diff mbox series

[net-next,v2,7/8] netdevsim: add SR-IOV functionality

Message ID 20171201013540.18106-8-jakub.kicinski@netronome.com
State Changes Requested, archived
Delegated to: BPF Maintainers
Headers show
Series xdp: make stack perform remove and tests | expand

Commit Message

Jakub Kicinski Dec. 1, 2017, 1:35 a.m. UTC 
dummy driver was extended with VF-related netdev APIs for testing
SR-IOV-related software.  netdevsim did not exist back then.
Implement SR-IOV functionality in netdevsim.  Notable difference
is that since netdevsim has no module parameters, we will actually
create a device with sriov_numvfs attribute for each netdev.
The zero MAC address is accepted as some HW use it to mean any
address is allowed.  Link state is also now validated.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
CC: Phil Sutter <phil@nwl.cc>
CC: Sabrina Dubroca  <sd@queasysnail.net>
---
 drivers/net/netdevsim/netdev.c    | 273 +++++++++++++++++++++++++++++++++++++-
 drivers/net/netdevsim/netdevsim.h |  12 ++
 2 files changed, 283 insertions(+), 2 deletions(-)

Comments

Phil Sutter Dec. 1, 2017, 1:43 p.m. UTC | #1 
On Thu, Nov 30, 2017 at 05:35:39PM -0800, Jakub Kicinski wrote:
[...]
> +static int nsim_vfs_enable(struct netdevsim *ns, unsigned int num_vfs)
> +{
> +	ns->vfconfigs = kcalloc(num_vfs, sizeof(struct nsim_vf_config),
> +				GFP_KERNEL);
> +	if (!ns->vfconfigs)
> +		return -ENOMEM;
> +	ns->num_vfs = num_vfs;
> +
> +	return 0;
> +}
> +
> +static void nsim_vfs_disable(struct netdevsim *ns)
> +{
> +	kfree(ns->vfconfigs);
> +	ns->vfconfigs = NULL;
> +	ns->num_vfs = 0;
> +}

Why not something like:

| static int nsim_vfs_set(struct netdevsim *ns, unsigned int num_vfs)
| {
| 	void *ptr = krealloc(ns->vfconfigs,
| 			     num_vfs * sizeof(struct nsim_vf_config),
| 			     GFP_KERNEL);
| 
| 	if (!ptr)
| 		return -ENOMEM;
| 
| 	ns->vfconfigs = ptr;
| 	ns->num_vfs = num_vfs;
| 	return 0;
| }

> +static ssize_t
> +nsim_numvfs_store(struct device *dev, struct device_attribute *attr,
> +		  const char *buf, size_t count)
> +{
> +	struct netdevsim *ns = to_nsim(dev);
> +	unsigned int num_vfs;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 0, &num_vfs);
> +	if (ret)
> +		return ret;
> +
> +	rtnl_lock();
> +	if (ns->num_vfs == num_vfs)
> +		goto exit_good;

Then replace this:

> +	if (ns->num_vfs && num_vfs) {
> +		ret = -EBUSY;
> +		goto exit_unlock;
> +	}
> +
> +	if (num_vfs) {
> +		ret = nsim_vfs_enable(ns, num_vfs);
> +		if (ret)
> +			goto exit_unlock;
> +	} else {
> +		nsim_vfs_disable(ns);
> +	}

with just:

|	nsim_vfs_set(ns, num_vfs);

> +exit_good:
> +	ret = count;
> +exit_unlock:
> +	rtnl_unlock();
> +
> +	return ret;
> +}

[...]

> +static void nsim_free(struct net_device *dev)
> +{
> +	struct netdevsim *ns = netdev_priv(dev);
> +
> +	device_unregister(&ns->dev);
>  }

Shouldn't this also kfree(ns->vfconfigs)?

Cheers, Phil
Jakub Kicinski Dec. 1, 2017, 8:14 p.m. UTC | #2 
On Fri, 1 Dec 2017 14:43:06 +0100, Phil Sutter wrote:
> On Thu, Nov 30, 2017 at 05:35:39PM -0800, Jakub Kicinski wrote:
> [...]
> > +static int nsim_vfs_enable(struct netdevsim *ns, unsigned int num_vfs)
> > +{
> > +	ns->vfconfigs = kcalloc(num_vfs, sizeof(struct nsim_vf_config),
> > +				GFP_KERNEL);
> > +	if (!ns->vfconfigs)
> > +		return -ENOMEM;
> > +	ns->num_vfs = num_vfs;
> > +
> > +	return 0;
> > +}
> > +
> > +static void nsim_vfs_disable(struct netdevsim *ns)
> > +{
> > +	kfree(ns->vfconfigs);
> > +	ns->vfconfigs = NULL;
> > +	ns->num_vfs = 0;
> > +}  
> 
> Why not something like:
> 
> | static int nsim_vfs_set(struct netdevsim *ns, unsigned int num_vfs)
> | {
> | 	void *ptr = krealloc(ns->vfconfigs,
> | 			     num_vfs * sizeof(struct nsim_vf_config),
> | 			     GFP_KERNEL);
> | 
> | 	if (!ptr)
> | 		return -ENOMEM;
> | 
> | 	ns->vfconfigs = ptr;
> | 	ns->num_vfs = num_vfs;
> | 	return 0;
> | }

Um.  It either frees or allocates, never reallocates so I felt realloc
is misleading.  ZERO_SIZE_PTR is less clearly a NULL than a NULL.  I
will have to specify __GFP_ZERO.  It's not a calloc so there could be
potentially some overflows?

> > +static ssize_t
> > +nsim_numvfs_store(struct device *dev, struct device_attribute *attr,
> > +		  const char *buf, size_t count)
> > +{
> > +	struct netdevsim *ns = to_nsim(dev);
> > +	unsigned int num_vfs;
> > +	int ret;
> > +
> > +	ret = kstrtouint(buf, 0, &num_vfs);
> > +	if (ret)
> > +		return ret;
> > +
> > +	rtnl_lock();
> > +	if (ns->num_vfs == num_vfs)
> > +		goto exit_good;  
> 
> Then replace this:
> 
> > +	if (ns->num_vfs && num_vfs) {
> > +		ret = -EBUSY;
> > +		goto exit_unlock;
> > +	}
> > +
> > +	if (num_vfs) {
> > +		ret = nsim_vfs_enable(ns, num_vfs);
> > +		if (ret)
> > +			goto exit_unlock;
> > +	} else {
> > +		nsim_vfs_disable(ns);
> > +	}  
> 
> with just:
> 
> |	nsim_vfs_set(ns, num_vfs);

I'm trying to mirror the PCI subsystem behaviour here, which only
allows enable or disable, not increase.  I felt we should follow how
real devices behave:

	/* enable VFs */
	if (pdev->sriov->num_VFs) {
		dev_warn(&pdev->dev, "%d VFs already enabled. Disable before enabling %d VFs\n",
			 pdev->sriov->num_VFs, num_vfs);
		return -EBUSY;
	}

So IOW this is intentional.

> > +	ret = count;
> > +exit_unlock:
> > +	rtnl_unlock();
> > +
> > +	return ret;
> > +}  
> 
> [...]
> 
> > +static void nsim_free(struct net_device *dev)
> > +{
> > +	struct netdevsim *ns = netdev_priv(dev);
> > +
> > +	device_unregister(&ns->dev);
> >  }  
> 
> Shouldn't this also kfree(ns->vfconfigs)?

It's in uninit, I will move it to release.
Phil Sutter Dec. 1, 2017, 9:36 p.m. UTC | #3 
On Fri, Dec 01, 2017 at 12:14:07PM -0800, Jakub Kicinski wrote:
> On Fri, 1 Dec 2017 14:43:06 +0100, Phil Sutter wrote:
> > On Thu, Nov 30, 2017 at 05:35:39PM -0800, Jakub Kicinski wrote:
> > [...]
> > > +static int nsim_vfs_enable(struct netdevsim *ns, unsigned int num_vfs)
> > > +{
> > > +	ns->vfconfigs = kcalloc(num_vfs, sizeof(struct nsim_vf_config),
> > > +				GFP_KERNEL);
> > > +	if (!ns->vfconfigs)
> > > +		return -ENOMEM;
> > > +	ns->num_vfs = num_vfs;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void nsim_vfs_disable(struct netdevsim *ns)
> > > +{
> > > +	kfree(ns->vfconfigs);
> > > +	ns->vfconfigs = NULL;
> > > +	ns->num_vfs = 0;
> > > +}  
> > 
> > Why not something like:
> > 
> > | static int nsim_vfs_set(struct netdevsim *ns, unsigned int num_vfs)
> > | {
> > | 	void *ptr = krealloc(ns->vfconfigs,
> > | 			     num_vfs * sizeof(struct nsim_vf_config),
> > | 			     GFP_KERNEL);
> > | 
> > | 	if (!ptr)
> > | 		return -ENOMEM;
> > | 
> > | 	ns->vfconfigs = ptr;
> > | 	ns->num_vfs = num_vfs;
> > | 	return 0;
> > | }
> 
> Um.  It either frees or allocates, never reallocates so I felt realloc
> is misleading.  ZERO_SIZE_PTR is less clearly a NULL than a NULL.  I
> will have to specify __GFP_ZERO.  It's not a calloc so there could be
> potentially some overflows?

I don't understand: How can overflows happen if I use malloc() instead
of calloc()?

> > > +static ssize_t
> > > +nsim_numvfs_store(struct device *dev, struct device_attribute *attr,
> > > +		  const char *buf, size_t count)
> > > +{
> > > +	struct netdevsim *ns = to_nsim(dev);
> > > +	unsigned int num_vfs;
> > > +	int ret;
> > > +
> > > +	ret = kstrtouint(buf, 0, &num_vfs);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	rtnl_lock();
> > > +	if (ns->num_vfs == num_vfs)
> > > +		goto exit_good;  
> > 
> > Then replace this:
> > 
> > > +	if (ns->num_vfs && num_vfs) {
> > > +		ret = -EBUSY;
> > > +		goto exit_unlock;
> > > +	}
> > > +
> > > +	if (num_vfs) {
> > > +		ret = nsim_vfs_enable(ns, num_vfs);
> > > +		if (ret)
> > > +			goto exit_unlock;
> > > +	} else {
> > > +		nsim_vfs_disable(ns);
> > > +	}  
> > 
> > with just:
> > 
> > |	nsim_vfs_set(ns, num_vfs);
> 
> I'm trying to mirror the PCI subsystem behaviour here, which only
> allows enable or disable, not increase.  I felt we should follow how
> real devices behave:
> 
> 	/* enable VFs */
> 	if (pdev->sriov->num_VFs) {
> 		dev_warn(&pdev->dev, "%d VFs already enabled. Disable before enabling %d VFs\n",
> 			 pdev->sriov->num_VFs, num_vfs);
> 		return -EBUSY;
> 	}
> 
> So IOW this is intentional.

Ah, I see. Yes, then it makes sense! Keeping this virtual VF
functionality as close to real ones as possible is certainly feasible.

> > > +	ret = count;
> > > +exit_unlock:
> > > +	rtnl_unlock();
> > > +
> > > +	return ret;
> > > +}  
> > 
> > [...]
> > 
> > > +static void nsim_free(struct net_device *dev)
> > > +{
> > > +	struct netdevsim *ns = netdev_priv(dev);
> > > +
> > > +	device_unregister(&ns->dev);
> > >  }  
> > 
> > Shouldn't this also kfree(ns->vfconfigs)?
> 
> It's in uninit, I will move it to release.

Oh, I missed that. If you're certain this won't lead to memleaks, no
objection from my side. :)

Cheers, Phil
Jakub Kicinski Dec. 1, 2017, 9:45 p.m. UTC | #4 
On Fri, 1 Dec 2017 22:36:52 +0100, Phil Sutter wrote:
> On Fri, Dec 01, 2017 at 12:14:07PM -0800, Jakub Kicinski wrote:
> > On Fri, 1 Dec 2017 14:43:06 +0100, Phil Sutter wrote:  
> > > On Thu, Nov 30, 2017 at 05:35:39PM -0800, Jakub Kicinski wrote:
> > > [...]  
> > > > +static int nsim_vfs_enable(struct netdevsim *ns, unsigned int num_vfs)
> > > > +{
> > > > +	ns->vfconfigs = kcalloc(num_vfs, sizeof(struct nsim_vf_config),
> > > > +				GFP_KERNEL);
> > > > +	if (!ns->vfconfigs)
> > > > +		return -ENOMEM;
> > > > +	ns->num_vfs = num_vfs;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void nsim_vfs_disable(struct netdevsim *ns)
> > > > +{
> > > > +	kfree(ns->vfconfigs);
> > > > +	ns->vfconfigs = NULL;
> > > > +	ns->num_vfs = 0;
> > > > +}    
> > > 
> > > Why not something like:
> > > 
> > > | static int nsim_vfs_set(struct netdevsim *ns, unsigned int num_vfs)
> > > | {
> > > | 	void *ptr = krealloc(ns->vfconfigs,
> > > | 			     num_vfs * sizeof(struct nsim_vf_config),
> > > | 			     GFP_KERNEL);
> > > | 
> > > | 	if (!ptr)
> > > | 		return -ENOMEM;
> > > | 
> > > | 	ns->vfconfigs = ptr;
> > > | 	ns->num_vfs = num_vfs;
> > > | 	return 0;
> > > | }  
> > 
> > Um.  It either frees or allocates, never reallocates so I felt realloc
> > is misleading.  ZERO_SIZE_PTR is less clearly a NULL than a NULL.  I
> > will have to specify __GFP_ZERO.  It's not a calloc so there could be
> > potentially some overflows?  
> 
> I don't understand: How can overflows happen if I use malloc() instead
> of calloc()?

The multiplication may overflow.  That's why we have kmalloc_array().
Note this explicit check in kmalloc_array() (which is also called by
kcalloc):

	if (size != 0 && n > SIZE_MAX / size)
		return NULL;

Where:

#define SIZE_MAX	(~(size_t)0)

> > > > +	ret = count;
> > > > +exit_unlock:
> > > > +	rtnl_unlock();
> > > > +
> > > > +	return ret;
> > > > +}    
> > > 
> > > [...]
> > >   
> > > > +static void nsim_free(struct net_device *dev)
> > > > +{
> > > > +	struct netdevsim *ns = netdev_priv(dev);
> > > > +
> > > > +	device_unregister(&ns->dev);
> > > >  }    
> > > 
> > > Shouldn't this also kfree(ns->vfconfigs)?  
> > 
> > It's in uninit, I will move it to release.  
> 
> Oh, I missed that. If you're certain this won't lead to memleaks, no
> objection from my side. :)

OK, I will respin v3 with the free moved :)
Phil Sutter Dec. 1, 2017, 9:58 p.m. UTC | #5 
On Fri, Dec 01, 2017 at 01:45:09PM -0800, Jakub Kicinski wrote:
> On Fri, 1 Dec 2017 22:36:52 +0100, Phil Sutter wrote:
> > On Fri, Dec 01, 2017 at 12:14:07PM -0800, Jakub Kicinski wrote:
> > > On Fri, 1 Dec 2017 14:43:06 +0100, Phil Sutter wrote:  
> > > > On Thu, Nov 30, 2017 at 05:35:39PM -0800, Jakub Kicinski wrote:
> > > > [...]  
> > > > > +static int nsim_vfs_enable(struct netdevsim *ns, unsigned int num_vfs)
> > > > > +{
> > > > > +	ns->vfconfigs = kcalloc(num_vfs, sizeof(struct nsim_vf_config),
> > > > > +				GFP_KERNEL);
> > > > > +	if (!ns->vfconfigs)
> > > > > +		return -ENOMEM;
> > > > > +	ns->num_vfs = num_vfs;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static void nsim_vfs_disable(struct netdevsim *ns)
> > > > > +{
> > > > > +	kfree(ns->vfconfigs);
> > > > > +	ns->vfconfigs = NULL;
> > > > > +	ns->num_vfs = 0;
> > > > > +}    
> > > > 
> > > > Why not something like:
> > > > 
> > > > | static int nsim_vfs_set(struct netdevsim *ns, unsigned int num_vfs)
> > > > | {
> > > > | 	void *ptr = krealloc(ns->vfconfigs,
> > > > | 			     num_vfs * sizeof(struct nsim_vf_config),
> > > > | 			     GFP_KERNEL);
> > > > | 
> > > > | 	if (!ptr)
> > > > | 		return -ENOMEM;
> > > > | 
> > > > | 	ns->vfconfigs = ptr;
> > > > | 	ns->num_vfs = num_vfs;
> > > > | 	return 0;
> > > > | }  
> > > 
> > > Um.  It either frees or allocates, never reallocates so I felt realloc
> > > is misleading.  ZERO_SIZE_PTR is less clearly a NULL than a NULL.  I
> > > will have to specify __GFP_ZERO.  It's not a calloc so there could be
> > > potentially some overflows?  
> > 
> > I don't understand: How can overflows happen if I use malloc() instead
> > of calloc()?
> 
> The multiplication may overflow.  That's why we have kmalloc_array().
> Note this explicit check in kmalloc_array() (which is also called by
> kcalloc):
> 
> 	if (size != 0 && n > SIZE_MAX / size)
> 		return NULL;
> 
> Where:
> 
> #define SIZE_MAX	(~(size_t)0)

Ah, I see. Thanks for educating me on this!

> > > > > +	ret = count;
> > > > > +exit_unlock:
> > > > > +	rtnl_unlock();
> > > > > +
> > > > > +	return ret;
> > > > > +}    
> > > > 
> > > > [...]
> > > >   
> > > > > +static void nsim_free(struct net_device *dev)
> > > > > +{
> > > > > +	struct netdevsim *ns = netdev_priv(dev);
> > > > > +
> > > > > +	device_unregister(&ns->dev);
> > > > >  }    
> > > > 
> > > > Shouldn't this also kfree(ns->vfconfigs)?  
> > > 
> > > It's in uninit, I will move it to release.  
> > 
> > Oh, I missed that. If you're certain this won't lead to memleaks, no
> > objection from my side. :)
> 
> OK, I will respin v3 with the free moved :)

So it did leak? I'm glad the traffic I caused wasn't completely
pointless then. :)

Thanks, Phil
Jakub Kicinski Dec. 1, 2017, 10:14 p.m. UTC | #6 
On Fri, 1 Dec 2017 22:58:29 +0100, Phil Sutter wrote:
> > > > > > +	ret = count;
> > > > > > +exit_unlock:
> > > > > > +	rtnl_unlock();
> > > > > > +
> > > > > > +	return ret;
> > > > > > +}      
> > > > > 
> > > > > [...]
> > > > >     
> > > > > > +static void nsim_free(struct net_device *dev)
> > > > > > +{
> > > > > > +	struct netdevsim *ns = netdev_priv(dev);
> > > > > > +
> > > > > > +	device_unregister(&ns->dev);
> > > > > >  }      
> > > > > 
> > > > > Shouldn't this also kfree(ns->vfconfigs)?    
> > > > 
> > > > It's in uninit, I will move it to release.    
> > > 
> > > Oh, I missed that. If you're certain this won't lead to memleaks, no
> > > objection from my side. :)  
> > 
> > OK, I will respin v3 with the free moved :)  
> 
> So it did leak? I'm glad the traffic I caused wasn't completely
> pointless then. :)

There is a window where it could've been re-enabled and that
would leak, yes.  Thanks for catching it :)
diff mbox series

Patch

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 828c1ce49a8b..a7a2a2290957 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -25,6 +25,124 @@ 
 
 #include "netdevsim.h"
 
+struct nsim_vf_config {
+	int link_state;
+	u16 min_tx_rate;
+	u16 max_tx_rate;
+	u16 vlan;
+	__be16 vlan_proto;
+	u16 qos;
+	u8 vf_mac[ETH_ALEN];
+	bool spoofchk_enabled;
+	bool trusted;
+	bool rss_query_enabled;
+};
+
+static u32 nsim_dev_id;
+
+static int nsim_num_vf(struct device *dev)
+{
+	struct netdevsim *ns = to_nsim(dev);
+
+	return ns->num_vfs;
+}
+
+static struct bus_type nsim_bus = {
+	.name		= DRV_NAME,
+	.dev_name	= DRV_NAME,
+	.num_vf		= nsim_num_vf,
+};
+
+static int nsim_vfs_enable(struct netdevsim *ns, unsigned int num_vfs)
+{
+	ns->vfconfigs = kcalloc(num_vfs, sizeof(struct nsim_vf_config),
+				GFP_KERNEL);
+	if (!ns->vfconfigs)
+		return -ENOMEM;
+	ns->num_vfs = num_vfs;
+
+	return 0;
+}
+
+static void nsim_vfs_disable(struct netdevsim *ns)
+{
+	kfree(ns->vfconfigs);
+	ns->vfconfigs = NULL;
+	ns->num_vfs = 0;
+}
+
+static ssize_t
+nsim_numvfs_store(struct device *dev, struct device_attribute *attr,
+		  const char *buf, size_t count)
+{
+	struct netdevsim *ns = to_nsim(dev);
+	unsigned int num_vfs;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &num_vfs);
+	if (ret)
+		return ret;
+
+	rtnl_lock();
+	if (ns->num_vfs == num_vfs)
+		goto exit_good;
+	if (ns->num_vfs && num_vfs) {
+		ret = -EBUSY;
+		goto exit_unlock;
+	}
+
+	if (num_vfs) {
+		ret = nsim_vfs_enable(ns, num_vfs);
+		if (ret)
+			goto exit_unlock;
+	} else {
+		nsim_vfs_disable(ns);
+	}
+exit_good:
+	ret = count;
+exit_unlock:
+	rtnl_unlock();
+
+	return ret;
+}
+
+static ssize_t
+nsim_numvfs_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct netdevsim *ns = to_nsim(dev);
+
+	return sprintf(buf, "%u\n", ns->num_vfs);
+}
+
+static struct device_attribute nsim_numvfs_attr =
+	__ATTR(sriov_numvfs, 0664, nsim_numvfs_show, nsim_numvfs_store);
+
+static struct attribute *nsim_dev_attrs[] = {
+	&nsim_numvfs_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group nsim_dev_attr_group = {
+	.attrs = nsim_dev_attrs,
+};
+
+static const struct attribute_group *nsim_dev_attr_groups[] = {
+	&nsim_dev_attr_group,
+	NULL,
+};
+
+static void nsim_dev_release(struct device *dev)
+{
+	struct netdevsim *ns = to_nsim(dev);
+
+	free_netdev(ns->netdev);
+}
+
+struct device_type nsim_dev_type = {
+	.groups = nsim_dev_attr_groups,
+	.release = nsim_dev_release,
+};
+
 static int nsim_init(struct net_device *dev)
 {
 	struct netdevsim *ns = netdev_priv(dev);
@@ -37,8 +155,19 @@  static int nsim_init(struct net_device *dev)
 	if (err)
 		goto err_debugfs_destroy;
 
+	ns->dev.id = nsim_dev_id++;
+	ns->dev.bus = &nsim_bus;
+	ns->dev.type = &nsim_dev_type;
+	err = device_register(&ns->dev);
+	if (err)
+		goto err_bpf_uninit;
+
+	SET_NETDEV_DEV(dev, &ns->dev);
+
 	return 0;
 
+err_bpf_uninit:
+	nsim_bpf_uninit(ns);
 err_debugfs_destroy:
 	debugfs_remove_recursive(ns->ddir);
 	return err;
@@ -50,6 +179,14 @@  static void nsim_uninit(struct net_device *dev)
 
 	debugfs_remove_recursive(ns->ddir);
 	nsim_bpf_uninit(ns);
+	nsim_vfs_disable(ns);
+}
+
+static void nsim_free(struct net_device *dev)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+
+	device_unregister(&ns->dev);
 }
 
 static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
@@ -122,6 +259,123 @@  nsim_setup_tc_block(struct net_device *dev, struct tc_block_offload *f)
 	}
 }
 
+static int nsim_set_vf_mac(struct net_device *dev, int vf, u8 *mac)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+
+	/* Only refuse multicast addresses, zero address can mean unset/any. */
+	if (vf >= ns->num_vfs || is_multicast_ether_addr(mac))
+		return -EINVAL;
+	memcpy(ns->vfconfigs[vf].vf_mac, mac, ETH_ALEN);
+
+	return 0;
+}
+
+static int nsim_set_vf_vlan(struct net_device *dev, int vf,
+			    u16 vlan, u8 qos, __be16 vlan_proto)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+
+	if (vf >= ns->num_vfs || vlan > 4095 || qos > 7)
+		return -EINVAL;
+
+	ns->vfconfigs[vf].vlan = vlan;
+	ns->vfconfigs[vf].qos = qos;
+	ns->vfconfigs[vf].vlan_proto = vlan_proto;
+
+	return 0;
+}
+
+static int nsim_set_vf_rate(struct net_device *dev, int vf, int min, int max)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+
+	if (vf >= ns->num_vfs)
+		return -EINVAL;
+
+	ns->vfconfigs[vf].min_tx_rate = min;
+	ns->vfconfigs[vf].max_tx_rate = max;
+
+	return 0;
+}
+
+static int nsim_set_vf_spoofchk(struct net_device *dev, int vf, bool val)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+
+	if (vf >= ns->num_vfs)
+		return -EINVAL;
+	ns->vfconfigs[vf].spoofchk_enabled = val;
+
+	return 0;
+}
+
+static int nsim_set_vf_rss_query_en(struct net_device *dev, int vf, bool val)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+
+	if (vf >= ns->num_vfs)
+		return -EINVAL;
+	ns->vfconfigs[vf].rss_query_enabled = val;
+
+	return 0;
+}
+
+static int nsim_set_vf_trust(struct net_device *dev, int vf, bool val)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+
+	if (vf >= ns->num_vfs)
+		return -EINVAL;
+	ns->vfconfigs[vf].trusted = val;
+
+	return 0;
+}
+
+static int
+nsim_get_vf_config(struct net_device *dev, int vf, struct ifla_vf_info *ivi)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+
+	if (vf >= ns->num_vfs)
+		return -EINVAL;
+
+	ivi->vf = vf;
+	ivi->linkstate = ns->vfconfigs[vf].link_state;
+	ivi->min_tx_rate = ns->vfconfigs[vf].min_tx_rate;
+	ivi->max_tx_rate = ns->vfconfigs[vf].max_tx_rate;
+	ivi->vlan = ns->vfconfigs[vf].vlan;
+	ivi->vlan_proto = ns->vfconfigs[vf].vlan_proto;
+	ivi->qos = ns->vfconfigs[vf].qos;
+	memcpy(&ivi->mac, ns->vfconfigs[vf].vf_mac, ETH_ALEN);
+	ivi->spoofchk = ns->vfconfigs[vf].spoofchk_enabled;
+	ivi->trusted = ns->vfconfigs[vf].trusted;
+	ivi->rss_query_en = ns->vfconfigs[vf].rss_query_enabled;
+
+	return 0;
+}
+
+static int nsim_set_vf_link_state(struct net_device *dev, int vf, int state)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+
+	if (vf >= ns->num_vfs)
+		return -EINVAL;
+
+	switch (state) {
+	case IFLA_VF_LINK_STATE_AUTO:
+	case IFLA_VF_LINK_STATE_ENABLE:
+	case IFLA_VF_LINK_STATE_DISABLE:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ns->vfconfigs[vf].link_state = state;
+
+	return 0;
+}
+
 static int
 nsim_setup_tc(struct net_device *dev, enum tc_setup_type type, void *type_data)
 {
@@ -153,6 +407,14 @@  static const struct net_device_ops nsim_netdev_ops = {
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_change_mtu		= nsim_change_mtu,
 	.ndo_get_stats64	= nsim_get_stats64,
+	.ndo_set_vf_mac		= nsim_set_vf_mac,
+	.ndo_set_vf_vlan	= nsim_set_vf_vlan,
+	.ndo_set_vf_rate	= nsim_set_vf_rate,
+	.ndo_set_vf_spoofchk	= nsim_set_vf_spoofchk,
+	.ndo_set_vf_trust	= nsim_set_vf_trust,
+	.ndo_get_vf_config	= nsim_get_vf_config,
+	.ndo_set_vf_link_state	= nsim_set_vf_link_state,
+	.ndo_set_vf_rss_query_en = nsim_set_vf_rss_query_en,
 	.ndo_setup_tc		= nsim_setup_tc,
 	.ndo_set_features	= nsim_set_features,
 	.ndo_bpf		= nsim_bpf,
@@ -164,7 +426,7 @@  static void nsim_setup(struct net_device *dev)
 	eth_hw_addr_random(dev);
 
 	dev->netdev_ops = &nsim_netdev_ops;
-	dev->needs_free_netdev = true;
+	dev->priv_destructor = nsim_free;
 
 	dev->tx_queue_len = 0;
 	dev->flags |= IFF_NOARP;
@@ -209,12 +471,18 @@  static int __init nsim_module_init(void)
 	if (IS_ERR(nsim_ddir))
 		return PTR_ERR(nsim_ddir);
 
-	err = rtnl_link_register(&nsim_link_ops);
+	err = bus_register(&nsim_bus);
 	if (err)
 		goto err_debugfs_destroy;
 
+	err = rtnl_link_register(&nsim_link_ops);
+	if (err)
+		goto err_unreg_bus;
+
 	return 0;
 
+err_unreg_bus:
+	bus_unregister(&nsim_bus);
 err_debugfs_destroy:
 	debugfs_remove_recursive(nsim_ddir);
 	return err;
@@ -223,6 +491,7 @@  static int __init nsim_module_init(void)
 static void __exit nsim_module_exit(void)
 {
 	rtnl_link_unregister(&nsim_link_ops);
+	bus_unregister(&nsim_bus);
 	debugfs_remove_recursive(nsim_ddir);
 }
 
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 8779e6a8f885..32270de9395a 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -13,6 +13,7 @@ 
  * THE COST OF ALL NECESSARY SERVICING, REPAIR OR CORRECTION.
  */
 
+#include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/netdevice.h>
@@ -26,6 +27,7 @@ 
 
 struct bpf_prog;
 struct dentry;
+struct nsim_vf_config;
 
 struct netdevsim {
 	struct net_device *netdev;
@@ -34,8 +36,13 @@  struct netdevsim {
 	u64 tx_bytes;
 	struct u64_stats_sync syncp;
 
+	struct device dev;
+
 	struct dentry *ddir;
 
+	unsigned int num_vfs;
+	struct nsim_vf_config *vfconfigs;
+
 	struct bpf_prog	*bpf_offloaded;
 	u32 bpf_offloaded_id;
 
@@ -64,3 +71,8 @@  int nsim_bpf(struct net_device *dev, struct netdev_bpf *bpf);
 int nsim_bpf_disable_tc(struct netdevsim *ns);
 int nsim_bpf_setup_tc_block_cb(enum tc_setup_type type,
 			       void *type_data, void *cb_priv);
+
+static inline struct netdevsim *to_nsim(struct device *ptr)
+{
+	return container_of(ptr, struct netdevsim, dev);
+}