Re: [Qemu-devel] [RFC/RFT PATCH 2/5] vfio/pci: Add base BAR MemoryRegion

[Alexey Kardashevskiy]

On 18/12/17 16:02, Alex Williamson wrote:
> Add one more layer to our stack of MemoryRegions, this base region
> allows us to register BARs independently of the vfio region or to
> extend the size of BARs which do map to a region.  This will be
> useful when we want hypervisor defined BARs or sections of BARs,
> for purposes such as relocating MSI-X emulation.  We therefore call
> msix_init() based on this new base MemoryRegion, while the quirks,
> which only modify regions still operate on those sub-MemoryRegions.


Looks ok, one worry though - the default config produces this:


memory-region: address@hidden
  0000000000000000-ffffffffffffffff (prio 0, i/o): address@hidden
    0000210000000000-000021000000ffff (prio 1, i/o): 0001:03:00.0 base BAR 1
      0000210000000000-000021000000ffff (prio 0, i/o): 0001:03:00.0 BAR 1
      000021000000e000-000021000000e5ff (prio 0, i/o): msix-table
      000021000000f000-000021000000f00f (prio 0, i/o): msix-pba [disabled]
    0000210000040000-000021000007ffff (prio 1, i/o): 0001:03:00.0 base BAR 3
      0000210000040000-000021000007ffff (prio 0, i/o): 0001:03:00.0 BAR 3
        0000210000040000-000021000007ffff (prio 0, ramd): 0001:03:00.0 BAR
3 mmaps[0]


Where "BAR 1" and "msix-table" overlap. It resolves correctly:


FlatView #1
 AS "memory", root: system
 AS "cpu-memory", root: system
 Root memory region: system
  0000000000000000-000000007fffffff (prio 0, ram): ppc_spapr.ram
  0000210000000000-000021000000dfff (prio 0, i/o): 0001:03:00.0 BAR 1
  000021000000e000-000021000000e5ff (prio 0, i/o): msix-table
  000021000000e600-000021000000ffff (prio 0, i/o): 0001:03:00.0 BAR 1
@000000000000e600
  0000210000040000-000021000007ffff (prio 0, ramd): 0001:03:00.0 BAR 3 mmaps[0]


but this looks like an accident - should not we raise the msix-table
priority or lower the BAR1 priority (to -1)?



> 
> Signed-off-by: Alex Williamson <address@hidden>
> ---
>  hw/vfio/pci.c |   74 
> ++++++++++++++++++++++++++++++++++++++++++++-------------
>  hw/vfio/pci.h |    3 ++
>  2 files changed, 60 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index c977ee327f94..8f46fdd1d391 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1440,9 +1440,9 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int 
> pos, Error **errp)
>      vdev->msix->pending = g_malloc0(BITS_TO_LONGS(vdev->msix->entries) *
>                                      sizeof(unsigned long));
>      ret = msix_init(&vdev->pdev, vdev->msix->entries,
> -                    vdev->bars[vdev->msix->table_bar].region.mem,
> +                    vdev->bars[vdev->msix->table_bar].mr,
>                      vdev->msix->table_bar, vdev->msix->table_offset,
> -                    vdev->bars[vdev->msix->pba_bar].region.mem,
> +                    vdev->bars[vdev->msix->pba_bar].mr,
>                      vdev->msix->pba_bar, vdev->msix->pba_offset, pos,
>                      &err);
>      if (ret < 0) {
> @@ -1482,8 +1482,8 @@ static void vfio_teardown_msi(VFIOPCIDevice *vdev)
>  
>      if (vdev->msix) {
>          msix_uninit(&vdev->pdev,
> -                    vdev->bars[vdev->msix->table_bar].region.mem,
> -                    vdev->bars[vdev->msix->pba_bar].region.mem);
> +                    vdev->bars[vdev->msix->table_bar].mr,
> +                    vdev->bars[vdev->msix->pba_bar].mr);
>          g_free(vdev->msix->pending);
>      }
>  }
> @@ -1500,12 +1500,11 @@ static void vfio_mmap_set_enabled(VFIOPCIDevice 
> *vdev, bool enabled)
>      }
>  }
>  
> -static void vfio_bar_setup(VFIOPCIDevice *vdev, int nr)
> +static void vfio_bar_prepare(VFIOPCIDevice *vdev, int nr)
>  {
>      VFIOBAR *bar = &vdev->bars[nr];
>  
>      uint32_t pci_bar;
> -    uint8_t type;
>      int ret;
>  
>      /* Skip both unimplemented BARs and the upper half of 64bit BARS. */
> @@ -1524,23 +1523,52 @@ static void vfio_bar_setup(VFIOPCIDevice *vdev, int 
> nr)
>      pci_bar = le32_to_cpu(pci_bar);
>      bar->ioport = (pci_bar & PCI_BASE_ADDRESS_SPACE_IO);
>      bar->mem64 = bar->ioport ? 0 : (pci_bar & PCI_BASE_ADDRESS_MEM_TYPE_64);
> -    type = pci_bar & (bar->ioport ? ~PCI_BASE_ADDRESS_IO_MASK :
> -                                    ~PCI_BASE_ADDRESS_MEM_MASK);
> +    bar->type = pci_bar & (bar->ioport ? ~PCI_BASE_ADDRESS_IO_MASK :
> +                                         ~PCI_BASE_ADDRESS_MEM_MASK);
> +    bar->size = bar->region.size;
> +}
>  
> -    if (vfio_region_mmap(&bar->region)) {
> -        error_report("Failed to mmap %s BAR %d. Performance may be slow",
> -                     vdev->vbasedev.name, nr);
> +static void vfio_bars_prepare(VFIOPCIDevice *vdev)
> +{
> +    int i;
> +
> +    for (i = 0; i < PCI_ROM_SLOT; i++) {
> +        vfio_bar_prepare(vdev, i);
>      }
> +}
>  
> -    pci_register_bar(&vdev->pdev, nr, type, bar->region.mem);
> +static void vfio_bar_register(VFIOPCIDevice *vdev, int nr)
> +{
> +    VFIOBAR *bar = &vdev->bars[nr];
> +    char *name;
> +
> +    if (!bar->size) {
> +        return;
> +    }
> +
> +    bar->mr = g_new0(MemoryRegion, 1);
> +    name = g_strdup_printf("%s base BAR %d", vdev->vbasedev.name, nr);
> +    memory_region_init_io(bar->mr, OBJECT(vdev), NULL, NULL, name, 
> bar->size);
> +    g_free(name);
> +
> +    if (bar->region.size) {
> +        memory_region_add_subregion(bar->mr, 0, bar->region.mem);
> +
> +        if (vfio_region_mmap(&bar->region)) {
> +            error_report("Failed to mmap %s BAR %d. Performance may be slow",
> +                         vdev->vbasedev.name, nr);
> +        }
> +    }
> +
> +    pci_register_bar(&vdev->pdev, nr, bar->type, bar->mr);
>  }
>  
> -static void vfio_bars_setup(VFIOPCIDevice *vdev)
> +static void vfio_bars_register(VFIOPCIDevice *vdev)
>  {
>      int i;
>  
>      for (i = 0; i < PCI_ROM_SLOT; i++) {
> -        vfio_bar_setup(vdev, i);
> +        vfio_bar_register(vdev, i);
>      }
>  }
>  
> @@ -1549,8 +1577,13 @@ static void vfio_bars_exit(VFIOPCIDevice *vdev)
>      int i;
>  
>      for (i = 0; i < PCI_ROM_SLOT; i++) {
> +        VFIOBAR *bar = &vdev->bars[i];
> +
>          vfio_bar_quirk_exit(vdev, i);
> -        vfio_region_exit(&vdev->bars[i].region);
> +        vfio_region_exit(&bar->region);
> +        if (bar->region.size) {
> +            memory_region_del_subregion(bar->mr, bar->region.mem);
> +        }
>      }
>  
>      if (vdev->vga) {
> @@ -1564,8 +1597,14 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
>      int i;
>  
>      for (i = 0; i < PCI_ROM_SLOT; i++) {
> +        VFIOBAR *bar = &vdev->bars[i];
> +
>          vfio_bar_quirk_finalize(vdev, i);
> -        vfio_region_finalize(&vdev->bars[i].region);
> +        vfio_region_finalize(&bar->region);
> +        if (bar->size) {
> +           object_unparent(OBJECT(bar->mr));
> +           g_free(bar->mr);
> +        }
>      }
>  
>      if (vdev->vga) {
> @@ -2810,7 +2849,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>          goto error;
>      }
>  
> -    vfio_bars_setup(vdev);
> +    vfio_bars_prepare(vdev);
> +    vfio_bars_register(vdev);
>  
>      ret = vfio_add_capabilities(vdev, errp);
>      if (ret) {
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 3d753222ca4c..dcdb1a806769 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -33,6 +33,9 @@ typedef struct VFIOQuirk {
>  
>  typedef struct VFIOBAR {
>      VFIORegion region;
> +    MemoryRegion *mr;
> +    size_t size;
> +    uint8_t type;
>      bool ioport;
>      bool mem64;
>      QLIST_HEAD(, VFIOQuirk) quirks;
> 
> 


-- 
Alexey