From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54106) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VC10u-0005Tt-HH for qemu-devel@nongnu.org; Wed, 21 Aug 2013 01:31:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VC10i-0005T1-TG for qemu-devel@nongnu.org; Wed, 21 Aug 2013 01:31:24 -0400 Received: from mail-pa0-f47.google.com ([209.85.220.47]:33499) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VC10i-0005SV-JJ for qemu-devel@nongnu.org; Wed, 21 Aug 2013 01:31:12 -0400 Received: by mail-pa0-f47.google.com with SMTP id kl13so400507pab.6 for ; Tue, 20 Aug 2013 22:31:10 -0700 (PDT) Message-ID: <52145097.9050808@ozlabs.ru> Date: Wed, 21 Aug 2013 15:31:03 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1375863692-12207-1-git-send-email-aik@ozlabs.ru> <1375863692-12207-6-git-send-email-aik@ozlabs.ru> <1376345237.22121.15.camel@ul30vt.home> <520C6EE1.8020503@ozlabs.ru> <521219B6.9010108@redhat.com> In-Reply-To: <521219B6.9010108@redhat.com> Content-Type: text/plain; charset=KOI8-R Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/8] vfio: Add guest side IOMMU support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Anthony Liguori , "Michael S . Tsirkin" , qemu-devel@nongnu.org, Alexander Graf , Alex Williamson , qemu-ppc@nongnu.org, Paul Mackerras , David Gibson On 08/19/2013 11:12 PM, Paolo Bonzini wrote: > Il 15/08/2013 08:02, Alexey Kardashevskiy ha scritto: >> On 08/13/2013 08:07 AM, Alex Williamson wrote: >>>> +static void vfio_listener_region_add(MemoryListener *listener, >>>> + MemoryRegionSection *section) >>>> +{ >>>> + VFIOContainer *container = container_of(listener, VFIOContainer, >>>> + iommu_data.listener); >>>> + hwaddr iova, end; >>>> + int ret; >>>> >>>> if (vfio_listener_skipped_section(section)) { >>>> DPRINTF("SKIPPING region_add %"HWADDR_PRIx" - %"PRIx64"\n", >>>> @@ -1952,19 +2011,51 @@ static void vfio_listener_region_add(MemoryListener *listener, >>>> return; >>>> } >>>> >>>> - vaddr = memory_region_get_ram_ptr(section->mr) + >>>> - section->offset_within_region + >>>> - (iova - section->offset_within_address_space); >>>> - >>>> - DPRINTF("region_add %"HWADDR_PRIx" - %"HWADDR_PRIx" [%p]\n", >>>> - iova, end - 1, vaddr); >>>> - >>>> - memory_region_ref(section->mr); >>>> - ret = vfio_dma_map(container, iova, end - iova, vaddr, section->readonly); >>>> - if (ret) { >>>> - error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", " >>>> - "0x%"HWADDR_PRIx", %p) = %d (%m)", >>>> - container, iova, end - iova, vaddr, ret); >>>> + if (memory_region_is_iommu(section->mr)) { >>>> + VFIOGuestIOMMU *giommu; >>>> + >>>> + DPRINTF("region_add [iommu] %"HWADDR_PRIx" - %"HWADDR_PRIx"\n", >>>> + iova, end - 1); >>>> + >>>> + memory_region_ref(section->mr); >>>> + /* >>>> + * FIXME: We should do some checking to see if the >>>> + * capabilities of the host VFIO IOMMU are adequate to model >>>> + * the guest IOMMU >>>> + * >>>> + * FIXME: This assumes that the guest IOMMU is empty of >>>> + * mappings at this point - we should either enforce this, or >>>> + * loop through existing mappings to map them into VFIO. >>>> + * >>>> + * FIXME: For VFIO iommu types which have KVM acceleration to >>>> + * avoid bouncing all map/unmaps through qemu this way, this >>>> + * would be the right place to wire that up (tell the KVM >>>> + * device emulation the VFIO iommu handles to use). >>>> + */ >>>> + giommu = g_malloc0(sizeof(*giommu)); >>>> + giommu->iommu = section->mr; >>>> + giommu->container = container; >>>> + giommu->n.notify = vfio_iommu_map_notify; >>>> + QLIST_INSERT_HEAD(&container->guest_iommus, giommu, list); >>>> + memory_region_register_iommu_notifier(giommu->iommu, &giommu->n); >>>> + >>>> + } else if (memory_region_is_ram(section->mr)) { > > Please change this to an "else", and leave the refs outside the if, just > after checking for vfio_listener_skipped_section. Why right after vfio_listener_skipped_section? The whole block looks now as: === static void vfio_listener_region_add(MemoryListener *listener, MemoryRegionSection *section) { VFIOContainer *container = container_of(listener, VFIOContainer, iommu_data.listener); hwaddr iova, end; void *vaddr; int ret; if (vfio_listener_skipped_section(section)) { DPRINTF("SKIPPING region_add %"HWADDR_PRIx" - %"PRIx64"\n", section->offset_within_address_space, section->offset_within_address_space + section->size - 1); return; } if (unlikely((section->offset_within_address_space & ~TARGET_PAGE_MASK) != (section->offset_within_region & ~TARGET_PAGE_MASK))) { error_report("%s received unaligned region", __func__); return; } iova = TARGET_PAGE_ALIGN(section->offset_within_address_space); end = (section->offset_within_address_space + int128_get64(section->size)) & TARGET_PAGE_MASK; if (iova >= end) { return; } memory_region_ref(section->mr); === You want me to move memory_region_ref() earlier to add a reference even if two last checks fail? > This way it's clearer > that vfio_listener_region_add matches vfio_listener_region_del. Yes, true, reworked. Thanks for comments! -- Alexey