From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55406) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fC9Al-0003Hc-AZ for qemu-devel@nongnu.org; Fri, 27 Apr 2018 15:36:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fC9Ai-0005sb-6u for qemu-devel@nongnu.org; Fri, 27 Apr 2018 15:36:47 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:35104 helo=mx1.redhat.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fC9Ai-0005s5-13 for qemu-devel@nongnu.org; Fri, 27 Apr 2018 15:36:44 -0400 References: <20180219114332.70443-1-marcel@redhat.com> <20180219114332.70443-10-marcel@redhat.com> From: Marcel Apfelbaum Message-ID: <8659dc43-a5a2-f4ca-3381-8698a1a06ad3@redhat.com> Date: Fri, 27 Apr 2018 22:36:33 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH PULL v2 09/10] hw/rdma: Implementation of PVRDMA device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell , Yuval Shaia Cc: QEMU Developers On 27/04/2018 17:49, Peter Maydell wrote: > On 19 February 2018 at 11:43, Marcel Apfelbaum wrote: >> From: Yuval Shaia >> >> 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 >> Reviewed-by: Zhu Yanjun >> Signed-off-by: Yuval Shaia >> Signed-off-by: Marcel Apfelbaum > > 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), ®s_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 ? > I think that RDMA_BAR1_REGS_SIZE (256) is the size in bytes, this is the layout of the registers (linux kernel): /* Register offsets within PCI resource on BAR1. */ #define PVRDMA_REG_VERSION 0x00 /* R: Version of device. */ #define PVRDMA_REG_DSRLOW 0x04 /* W: Device shared region low PA. */ #define PVRDMA_REG_DSRHIGH 0x08 /* W: Device shared region high PA. */ #define PVRDMA_REG_CTL 0x0c /* W: PVRDMA_DEVICE_CTL */ #define PVRDMA_REG_REQUEST 0x10 /* W: Indicate device request. */ #define PVRDMA_REG_ERR 0x14 /* R: Device error. */ #define PVRDMA_REG_ICR 0x18 /* R: Interrupt cause. */ #define PVRDMA_REG_IMR 0x1c /* R/W: Interrupt mask. */ #define PVRDMA_REG_MACL 0x20 /* R/W: MAC address low. */ #define PVRDMA_REG_MACH 0x24 /* R/W: MAC address high. */ So we don't need 256 32-bit words. Yuval can you please confirm? Thanks, Marcel > thanks > -- PMM >