Hi Anatoly,
"Burakov, Anatoly" <anatoly.bura...@intel.com> wrote on 10/04/2017 03:13:22
PM:
> From: "Burakov, Anatoly" <anatoly.bura...@intel.com>
> To: Jonas Pfefferle <j...@zurich.ibm.com>, dev@dpdk.org
> Date: 10/04/2017 03:14 PM
> Subject: Re: [PATCH v2] vfio: refactor PCI BAR mapping
>
> 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 <j...@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.
I agree. I wanted to change this anyway but forgot. V3 is going out in a
sec. Thanks for the ack ;)
>
> Otherwise, did a quick smoke-test and it works, so
>
> Acked-by: Anatoly Burakov <anatoly.bura...@intel.com>
>
> Keep the ack if you decide to submit a v3 :)
>
