Re: [dpdk-dev] [PATCH V18 2/5] bus/pci: implement handle hot unplug operation

["Guo, Jia" <jia.guo@intel.com>]

On 4/4/2018 1:25 PM, Tan, Jianfeng wrote:
>
>> -----Original Message-----
>> From: Guo, Jia
>> Sent: Wednesday, April 4, 2018 2:17 AM
>> To: stephen@networkplumber.org; Richardson, Bruce; Yigit, Ferruh;
>> Ananyev, Konstantin; gaetan.rivet@6wind.com; Wu, Jingjing;
>> thomas@monjalon.net; motih@mellanox.com; Van Haaren, Harry; Tan,
>> Jianfeng
>> Cc: jblunck@infradead.org; shreyansh.jain@nxp.com; dev@dpdk.org; Guo,
>> Jia; Zhang, Helin
>> Subject: [PATCH V18 2/5] bus/pci: implement handle hot unplug operation
>>
>> When handle device hot unplug event, remap a dummy memory to avoid
>> bus read/write error.
>>
>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>> ---
>> v16->v15;
>> split patch, merge some function to be simple
>> ---
>>   drivers/bus/pci/pci_common.c     | 42
>> ++++++++++++++++++++++++++++++++++++++++
>>   drivers/bus/pci/pci_common_uio.c | 33
>> +++++++++++++++++++++++++++++++
>>   drivers/bus/pci/private.h        | 12 ++++++++++++
>>   3 files changed, 87 insertions(+)
>>
>> diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
>> index 2a00f36..fa077ec 100644
>> --- a/drivers/bus/pci/pci_common.c
>> +++ b/drivers/bus/pci/pci_common.c
>> @@ -474,6 +474,47 @@ pci_find_device(const struct rte_device *start,
>> rte_dev_cmp_t cmp,
>>   }
>>
>>   static int
>> +pci_handle_hot_unplug(struct rte_device *dev)
>> +{
>> +	struct rte_pci_device *pdev;
>> +	int ret;
>> +
>> +	if (dev == NULL)
>> +		return -EINVAL;
>> +
>> +	pdev = RTE_DEV_TO_PCI(dev);
>> +
>> +	/* remap resources for devices */
>> +	switch (pdev->kdrv) {
>> +	case RTE_KDRV_VFIO:
>> +#ifdef VFIO_PRESENT
>> +		/* TODO */
>> +#endif
> What's the difference between uio and vfio? We can just fall though?
the VFIO mapping is functional different with uio, and the key is i 
don't implement vfio hotplug right now, so let it TODO.
>> +		break;
>> +	case RTE_KDRV_IGB_UIO:
>> +	case RTE_KDRV_UIO_GENERIC:
>> +		if (rte_eal_using_phys_addrs()) {
> Why do we care about if we are using physical addresses?
please check with the mapping function.
>> +			/* map resources for devices that use uio */
>> +			ret = pci_uio_remap_resource(pdev);
>> +		}
>> +		break;
>> +	case RTE_KDRV_NIC_UIO:
>> +		ret = pci_uio_remap_resource(pdev);
>> +		break;
>> +	default:
>> +		RTE_LOG(DEBUG, EAL,
>> +			"  Not managed by a supported kernel driver, skipped\n");
>> +		ret = 1;
> -1 for such case?
thanks.
>> +		break;
>> +	}
>> +
>> +	if (ret != 0)
>> +		RTE_LOG(ERR, EAL, "failed to handle hot unplug of %s",
>> +			pdev->name);
>> +	return ret;
>> +}
>> +
>> +static int
>>   pci_plug(struct rte_device *dev)
>>   {
>>   	return pci_probe_all_drivers(RTE_DEV_TO_PCI(dev));
>> @@ -503,6 +544,7 @@ struct rte_pci_bus rte_pci_bus = {
>>   		.unplug = pci_unplug,
>>   		.parse = pci_parse,
>>   		.get_iommu_class = rte_pci_get_iommu_class,
>> +		.handle_hot_unplug = pci_handle_hot_unplug,
>>   	},
>>   	.device_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.device_list),
>>   	.driver_list = TAILQ_HEAD_INITIALIZER(rte_pci_bus.driver_list),
>> diff --git a/drivers/bus/pci/pci_common_uio.c
>> b/drivers/bus/pci/pci_common_uio.c
>> index 54bc20b..468ade4 100644
>> --- a/drivers/bus/pci/pci_common_uio.c
>> +++ b/drivers/bus/pci/pci_common_uio.c
>> @@ -146,6 +146,39 @@ pci_uio_unmap(struct mapped_pci_resource
>> *uio_res)
>>   	}
>>   }
>>
>> +/* remap the PCI resource of a PCI device in private virtual memory */
>> +int
>> +pci_uio_remap_resource(struct rte_pci_device *dev)
> Why's this function uio specific? I think we can move it to pci_common.c.
not convince because not let vfio in this patch set.
>> +{
>> +	int i;
>> +	uint64_t phaddr;
>> +	void *map_address;
>> +
>> +	if (dev == NULL)
>> +		return -1;
>> +
>> +	/* Map all BARs */
> s/Map/Remap
>   
>> +	for (i = 0; i != PCI_MAX_RESOURCE; i++) {
>> +		/* skip empty BAR */
>> +		phaddr = dev->mem_resource[i].phys_addr;
>> +		if (phaddr == 0)
>> +			continue;
> How about just simple:
>
> if (dev->mem_resource[i].phys_addr == 0)
>
>> +		pci_unmap_resource(dev->mem_resource[i].addr,
>> +				(size_t)dev->mem_resource[i].len);
>> +		map_address = pci_map_resource(
>> +				dev->mem_resource[i].addr, -1, 0,
>> +				(size_t)dev->mem_resource[i].len,
>> +				MAP_ANONYMOUS | MAP_FIXED);
>> +		if (map_address == MAP_FAILED) {
>> +			RTE_LOG(ERR, EAL,
>> +				"Cannot remap resource for device %s\n",
>> dev->name);
>> +			return -1;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static struct mapped_pci_resource *
>>   pci_uio_find_resource(struct rte_pci_device *dev)
>>   {
>> diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
>> index 88fa587..7a862ef 100644
>> --- a/drivers/bus/pci/private.h
>> +++ b/drivers/bus/pci/private.h
>> @@ -173,6 +173,18 @@ void pci_uio_free_resource(struct rte_pci_device
>> *dev,
>>   		struct mapped_pci_resource *uio_res);
>>
>>   /**
>> + * remap the pci uio resource..
>> + *
>> + * @param dev
>> + *   Point to the struct rte pci device.
>> + * @return
>> + *   - On success, zero.
>> + *   - On failure, a negative value.
>> + */
>> +int
>> +pci_uio_remap_resource(struct rte_pci_device *dev);
> If we define
>
>> +
>> +/**
>>    * Map device memory to uio resource
>>    *
>>    * This function is private to EAL.
>> --
>> 2.7.4