Re: [Qemu-devel] [PATCH 04/38] tests/libqos/pci-pc: Fix qpci_pc_iomap() to map BARs aligned

[Marc-André Lureau]

On Mon, Feb 29, 2016 at 7:40 PM, Markus Armbruster <address@hidden> wrote:
> qpci_pc_iomap() maps BARs one after the other, without padding.  This
> is wrong.  PCI Local Bus Specification Revision 3.0, 6.2.5.1. Address
> Maps: "all address spaces used are a power of two in size and are
> naturally aligned".  That's because the size of a BAR is given by the
> number of address bits the device decodes, and the BAR needs to be
> mapped at a multiple of that size to ensure the address decoding
> works.
>
> Fix qpci_pc_iomap() accordingly.  This takes care of a FIXME in
> ivshmem-test.
>
> Signed-off-by: Markus Armbruster <address@hidden>

Reviewed-by: Marc-André Lureau <address@hidden>

Neat, thanks for fixing my fixme ;)

> ---
>  tests/ivshmem-test.c  | 17 ++++++++---------
>  tests/libqos/pci-pc.c |  8 ++++++--
>  2 files changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
> index e184c67..e118377 100644
> --- a/tests/ivshmem-test.c
> +++ b/tests/ivshmem-test.c
> @@ -110,19 +110,18 @@ static void setup_vm_cmd(IVState *s, const char *cmd, 
> bool msix)
>      s->pcibus = qpci_init_pc();
>      s->dev = get_device(s->pcibus);
>
> -    /* FIXME: other bar order fails, mappings changes */
> -    s->mem_base = qpci_iomap(s->dev, 2, &barsize);
> -    g_assert_nonnull(s->mem_base);
> -    g_assert_cmpuint(barsize, ==, TMPSHMSIZE);
> -
> -    if (msix) {
> -        qpci_msix_enable(s->dev);
> -    }
> -
>      s->reg_base = qpci_iomap(s->dev, 0, &barsize);
>      g_assert_nonnull(s->reg_base);
>      g_assert_cmpuint(barsize, ==, 256);
>
> +    if (msix) {
> +        qpci_msix_enable(s->dev);
> +    }
> +
> +    s->mem_base = qpci_iomap(s->dev, 2, &barsize);
> +    g_assert_nonnull(s->mem_base);
> +    g_assert_cmpuint(barsize, ==, TMPSHMSIZE);
> +
>      qpci_device_enable(s->dev);
>  }
>
> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> index 08167c0..77f15e5 100644
> --- a/tests/libqos/pci-pc.c
> +++ b/tests/libqos/pci-pc.c
> @@ -184,7 +184,9 @@ static void *qpci_pc_iomap(QPCIBus *bus, QPCIDevice *dev, 
> int barno, uint64_t *s
>      if (io_type == PCI_BASE_ADDRESS_SPACE_IO) {
>          uint16_t loc;
>
> -        g_assert((s->pci_iohole_alloc + size) <= s->pci_iohole_size);
> +        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;
>
> @@ -194,7 +196,9 @@ static void *qpci_pc_iomap(QPCIBus *bus, QPCIDevice *dev, 
> int barno, uint64_t *s
>      } else {
>          uint64_t loc;
>
> -        g_assert((s->pci_hole_alloc + size) <= s->pci_hole_size);
> +        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;
>
> --
> 2.4.3
>
>



-- 
Marc-André Lureau