Re: [dpdk-dev] [PATCH v2] vfio: refactor PCI BAR mapping

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 25-Sep-17 4:04 PM, Jonas Pfefferle wrote:
> Split pci_vfio_map_resource for primary and secondary processes.
> Save all relevant mapping data in primary process to allow
> the secondary process to perform mappings.
> 
> Signed-off-by: Jonas Pfefferle <jpf@zurich.ibm.com>
> ---
> v2:
> * fix zero size and offset when trying to mmap non msix bar
> 
>   lib/librte_eal/common/include/rte_pci.h    |   7 +
>   lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 446 +++++++++++++++++------------
>   2 files changed, 271 insertions(+), 182 deletions(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
> index 8b12339..0821af9 100644
> --- a/lib/librte_eal/common/include/rte_pci.h
> +++ b/lib/librte_eal/common/include/rte_pci.h
> @@ -214,6 +214,12 @@ struct pci_map {
>   	uint64_t phaddr;
>   };
>   
> +struct pci_msix_table {
> +	int bar_index;
> +	uint32_t offset;
> +	uint32_t size;
> +};
> +
>   /**
>    * A structure describing a mapped PCI resource.
>    * For multi-process we need to reproduce all PCI mappings in secondary
> @@ -226,6 +232,7 @@ struct mapped_pci_resource {
>   	char path[PATH_MAX];
>   	int nb_maps;
>   	struct pci_map maps[PCI_MAX_RESOURCE];
> +	struct pci_msix_table msix_table;
>   };
>   
>   /** mapped pci device list */
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> index aa9d96e..6443bd5 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> @@ -88,8 +88,7 @@ pci_vfio_write_config(const struct rte_intr_handle *intr_handle,
>   
>   /* get PCI BAR number where MSI-X interrupts are */
>   static int
> -pci_vfio_get_msix_bar(int fd, int *msix_bar, uint32_t *msix_table_offset,
> -		      uint32_t *msix_table_size)
> +pci_vfio_get_msix_bar(int fd, struct pci_msix_table *msix_table)
>   {
>   	int ret;
>   	uint32_t reg;
> @@ -161,9 +160,10 @@ pci_vfio_get_msix_bar(int fd, int *msix_bar, uint32_t *msix_table_offset,
>   				return -1;
>   			}
>   
> -			*msix_bar = reg & RTE_PCI_MSIX_TABLE_BIR;
> -			*msix_table_offset = reg & RTE_PCI_MSIX_TABLE_OFFSET;
> -			*msix_table_size = 16 * (1 + (flags & RTE_PCI_MSIX_FLAGS_QSIZE));
> +			msix_table->bar_index = reg & RTE_PCI_MSIX_TABLE_BIR;
> +			msix_table->offset = reg & RTE_PCI_MSIX_TABLE_OFFSET;
> +			msix_table->size =
> +				16 * (1 + (flags & RTE_PCI_MSIX_FLAGS_QSIZE));
>   
>   			return 0;
>   		}
> @@ -300,25 +300,150 @@ pci_vfio_setup_interrupts(struct rte_pci_device *dev, int vfio_dev_fd)
>   	return -1;
>   }
>   
> -/*
> - * map the PCI resources of a PCI device in virtual memory (VFIO version).
> - * primary and secondary processes follow almost exactly the same path
> - */
> -int
> -pci_vfio_map_resource(struct rte_pci_device *dev)
> +static int
> +pci_vfio_is_ioport_bar(int vfio_dev_fd, int bar_index)
> +{
> +	uint32_t ioport_bar;
> +	int ret;
> +
> +	ret = pread64(vfio_dev_fd, &ioport_bar, sizeof(ioport_bar),
> +			  VFIO_GET_REGION_ADDR(VFIO_PCI_CONFIG_REGION_INDEX)
> +			  + PCI_BASE_ADDRESS_0 + bar_index*4);
> +	if (ret != sizeof(ioport_bar)) {
> +		RTE_LOG(ERR, EAL, "Cannot read command (%x) from config space!\n",
> +			PCI_BASE_ADDRESS_0 + bar_index*4);
> +		return -1;
> +	}
> +
> +	return ioport_bar & PCI_BASE_ADDRESS_SPACE_IO;

Not sure i like this. I think it's better to be explicit, e.g.
return ioport_bar & PCI_BASE_ADDRESS_SPACE_IO != 0;

Makes no difference (both because return value is non-zero and because 
PCI_BASE_ADDRESS_SPACE_IO is 0x01), but still, better to make intentions 
clear i think.

Otherwise, did a quick smoke-test and it works, so

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

Keep the ack if you decide to submit a v3 :)