[1/5] bus/vmbus: add devargs support

Message ID 20180830223512.21297-2-stephen@networkplumber.org
State Accepted, archived
Delegated to: Ferruh Yigit
Headers show
Series
  • netvsc changes for 18.11
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Stephen Hemminger Aug. 30, 2018, 10:35 p.m.
From: Stephen Hemminger <sthemmin@microsoft.com>

Take device arguments from command line and put
them in the device devargs.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/bus/vmbus/linux/vmbus_bus.c |  2 ++
 drivers/bus/vmbus/private.h         |  3 +++
 drivers/bus/vmbus/vmbus_common.c    | 22 +++++++++++++++++++++-
 3 files changed, 26 insertions(+), 1 deletion(-)

Comments

Ferruh Yigit Sept. 14, 2018, 12:46 p.m. | #1
On 8/30/2018 11:35 PM, Stephen Hemminger wrote:
> From: Stephen Hemminger <sthemmin@microsoft.com>
> 
> Take device arguments from command line and put
> them in the device devargs.
> 
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>

<...>

> @@ -204,6 +203,27 @@ vmbus_parse(const char *name, void *addr)
>  	return ret;
>  }
>  
> +/*
> + * scan for matching device args on command line
> + * example:
> + *	-w 'vmbus(635a7ae3-091e-4410-ad59-667c4f8c04c3,latency=20)'

This is just in comment but,

I guess latest syntax is:
 -w "vmbus:635a7ae3-091e-4410-ad59-667c4f8c04c3,latency=20"

@Gaetan, is latest devarg syntax documented somewhere?
Gaëtan Rivet Sept. 14, 2018, 1:06 p.m. | #2
Hi,

On Fri, Sep 14, 2018 at 01:46:59PM +0100, Ferruh Yigit wrote:
> On 8/30/2018 11:35 PM, Stephen Hemminger wrote:
> > From: Stephen Hemminger <sthemmin@microsoft.com>
> > 
> > Take device arguments from command line and put
> > them in the device devargs.
> > 
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> 
> <...>
> 
> > @@ -204,6 +203,27 @@ vmbus_parse(const char *name, void *addr)
> >  	return ret;
> >  }
> >  
> > +/*
> > + * scan for matching device args on command line
> > + * example:
> > + *	-w 'vmbus(635a7ae3-091e-4410-ad59-667c4f8c04c3,latency=20)'
> 
> This is just in comment but,
> 
> I guess latest syntax is:
>  -w "vmbus:635a7ae3-091e-4410-ad59-667c4f8c04c3,latency=20"
> 
> @Gaetan, is latest devarg syntax documented somewhere?

That's the current syntax indeed. Some documentation is found at

lib/librte_eal/common/include/rte_devargs.h:100

Where it is specified that the bus name can be either omitted or
followed by any character, to separate it from the device identifier.

This means that using ':' is fine, as well as '('. As long as the device
PMD afterward ignore the dangling ')' during devargs parsing, this should
be fine.

I don't think this is very clean, but it works.
Ferruh Yigit Sept. 14, 2018, 1:19 p.m. | #3
On 9/14/2018 2:06 PM, Gaëtan Rivet wrote:
> Hi,
> 
> On Fri, Sep 14, 2018 at 01:46:59PM +0100, Ferruh Yigit wrote:
>> On 8/30/2018 11:35 PM, Stephen Hemminger wrote:
>>> From: Stephen Hemminger <sthemmin@microsoft.com>
>>>
>>> Take device arguments from command line and put
>>> them in the device devargs.
>>>
>>> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
>>
>> <...>
>>
>>> @@ -204,6 +203,27 @@ vmbus_parse(const char *name, void *addr)
>>>  	return ret;
>>>  }
>>>  
>>> +/*
>>> + * scan for matching device args on command line
>>> + * example:
>>> + *	-w 'vmbus(635a7ae3-091e-4410-ad59-667c4f8c04c3,latency=20)'
>>
>> This is just in comment but,
>>
>> I guess latest syntax is:
>>  -w "vmbus:635a7ae3-091e-4410-ad59-667c4f8c04c3,latency=20"
>>
>> @Gaetan, is latest devarg syntax documented somewhere?
> 
> That's the current syntax indeed. Some documentation is found at
> 
> lib/librte_eal/common/include/rte_devargs.h:100
> 
> Where it is specified that the bus name can be either omitted or
> followed by any character, to separate it from the device identifier.
> 
> This means that using ':' is fine, as well as '('. As long as the device
> PMD afterward ignore the dangling ')' during devargs parsing, this should
> be fine.
> 
> I don't think this is very clean, but it works.

Thanks for the info, I see how "(" works, but ")" is takes as part of argument
and causing problem, I think better to not give "()" as supported syntax at all.

btw, now both -w and --vdev are valid and can be used interchangeably, right? I
mean all following are valid?
-w pci:0000:86:06.0,enable_floating_veb=1
-w vdev:net_pcap,iface=lo
--vdev pci:0000:86:06.0,enable_floating_veb=1
--vdev vdev:net_pcap,iface=lo
Gaëtan Rivet Sept. 14, 2018, 1:58 p.m. | #4
On Fri, Sep 14, 2018 at 02:19:19PM +0100, Ferruh Yigit wrote:
> On 9/14/2018 2:06 PM, Gaëtan Rivet wrote:
> > Hi,
> > 
> > On Fri, Sep 14, 2018 at 01:46:59PM +0100, Ferruh Yigit wrote:
> >> On 8/30/2018 11:35 PM, Stephen Hemminger wrote:
> >>> From: Stephen Hemminger <sthemmin@microsoft.com>
> >>>
> >>> Take device arguments from command line and put
> >>> them in the device devargs.
> >>>
> >>> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> >>
> >> <...>
> >>
> >>> @@ -204,6 +203,27 @@ vmbus_parse(const char *name, void *addr)
> >>>  	return ret;
> >>>  }
> >>>  
> >>> +/*
> >>> + * scan for matching device args on command line
> >>> + * example:
> >>> + *	-w 'vmbus(635a7ae3-091e-4410-ad59-667c4f8c04c3,latency=20)'
> >>
> >> This is just in comment but,
> >>
> >> I guess latest syntax is:
> >>  -w "vmbus:635a7ae3-091e-4410-ad59-667c4f8c04c3,latency=20"
> >>
> >> @Gaetan, is latest devarg syntax documented somewhere?
> > 
> > That's the current syntax indeed. Some documentation is found at
> > 
> > lib/librte_eal/common/include/rte_devargs.h:100
> > 
> > Where it is specified that the bus name can be either omitted or
> > followed by any character, to separate it from the device identifier.
> > 
> > This means that using ':' is fine, as well as '('. As long as the device
> > PMD afterward ignore the dangling ')' during devargs parsing, this should
> > be fine.
> > 
> > I don't think this is very clean, but it works.
> 
> Thanks for the info, I see how "(" works, but ")" is takes as part of argument
> and causing problem, I think better to not give "()" as supported syntax at all.
> 
> btw, now both -w and --vdev are valid and can be used interchangeably, right? I
> mean all following are valid?
> -w pci:0000:86:06.0,enable_floating_veb=1
> -w vdev:net_pcap,iface=lo
> --vdev pci:0000:86:06.0,enable_floating_veb=1
> --vdev vdev:net_pcap,iface=lo
> 
> 

They are both valid but cannot be used interchangeably.

This was the case at one point, between two rcs, because I had removed
the devtype and the bus black/white-listing was configured another way.

A user complained about the API change because it was not announced in
time IIRC, and this was scrapped. Since then, the effort has been to on
the new syntax instead of cleaning the old system.

They cannot be used interchangeably because an rte_devtype is defined by
the parameter choosen. BLACKLISTED_PCI for -b, WHITELISTED_PCI for -w,
and VIRTUAL for --vdev.

-b will actually change the bus configuration to "blacklist mode", -w to
"whitelist mode", and --vdev will do nothing.

So a PCI device declared using --vdev, will actually be skipped during
PCI probe because by default it operates in blacklist mode, and the
device policy is not WHITELISTED. This could surprise the one attempting
this trick.

This semantic is subtle and bug-prone. I'm adding it to the pile of
reasons I'd like to remove the blacklist mode altogether.
Stephen Hemminger Sept. 14, 2018, 3:21 p.m. | #5
On Fri, 14 Sep 2018 15:58:04 +0200
Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:

> On Fri, Sep 14, 2018 at 02:19:19PM +0100, Ferruh Yigit wrote:
> > On 9/14/2018 2:06 PM, Gaëtan Rivet wrote:  
> > > Hi,
> > > 
> > > On Fri, Sep 14, 2018 at 01:46:59PM +0100, Ferruh Yigit wrote:  
> > >> On 8/30/2018 11:35 PM, Stephen Hemminger wrote:  
> > >>> From: Stephen Hemminger <sthemmin@microsoft.com>
> > >>>
> > >>> Take device arguments from command line and put
> > >>> them in the device devargs.
> > >>>
> > >>> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>  
> > >>
> > >> <...>
> > >>  
> > >>> @@ -204,6 +203,27 @@ vmbus_parse(const char *name, void *addr)
> > >>>  	return ret;
> > >>>  }
> > >>>  
> > >>> +/*
> > >>> + * scan for matching device args on command line
> > >>> + * example:
> > >>> + *	-w 'vmbus(635a7ae3-091e-4410-ad59-667c4f8c04c3,latency=20)'  
> > >>
> > >> This is just in comment but,
> > >>
> > >> I guess latest syntax is:
> > >>  -w "vmbus:635a7ae3-091e-4410-ad59-667c4f8c04c3,latency=20"
> > >>
> > >> @Gaetan, is latest devarg syntax documented somewhere?  
> > > 
> > > That's the current syntax indeed. Some documentation is found at
> > > 
> > > lib/librte_eal/common/include/rte_devargs.h:100
> > > 
> > > Where it is specified that the bus name can be either omitted or
> > > followed by any character, to separate it from the device identifier.
> > > 
> > > This means that using ':' is fine, as well as '('. As long as the device
> > > PMD afterward ignore the dangling ')' during devargs parsing, this should
> > > be fine.
> > > 
> > > I don't think this is very clean, but it works.  
> > 
> > Thanks for the info, I see how "(" works, but ")" is takes as part of argument
> > and causing problem, I think better to not give "()" as supported syntax at all.
> > 
> > btw, now both -w and --vdev are valid and can be used interchangeably, right? I
> > mean all following are valid?
> > -w pci:0000:86:06.0,enable_floating_veb=1
> > -w vdev:net_pcap,iface=lo
> > --vdev pci:0000:86:06.0,enable_floating_veb=1
> > --vdev vdev:net_pcap,iface=lo
> > 
> >   
> 
> They are both valid but cannot be used interchangeably.
> 
> This was the case at one point, between two rcs, because I had removed
> the devtype and the bus black/white-listing was configured another way.
> 
> A user complained about the API change because it was not announced in
> time IIRC, and this was scrapped. Since then, the effort has been to on
> the new syntax instead of cleaning the old system.
> 
> They cannot be used interchangeably because an rte_devtype is defined by
> the parameter choosen. BLACKLISTED_PCI for -b, WHITELISTED_PCI for -w,
> and VIRTUAL for --vdev.
> 
> -b will actually change the bus configuration to "blacklist mode", -w to
> "whitelist mode", and --vdev will do nothing.
> 
> So a PCI device declared using --vdev, will actually be skipped during
> PCI probe because by default it operates in blacklist mode, and the
> device policy is not WHITELISTED. This could surprise the one attempting
> this trick.
> 
> This semantic is subtle and bug-prone. I'm adding it to the pile of
> reasons I'd like to remove the blacklist mode altogether.
> 

It would have been nice if there was a --devargs instead of overloading -w and -b.
Especially when just changing parameter on a device.

Patch

diff --git a/drivers/bus/vmbus/linux/vmbus_bus.c b/drivers/bus/vmbus/linux/vmbus_bus.c
index 52d6a3c05306..527a6a39f2bc 100644
--- a/drivers/bus/vmbus/linux/vmbus_bus.c
+++ b/drivers/bus/vmbus/linux/vmbus_bus.c
@@ -276,6 +276,8 @@  vmbus_scan_one(const char *name)
 		dev->device.numa_node = SOCKET_ID_ANY;
 	}
 
+	dev->device.devargs = vmbus_devargs_lookup(dev);
+
 	/* device is valid, add in list (sorted) */
 	VMBUS_LOG(DEBUG, "Adding vmbus device %s", name);
 
diff --git a/drivers/bus/vmbus/private.h b/drivers/bus/vmbus/private.h
index 9964fc42a7b5..f2022a68cb2b 100644
--- a/drivers/bus/vmbus/private.h
+++ b/drivers/bus/vmbus/private.h
@@ -66,6 +66,9 @@  struct vmbus_channel {
 
 #define VMBUS_MAX_CHANNELS	64
 
+struct rte_devargs *
+vmbus_devargs_lookup(struct rte_vmbus_device *dev);
+
 int vmbus_chan_create(const struct rte_vmbus_device *device,
 		      uint16_t relid, uint16_t subid, uint8_t monitor_id,
 		      struct vmbus_channel **new_chan);
diff --git a/drivers/bus/vmbus/vmbus_common.c b/drivers/bus/vmbus/vmbus_common.c
index c7165ad54fe2..979fabdcbb61 100644
--- a/drivers/bus/vmbus/vmbus_common.c
+++ b/drivers/bus/vmbus/vmbus_common.c
@@ -85,7 +85,6 @@  vmbus_match(const struct rte_vmbus_driver *dr,
 
 	return false;
 }
-
 /*
  * If device ID match, call the devinit() function of the driver.
  */
@@ -204,6 +203,27 @@  vmbus_parse(const char *name, void *addr)
 	return ret;
 }
 
+/*
+ * scan for matching device args on command line
+ * example:
+ *	-w 'vmbus(635a7ae3-091e-4410-ad59-667c4f8c04c3,latency=20)'
+ */
+struct rte_devargs *
+vmbus_devargs_lookup(struct rte_vmbus_device *dev)
+{
+	struct rte_devargs *devargs;
+	rte_uuid_t addr;
+
+	RTE_EAL_DEVARGS_FOREACH("vmbus", devargs) {
+		vmbus_parse(devargs->name, &addr);
+
+		if (rte_uuid_compare(dev->device_id, addr) == 0)
+			return devargs;
+	}
+	return NULL;
+
+}
+
 /* register vmbus driver */
 void
 rte_vmbus_register(struct rte_vmbus_driver *driver)