Re: [dpdk-dev] [PATCH v3 4/5] bus: add device arguments name parsing API

[Thomas Monjalon <thomas@monjalon.net>]

01/04/2021 17:13, Xueming(Steven) Li:
>From: Thomas Monjalon <thomas@monjalon.net>
> >30/03/2021 14:15, Xueming Li:
> >> To use Global Device Syntax as devargs, name is required for device
> >> management.
> >
> >Context is missing.
> >You mean the argument "name" for the vdev bus?
> 
> Devargs.name, it is used by probe and device iterator.

I think we could avoid having a name with the new syntax.
In my understanding, this is for compatibility with the legacy syntax.

> To locate a device from a devargs, devargs.name is compared
> agains name of probed devices.
> This not an issue for legacy syntax,
> since the string after "bus:" is saved as name.

It would be interesting to explain where the name is parsed
for the legacy syntax: rte_devargs_parse
and for the new syntax: rte_devargs_layers_parse called
in rte_devargs_parse in the next patch.

> >> In legacy parsing API, devargs name was extracted after bus name:
> >>   bus:name,kv_params,,,
> >>
> >> To parse new Global Device Syntax, this patch introduces new bus API
> >> to parse devargs and update name, different bus driver might choose
> >> different keys from parameters with unified formating, example:
> >>  -a bus=pci,addr=83:00.0/class=eth/driver=mlx5,...
> >>     name: 0000:03:00.0
> >>  -a bus=vdev,name=pcap0/class=eth/driver=pcap,...
> >>     name:pcap0
> >
> >Only PCI and vdev buses are implemented.
> >What can be the plan for others?
> >We should track the progress somewhere, maybe with TODO comments.
> 
> Like legacy parser, how about using "name" as default name key, the new syntax parser can resolve it by default.
> Then PCI bus overrides by using "addr" key in new bus API, 
> vdev and other bus drivers simply use default implementation, i.e. using "name" as key..

Yes, you mean if devargs_parse is not implemented by the bus driver,
the default is to parse the name property,
while the PCI implementation fills the devargs name with the addr property.

> >[...]
> >> +int
> >> +rte_pci_devargs_parse(struct rte_devargs *da) {
> >> +	struct rte_kvargs *kvargs;
> >> +	const char *addr_str;
> >> +	struct rte_pci_addr addr;
> >> +	int ret;
> >> +
> >> +	if (da == NULL)
> >> +		return 0;
> >> +	RTE_ASSERT(da->bus_str != NULL);
> >> +
> >> +	kvargs = rte_kvargs_parse(da->bus_str, NULL);
> >> +	if (kvargs == NULL) {
> >> +		RTE_LOG(ERR, EAL, "cannot parse argument list: %s\n",
> >> +			da->bus_str);
> >> +		ret = -ENODEV;
> >> +		goto out;
> >> +	}
> >> +
> >> +	addr_str = rte_kvargs_get(kvargs, pci_params_keys[RTE_PCI_PARAM_ADDR]);
> >> +	if (addr_str == NULL) {
> >> +		RTE_LOG(ERR, EAL, "No PCI address specified using '%s=<id>' in: %s\n",
> >> +			pci_params_keys[RTE_PCI_PARAM_ADDR], da->bus_str);
> >> +		ret = -ENODEV;
> >> +		goto out;
> >> +	}
> >> +
> >> +	ret = rte_pci_addr_parse(addr_str, &addr);
> >> +	if (ret != 0) {
> >> +		RTE_LOG(ERR, EAL, "PCI address invalid: %s\n", da->bus_str);
> >> +		ret = -EINVAL;
> >> +		goto out;
> >> +	}
> >> +
> >> +	rte_pci_device_name(&addr, da->name, sizeof(da->name));
> >> +
> >> +	/* TODO: class parse -> driver parse */
> >
> >Please could you give a longer explanation of what is missing?
> 
> Just an option to give class driver and device driver to override name parsing.
> But this should happen in new devargs parser, not here if needed.
> 
> I'll remove this line, and mention it in commit log.

What would be the benefit of overriding devargs name parsing?
I feel it would be better to not rely on the devargs name
with the new syntax if we have such need.