Re: [Qemu-devel] [PATCH 3/8] libqos: Move BAR assignment to common code

[David Gibson]

On Tue, Oct 18, 2016 at 05:00:08PM +0200, Laurent Vivier wrote:
> 
> 
> On 18/10/2016 12:52, David Gibson wrote:
> > The PCI backends in libqos each supply an iomap() and iounmap() function
> > which is used to set up a specified PCI BAR.  But PCI BAR allocation takes
> > place entirely within PCI space, so doesn't really need per-backend
> > versions.  For example, Linux includes generic BAR allocation code used on
> > platforms where that isn't done by firmware.
> > 
> > This patch merges the BAR allocation from the two existing backends into a
> > single simplified copy.  The back ends just need to set up some parameters
> > describing the window of PCI IO and PCI memory addresses which are
> > available for allocation.  Like both the existing versions the new one uses
> > a simple bump allocator.
> > 
> > Note that (again like the existing versions) this doesn't really handle
> > 64-bit memory BARs properly.  It is actually used for such a BAR by the
> > ivshmem test, and apparently the 32-bit MMIO BAR logic is close enough to
> > work, as long as the BAR isn't too big.  Fixing that to properly handle
> > 64-bit BAR allocation is a problem for another time.
> > 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> >  tests/libqos/pci-pc.c    | 79 
> > ++--------------------------------------------
> >  tests/libqos/pci-spapr.c | 81 
> > ++----------------------------------------------
> >  tests/libqos/pci.c       | 56 +++++++++++++++++++++++++++++++--
> >  tests/libqos/pci.h       |  7 ++---
> >  4 files changed, 63 insertions(+), 160 deletions(-)
> > 
> > diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> > index 51dff8a..b087d13 100644
> > --- a/tests/libqos/pci-pc.c
> > +++ b/tests/libqos/pci-pc.c
> > @@ -17,7 +17,6 @@
> >  #include "hw/pci/pci_regs.h"
> >  
> >  #include "qemu-common.h"
> > -#include "qemu/host-utils.h"
> >  
> >  
> >  #define ACPI_PCIHP_ADDR         0xae00
> > @@ -132,71 +131,6 @@ static void qpci_pc_config_writel(QPCIBus *bus, int 
> > devfn, uint8_t offset, uint3
> >      outl(0xcfc, value);
> >  }
> >  
> > -static void *qpci_pc_iomap(QPCIBus *bus, QPCIDevice *dev, int barno, 
> > uint64_t *sizeptr)
> > -{
> > -    QPCIBusPC *s = container_of(bus, QPCIBusPC, bus);
> > -    static const int bar_reg_map[] = {
> > -        PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_1, PCI_BASE_ADDRESS_2,
> > -        PCI_BASE_ADDRESS_3, PCI_BASE_ADDRESS_4, PCI_BASE_ADDRESS_5,
> > -    };
> > -    int bar_reg;
> > -    uint32_t addr;
> > -    uint64_t size;
> > -    uint32_t io_type;
> > -
> > -    g_assert(barno >= 0 && barno <= 5);
> > -    bar_reg = bar_reg_map[barno];
> > -
> > -    qpci_config_writel(dev, bar_reg, 0xFFFFFFFF);
> > -    addr = qpci_config_readl(dev, bar_reg);
> > -
> > -    io_type = addr & PCI_BASE_ADDRESS_SPACE;
> > -    if (io_type == PCI_BASE_ADDRESS_SPACE_IO) {
> > -        addr &= PCI_BASE_ADDRESS_IO_MASK;
> > -    } else {
> > -        addr &= PCI_BASE_ADDRESS_MEM_MASK;
> > -    }
> > -
> > -    size = (1ULL << ctzl(addr));
> > -    if (size == 0) {
> > -        return NULL;
> > -    }
> > -    if (sizeptr) {
> > -        *sizeptr = size;
> > -    }
> > -
> > -    if (io_type == PCI_BASE_ADDRESS_SPACE_IO) {
> > -        uint16_t loc;
> > -
> > -        g_assert(QEMU_ALIGN_UP(s->pci_iohole_alloc, size) + size
> > -                 <= s->pci_iohole_size);
> > -        s->pci_iohole_alloc = QEMU_ALIGN_UP(s->pci_iohole_alloc, size);
> > -        loc = s->pci_iohole_start + s->pci_iohole_alloc;
> > -        s->pci_iohole_alloc += size;
> > -
> > -        qpci_config_writel(dev, bar_reg, loc | PCI_BASE_ADDRESS_SPACE_IO);
> > -
> > -        return (void *)(intptr_t)loc;
> > -    } else {
> > -        uint64_t loc;
> > -
> > -        g_assert(QEMU_ALIGN_UP(s->pci_hole_alloc, size) + size
> > -                 <= s->pci_hole_size);
> > -        s->pci_hole_alloc = QEMU_ALIGN_UP(s->pci_hole_alloc, size);
> > -        loc = s->pci_hole_start + s->pci_hole_alloc;
> > -        s->pci_hole_alloc += size;
> > -
> > -        qpci_config_writel(dev, bar_reg, loc);
> > -
> > -        return (void *)(intptr_t)loc;
> > -    }
> > -}
> > -
> > -static void qpci_pc_iounmap(QPCIBus *bus, void *data)
> > -{
> > -    /* FIXME */
> > -}
> > -
> >  QPCIBus *qpci_init_pc(QGuestAllocator *alloc)
> >  {
> >      QPCIBusPC *ret;
> > @@ -227,16 +161,9 @@ QPCIBus *qpci_init_pc(QGuestAllocator *alloc)
> >      ret->bus.config_writew = qpci_pc_config_writew;
> >      ret->bus.config_writel = qpci_pc_config_writel;
> >  
> > -    ret->bus.iomap = qpci_pc_iomap;
> > -    ret->bus.iounmap = qpci_pc_iounmap;
> > -
> > -    ret->pci_hole_start = 0xE0000000;
> > -    ret->pci_hole_size = 0x20000000;
> > -    ret->pci_hole_alloc = 0;
> > -
> > -    ret->pci_iohole_start = 0xc000;
> > -    ret->pci_iohole_size = 0x4000;
> > -    ret->pci_iohole_alloc = 0;
> 
> I think you can remove all these fields (pci_hole_.., pci_iohole_...)
> from QPCIBusPC.
> 
> > +    ret->bus.pio_alloc_ptr = 0xc000;
> > +    ret->bus.mmio_alloc_ptr = 0xE0000000;
> > +    ret->bus.mmio_limit = 0x100000000ULL;
> >  
> >      return &ret->bus;
> >  }
> > diff --git a/tests/libqos/pci-spapr.c b/tests/libqos/pci-spapr.c
> > index 2d26a94..9a18d8a 100644
> > --- a/tests/libqos/pci-spapr.c
> > +++ b/tests/libqos/pci-spapr.c
> > @@ -167,72 +167,6 @@ static void qpci_spapr_config_writel(QPCIBus *bus, int 
> > devfn, uint8_t offset,
> >      qrtas_ibm_write_pci_config(s->alloc, s->buid, config_addr, 4, value);
> >  }
> >  
> > -static void *qpci_spapr_iomap(QPCIBus *bus, QPCIDevice *dev, int barno,
> > -                              uint64_t *sizeptr)
> > -{
> > -    QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus);
> > -    static const int bar_reg_map[] = {
> > -        PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_1, PCI_BASE_ADDRESS_2,
> > -        PCI_BASE_ADDRESS_3, PCI_BASE_ADDRESS_4, PCI_BASE_ADDRESS_5,
> > -    };
> > -    int bar_reg;
> > -    uint32_t addr;
> > -    uint64_t size;
> > -    uint32_t io_type;
> > -
> > -    g_assert(barno >= 0 && barno <= 5);
> > -    bar_reg = bar_reg_map[barno];
> > -
> > -    qpci_config_writel(dev, bar_reg, 0xFFFFFFFF);
> > -    addr = qpci_config_readl(dev, bar_reg);
> > -
> > -    io_type = addr & PCI_BASE_ADDRESS_SPACE;
> > -    if (io_type == PCI_BASE_ADDRESS_SPACE_IO) {
> > -        addr &= PCI_BASE_ADDRESS_IO_MASK;
> > -    } else {
> > -        addr &= PCI_BASE_ADDRESS_MEM_MASK;
> > -    }
> > -
> > -    size = (1ULL << ctzl(addr));
> > -    if (size == 0) {
> > -        return NULL;
> > -    }
> > -    if (sizeptr) {
> > -        *sizeptr = size;
> > -    }
> > -
> > -    if (io_type == PCI_BASE_ADDRESS_SPACE_IO) {
> > -        uint16_t loc;
> > -
> > -        g_assert(QEMU_ALIGN_UP(s->pci_iohole_alloc, size) + size
> > -                 <= s->pci_iohole_size);
> > -        s->pci_iohole_alloc = QEMU_ALIGN_UP(s->pci_iohole_alloc, size);
> > -        loc = s->pci_iohole_start + s->pci_iohole_alloc;
> > -        s->pci_iohole_alloc += size;
> > -
> > -        qpci_config_writel(dev, bar_reg, loc | PCI_BASE_ADDRESS_SPACE_IO);
> > -
> > -        return (void *)(unsigned long)loc;
> > -    } else {
> > -        uint64_t loc;
> > -
> > -        g_assert(QEMU_ALIGN_UP(s->pci_hole_alloc, size) + size
> > -                 <= s->pci_hole_size);
> > -        s->pci_hole_alloc = QEMU_ALIGN_UP(s->pci_hole_alloc, size);
> > -        loc = s->pci_hole_start + s->pci_hole_alloc;
> > -        s->pci_hole_alloc += size;
> > -
> > -        qpci_config_writel(dev, bar_reg, loc);
> > -
> > -        return (void *)(unsigned long)loc;
> > -    }
> > -}
> > -
> > -static void qpci_spapr_iounmap(QPCIBus *bus, void *data)
> > -{
> > -    /* FIXME */
> > -}
> > -
> >  #define SPAPR_PCI_BASE               (1ULL << 45)
> >  
> >  #define SPAPR_PCI_MMIO32_WIN_SIZE    0x80000000 /* 2 GiB */
> > @@ -270,9 +204,6 @@ QPCIBus *qpci_init_spapr(QGuestAllocator *alloc)
> >      ret->bus.config_writew = qpci_spapr_config_writew;
> >      ret->bus.config_writel = qpci_spapr_config_writel;
> >  
> > -    ret->bus.iomap = qpci_spapr_iomap;
> > -    ret->bus.iounmap = qpci_spapr_iounmap;
> > -
> >      /* FIXME: We assume the default location of the PHB for now.
> >       * Ideally we'd parse the device tree deposited in the guest to
> >       * get the window locations */
> > @@ -287,15 +218,9 @@ QPCIBus *qpci_init_spapr(QGuestAllocator *alloc)
> >      ret->mmio32.pci_base = 0x80000000; /* 2 GiB */
> >      ret->mmio32.size = SPAPR_PCI_MMIO32_WIN_SIZE;
> >  
> > -    ret->pci_hole_start = 0xC0000000;
> > -    ret->pci_hole_size =
> > -        ret->mmio32.pci_base + ret->mmio32.size - ret->pci_hole_start;
> > -    ret->pci_hole_alloc = 0;
> > -
> > -    ret->pci_iohole_start = 0xc000;
> > -    ret->pci_iohole_size =
> > -        ret->pio.pci_base + ret->pio.size - ret->pci_iohole_start;
> > -    ret->pci_iohole_alloc = 0;
> 
> I think you can remove all these fields (pci_hole_.., pci_iohole_...)
> from QPCIBusSPAPR.

Ah, yes, meant to remove them from both PC and spapr, but forgot.
Done for the next spin.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature