From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37932) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aY2S3-0005zN-3d for qemu-devel@nongnu.org; Mon, 22 Feb 2016 21:11:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aY2Ry-0003da-Al for qemu-devel@nongnu.org; Mon, 22 Feb 2016 21:11:47 -0500 Received: from mail-pf0-x22c.google.com ([2607:f8b0:400e:c00::22c]:36625) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aY2Rx-0003d6-AL for qemu-devel@nongnu.org; Mon, 22 Feb 2016 21:11:42 -0500 Received: by mail-pf0-x22c.google.com with SMTP id e127so103035220pfe.3 for ; Mon, 22 Feb 2016 18:11:41 -0800 (PST) References: <1456121379-13434-1-git-send-email-aik@ozlabs.ru> <20160222062631.GH2808@voom.fritz.box> <56CAF2B3.2030502@ozlabs.ru> <20160222121227.GN2808@voom.fritz.box> From: Alexey Kardashevskiy Message-ID: <56CBBFD7.3050806@ozlabs.ru> Date: Tue, 23 Feb 2016 13:11:35 +1100 MIME-Version: 1.0 In-Reply-To: <20160222121227.GN2808@voom.fritz.box> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH qemu] memory: Fix IOMMU replay base address List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: qemu-devel@nongnu.org On 02/22/2016 11:12 PM, David Gibson wrote: > On Mon, Feb 22, 2016 at 10:36:19PM +1100, Alexey Kardashevskiy wrote: >> On 02/22/2016 05:26 PM, David Gibson wrote: >>> On Mon, Feb 22, 2016 at 05:09:39PM +1100, Alexey Kardashevskiy wrote: >>>> Since a788f227 "memory: Allow replay of IOMMU mapping notifications" >>>> when new VFIO listener is added, all existing IOMMU mappings are replayed. >>>> However there is a problem that the base address of an IOMMU memory region >>>> (IOMMU MR) is ignored which is not a problem for the existing user (which is >>>> pseries) with its default 32bit DMA window starting at 0 but it is if there is >>>> another DMA window. >>>> >>>> This adjusts the replaying address by mr->addr. >>> >>> Uh.. this doesn't look right to me. AFAICT from the existing >>> implementations the 'addr' parameter to the translate function is an >>> offset within the memory region, which would make the original version >>> correct. >> >> Ok, then spapr_tce_translate_iommu() needs to be fixed. Or I am missing >> something here? The @addr field is definitely ignored now. > > I think at least one of us is missing something. In the normal AS > translation path, IIUC, it will subtract the mr->addr value to give > offsets which are passed to translate. It uses those offsets to find > the TCE and returns a translated address. Where else the @addr be > used? Ok, this patch is definitely wrong. The actual problem I am debugging is this: 1. run guest with an emulated XHCI to have 2 DMA windows; 2. hotplug vfio-pci device. 3. observe failing vfio_dma_map(). This triggers memory_region_register_iommu_notifier() + memory_region_iommu_replay() which calls vfio_iommu_map_notify() in a loop. For the second window (huge one, 0x800000000000000 bus address) it fails as iotlb.iova's returned by spapr_tce_translate_iommu() are offsets within IOMMU MR. But vfio_dma_map() (which is eventually called by vfio_iommu_map_notify()) needs an absolute iotlb->iova address. imho this should be correct: diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c index 2e02dc6..0d6b926 100644 --- a/hw/ppc/spapr_iommu.c +++ b/hw/ppc/spapr_iommu.c @@ -126,7 +126,7 @@ static IOMMUTLBEntry spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr, hwaddr page_mask = IOMMU_PAGE_MASK(tcet->page_shift); tce = tcet->table[addr >> tcet->page_shift]; - ret.iova = addr & page_mask; + ret.iova = (addr + iommu->addr) & page_mask; ret.translated_addr = tce & page_mask; However this does not help either. Because at the moment I create hardware windows from a helper which is called from spapr_phb_hot_plug_child() (HotplugHandlerClass::plug() callback). And this is too late as pci_qdev_realize("vfio-pci") calls: vfio_connect_container -> ... -> vfio_listener_region_add -> .. -> memory_region_iommu_replay and the latter fails - there is no huge window in the host kernel yet. What is the right way of fixing this?... This memory_region_iommu_replay() thing really creates problems for me :-/ > > >> >> >>> >>>> Signed-off-by: Alexey Kardashevskiy >>>> --- >>>> memory.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/memory.c b/memory.c >>>> index 09041ed..377269b 100644 >>>> --- a/memory.c >>>> +++ b/memory.c >>>> @@ -1436,7 +1436,7 @@ void memory_region_iommu_replay(MemoryRegion *mr, Notifier *n, >>>> IOMMUTLBEntry iotlb; >>>> >>>> for (addr = 0; addr < memory_region_size(mr); addr += granularity) { >>>> - iotlb = mr->iommu_ops->translate(mr, addr, is_write); >>>> + iotlb = mr->iommu_ops->translate(mr, mr->addr + addr, is_write); >>>> if (iotlb.perm != IOMMU_NONE) { >>>> n->notify(n, &iotlb); >>>> } >>> >> >> > -- Alexey