Re: [dpdk-dev] [PATCH 04/14] vhost: make vDPA framework bus agnostic

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 6/12/20 2:46 PM, Gaëtan Rivet wrote:
> On 11/06/20 23:37 +0200, Maxime Coquelin wrote:
>> This patch makes the vDPA framework to no more
>> support only PCI devices, but any devices by relying
>> on the generic device name as identifier.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  drivers/vdpa/ifc/ifcvf_vdpa.c          |  6 +-
>>  drivers/vdpa/mlx5/mlx5_vdpa.c          |  8 +--
>>  drivers/vdpa/mlx5/mlx5_vdpa.h          |  2 +-
>>  examples/vdpa/main.c                   | 49 ++++++++--------
>>  lib/librte_vhost/rte_vdpa.h            | 42 +++++++-------
>>  lib/librte_vhost/rte_vhost_version.map |  1 +
>>  lib/librte_vhost/vdpa.c                | 79 +++++++++++---------------
>>  7 files changed, 85 insertions(+), 102 deletions(-)
>>
> 
> [...]
> 
>> diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.c b/drivers/vdpa/mlx5/mlx5_vdpa.c
>> index 1113d6cef0..e8255c7d7e 100644
>> --- a/drivers/vdpa/mlx5/mlx5_vdpa.c
>> +++ b/drivers/vdpa/mlx5/mlx5_vdpa.c
>> @@ -501,14 +501,13 @@ mlx5_vdpa_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>>  	priv->caps = attr.vdpa;
>>  	priv->log_max_rqt_size = attr.log_max_rqt_size;
>>  	priv->ctx = ctx;
>> -	priv->dev_addr.pci_addr = pci_dev->addr;
>> -	priv->dev_addr.type = VDPA_ADDR_PCI;
>> +	priv->pci_dev = pci_dev;
>>  	priv->var = mlx5_glue->dv_alloc_var(ctx, 0);
>>  	if (!priv->var) {
>>  		DRV_LOG(ERR, "Failed to allocate VAR %u.\n", errno);
>>  		goto error;
>>  	}
>> -	priv->id = rte_vdpa_register_device(&priv->dev_addr, &mlx5_vdpa_ops);
>> +	priv->id = rte_vdpa_register_device(&pci_dev->device, &mlx5_vdpa_ops);
>>  	if (priv->id < 0) {
>>  		DRV_LOG(ERR, "Failed to register vDPA device.");
>>  		rte_errno = rte_errno ? rte_errno : EINVAL;
>> @@ -550,8 +549,7 @@ mlx5_vdpa_pci_remove(struct rte_pci_device *pci_dev)
>>  
>>  	pthread_mutex_lock(&priv_list_lock);
>>  	TAILQ_FOREACH(priv, &priv_list, next) {
>> -		if (memcmp(&priv->dev_addr.pci_addr, &pci_dev->addr,
>> -			   sizeof(pci_dev->addr)) == 0) {
>> +		if (priv->pci_dev == pci_dev) {
> 
> Assignment is ok I think but comparison is not, rte_pci_addr_cmp()
> should be used here instead. (memcmp was not correct either.)
> 
> The structure can be padded depending on arch.
> 
> Everything else seems ok to me.
> 

Ok, I will change to use rte_pci_addr_cmp() in next revision.

Thanks,
Maxime