[dpdk-dev,V18,2/5] bus/pci: implement handle hot unplug operation
Checks
Commit Message
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(+)
Comments
> -----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?
> + 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?
> + /* 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?
> + 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.
> +{
> + 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
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
@@ -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
+ break;
+ case RTE_KDRV_IGB_UIO:
+ case RTE_KDRV_UIO_GENERIC:
+ if (rte_eal_using_phys_addrs()) {
+ /* 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;
+ 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),
@@ -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)
+{
+ int i;
+ uint64_t phaddr;
+ void *map_address;
+
+ if (dev == NULL)
+ return -1;
+
+ /* Map all BARs */
+ for (i = 0; i != PCI_MAX_RESOURCE; i++) {
+ /* skip empty BAR */
+ phaddr = dev->mem_resource[i].phys_addr;
+ if (phaddr == 0)
+ continue;
+ 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)
{
@@ -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);
+
+/**
* Map device memory to uio resource
*
* This function is private to EAL.