Re: [Qemu-devel] [PATCH PULL v2 09/10] hw/rdma: Implementation of PVRDMA device

[Peter Maydell]

On 19 February 2018 at 11:43, Marcel Apfelbaum <address@hidden> wrote:
> From: Yuval Shaia <address@hidden>
>
> PVRDMA is the QEMU implementation of VMware's paravirtualized RDMA device.
> It works with its Linux Kernel driver AS IS, no need for any special
> guest modifications.
>
> While it complies with the VMware device, it can also communicate with
> bare metal RDMA-enabled machines and does not require an RDMA HCA in the
> host, it can work with Soft-RoCE (rxe).
>
> It does not require the whole guest RAM to be pinned allowing memory
> over-commit and, even if not implemented yet, migration support will be
> possible with some HW assistance.
>
> Implementation is divided into 2 components, rdma general and pvRDMA
> specific functions and structures.
>
> The second PVRDMA sub-module - interaction with PCI layer.
> - Device configuration and setup (MSIX, BARs etc).
> - Setup of DSR (Device Shared Resources)
> - Setup of device ring.
> - Device management.
>
> Reviewed-by: Dotan Barak <address@hidden>
> Reviewed-by: Zhu Yanjun <address@hidden>
> Signed-off-by: Yuval Shaia <address@hidden>
> Signed-off-by: Marcel Apfelbaum <address@hidden>

Hi; this is something odd I noticed reading through the code.

> +static void init_bars(PCIDevice *pdev)
> +{
> +    PVRDMADev *dev = PVRDMA_DEV(pdev);
> +
> +    /* BAR 0 - MSI-X */
> +    memory_region_init(&dev->msix, OBJECT(dev), "pvrdma-msix",
> +                       RDMA_BAR0_MSIX_SIZE);
> +    pci_register_bar(pdev, RDMA_MSIX_BAR_IDX, PCI_BASE_ADDRESS_SPACE_MEMORY,
> +                     &dev->msix);
> +
> +    /* BAR 1 - Registers */
> +    memset(&dev->regs_data, 0, sizeof(dev->regs_data));
> +    memory_region_init_io(&dev->regs, OBJECT(dev), &regs_ops, dev,
> +                          "pvrdma-regs", RDMA_BAR1_REGS_SIZE);

RDMA_BAR1_REGS_SIZE is used in pvrdma.h to declare the regs array:
    uint32_t regs_data[RDMA_BAR1_REGS_SIZE];
and that and the way that get_reg_val/set_reg_val do "addr >> 2" to
convert from an address to an array index suggest that it is the
size of the BAR in 32-bit words. However here we're passing it
as the size parameter of memory_region_init_io(), which wants a
size in bytes. Which is correct ?

thanks
-- PMM