Re: [dpdk-dev] [PATCH V18 2/5] bus/pci: implement handle hot unplug operation
On 4/4/2018 1:25 PM, Tan, Jianfeng wrote: -Original Message- From: Guo, Jia Sent: Wednesday, April 4, 2018 2:17 AM To: step...@networkplumber.org; Richardson, Bruce; Yigit, Ferruh; Ananyev, Konstantin; gaetan.ri...@6wind.com; Wu, Jingjing; tho...@monjalon.net; mo...@mellanox.com; Van Haaren, Harry; Tan, Jianfeng Cc: jblu...@infradead.org; shreyansh.j...@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 --- 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
Re: [dpdk-dev] [PATCH V18 2/5] bus/pci: implement handle hot unplug operation
> -Original Message- > From: Guo, Jia > Sent: Wednesday, April 4, 2018 2:17 AM > To: step...@networkplumber.org; Richardson, Bruce; Yigit, Ferruh; > Ananyev, Konstantin; gaetan.ri...@6wind.com; Wu, Jingjing; > tho...@monjalon.net; mo...@mellanox.com; Van Haaren, Harry; Tan, > Jianfeng > Cc: jblu...@infradead.org; shreyansh.j...@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 > --- > 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_p