From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49859) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZEuAc-00025P-Li for qemu-devel@nongnu.org; Tue, 14 Jul 2015 02:58:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZEuAZ-0005f6-Dp for qemu-devel@nongnu.org; Tue, 14 Jul 2015 02:58:26 -0400 Received: from mail-pd0-f175.google.com ([209.85.192.175]:36603) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZEuAZ-0005br-68 for qemu-devel@nongnu.org; Tue, 14 Jul 2015 02:58:23 -0400 Received: by pdjr16 with SMTP id r16so723021pdj.3 for ; Mon, 13 Jul 2015 23:58:22 -0700 (PDT) References: <1436799381-16150-1-git-send-email-aik@ozlabs.ru> <1436799381-16150-2-git-send-email-aik@ozlabs.ru> <1436814815.1391.388.camel@redhat.com> From: Alexey Kardashevskiy Message-ID: <55A4B306.6040402@ozlabs.ru> Date: Tue, 14 Jul 2015 16:58:14 +1000 MIME-Version: 1.0 In-Reply-To: <1436814815.1391.388.camel@redhat.com> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH qemu v2 1/5] vfio: Switch from TARGET_PAGE_MASK to qemu_real_host_page_mask List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: Michael Roth , qemu-ppc@nongnu.org, qemu-devel@nongnu.org, David Gibson On 07/14/2015 05:13 AM, Alex Williamson wrote: > On Tue, 2015-07-14 at 00:56 +1000, Alexey Kardashevskiy wrote: >> These started switching from TARGET_PAGE_MASK (hardcoded as 4K) to >> a real host page size: >> 4e51361d7 "cpu-all: complete "real" host page size API" and >> f7ceed190 "vfio: cpu: Use "real" page size API" >> >> This finished the transition by: >> - %s/TARGET_PAGE_MASK/qemu_real_host_page_mask/ >> - %s/TARGET_PAGE_ALIGN/REAL_HOST_PAGE_ALIGN/ >> - removing bitfield length for offsets in VFIOQuirk::data as >> qemu_real_host_page_mask is not a macro > > Can we assume that none of the changes to quirks have actually been > tested? No, why? :) I tried it on one of NVIDIAs I got here - VGA compatible controller: NVIDIA Corporation GM107GL [Quadro K2200] (rev a2) The driver was from NVIDIA (not nouveau) and the test was "acos" (some basic CUDA test). > I don't really support them being bundled in here since they > really aren't related to what you're doing. This makes sense, I'll move them to a separate patch and add a note how it helps on a 64k-pages host. > For DMA we generally want > to be host IOMMU page aligned, Do all known IOMMUs use a constant page size? IOMMU memory region does not have an IOMMU page size/mask and I wanted to add it there but not sure if it is generic enough. > which we can generally assume is the same > as host page aligned, They are almost never the same on sPAPR for 32bit windows... > but quirks are simply I/O regions, so I think they > ought to continue to be target page aligned. Without s/TARGET_PAGE_MASK/qemu_real_host_page_mask/, &vfio_nvidia_88000_quirk fails+exits in kvm_set_phys_mem() as the size of section is 0x88000. It still works with x-mmap=false (or TCG, I suppose) though. >> This keeps using TARGET_PAGE_MASK for IOMMU regions though as it is >> the minimum page size which IOMMU regions may be using and at the moment >> memory regions do not carry the actual page size. >> >> Signed-off-by: Alexey Kardashevskiy >> --- >> >> In reality DMA windows are always a lot bigger than a single 4K page >> and aligned to 32/64MB, may be only use here qemu_real_host_page_mask? > > I don't understand what this is asking either. While the bulk of memory > is going to be mapped in larger chunks, we do occasionally see 4k > mappings on x86, particularly in some of the legacy low memory areas. The question was not about individual mappings - these are handled by a iommu memory region notifier; here we are dealing with DMA windows which are always megabytes but nothing really prevents/prohibits a guest from requesting a 4K _window_ (with a single TCE entry). Whether we want to support such small windows or not - this was a question. > >> --- >> hw/vfio/common.c | 26 ++++++++++++++++---------- >> hw/vfio/pci.c | 30 +++++++++++++++--------------- >> 2 files changed, 31 insertions(+), 25 deletions(-) >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 85ee9b0..d115ec9 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -321,6 +321,9 @@ static void vfio_listener_region_add(MemoryListener *listener, >> Int128 llend; >> void *vaddr; >> int ret; >> + const bool is_iommu = memory_region_is_iommu(section->mr); >> + const hwaddr page_mask = >> + is_iommu ? TARGET_PAGE_MASK : qemu_real_host_page_mask; >> >> if (vfio_listener_skipped_section(section)) { >> trace_vfio_listener_region_add_skip( >> @@ -330,16 +333,16 @@ static void vfio_listener_region_add(MemoryListener *listener, >> return; >> } >> >> - if (unlikely((section->offset_within_address_space & ~TARGET_PAGE_MASK) != >> - (section->offset_within_region & ~TARGET_PAGE_MASK))) { >> + if (unlikely((section->offset_within_address_space & ~page_mask) != >> + (section->offset_within_region & ~page_mask))) { >> error_report("%s received unaligned region", __func__); >> return; >> } >> >> - iova = TARGET_PAGE_ALIGN(section->offset_within_address_space); >> + iova = ROUND_UP(section->offset_within_address_space, ~page_mask + 1); >> llend = int128_make64(section->offset_within_address_space); >> llend = int128_add(llend, section->size); >> - llend = int128_and(llend, int128_exts64(TARGET_PAGE_MASK)); >> + llend = int128_and(llend, int128_exts64(page_mask)); >> >> if (int128_ge(int128_make64(iova), llend)) { >> return; >> @@ -347,7 +350,7 @@ static void vfio_listener_region_add(MemoryListener *listener, >> >> memory_region_ref(section->mr); >> >> - if (memory_region_is_iommu(section->mr)) { >> + if (is_iommu) { >> VFIOGuestIOMMU *giommu; >> >> trace_vfio_listener_region_add_iommu(iova, >> @@ -423,6 +426,9 @@ static void vfio_listener_region_del(MemoryListener *listener, >> iommu_data.type1.listener); >> hwaddr iova, end; >> int ret; >> + const bool is_iommu = memory_region_is_iommu(section->mr); >> + const hwaddr page_mask = >> + is_iommu ? TARGET_PAGE_MASK : qemu_real_host_page_mask; >> >> if (vfio_listener_skipped_section(section)) { >> trace_vfio_listener_region_del_skip( >> @@ -432,13 +438,13 @@ static void vfio_listener_region_del(MemoryListener *listener, >> return; >> } >> >> - if (unlikely((section->offset_within_address_space & ~TARGET_PAGE_MASK) != >> - (section->offset_within_region & ~TARGET_PAGE_MASK))) { >> + if (unlikely((section->offset_within_address_space & ~page_mask) != >> + (section->offset_within_region & ~page_mask))) { >> error_report("%s received unaligned region", __func__); >> return; >> } >> >> - if (memory_region_is_iommu(section->mr)) { >> + if (is_iommu) { >> VFIOGuestIOMMU *giommu; >> >> QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) { >> @@ -459,9 +465,9 @@ static void vfio_listener_region_del(MemoryListener *listener, >> */ >> } >> >> - iova = TARGET_PAGE_ALIGN(section->offset_within_address_space); >> + iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space); >> end = (section->offset_within_address_space + int128_get64(section->size)) & >> - TARGET_PAGE_MASK; >> + page_mask; >> >> if (iova >= end) { >> return; >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index 2ed877f..7694afe 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -50,16 +50,16 @@ typedef struct VFIOQuirk { >> struct VFIOPCIDevice *vdev; >> QLIST_ENTRY(VFIOQuirk) next; >> struct { >> - uint32_t base_offset:TARGET_PAGE_BITS; >> - uint32_t address_offset:TARGET_PAGE_BITS; >> + uint32_t base_offset; >> + uint32_t address_offset; >> uint32_t address_size:3; >> uint32_t bar:3; >> >> uint32_t address_match; >> uint32_t address_mask; >> >> - uint32_t address_val:TARGET_PAGE_BITS; >> - uint32_t data_offset:TARGET_PAGE_BITS; >> + uint32_t address_val; >> + uint32_t data_offset; >> uint32_t data_size:3; >> >> uint8_t flags; >> @@ -1319,8 +1319,8 @@ static uint64_t vfio_generic_quirk_read(void *opaque, >> { >> VFIOQuirk *quirk = opaque; >> VFIOPCIDevice *vdev = quirk->vdev; >> - hwaddr base = quirk->data.address_match & TARGET_PAGE_MASK; >> - hwaddr offset = quirk->data.address_match & ~TARGET_PAGE_MASK; >> + hwaddr base = quirk->data.address_match & qemu_real_host_page_mask; >> + hwaddr offset = quirk->data.address_match & ~qemu_real_host_page_mask; >> uint64_t data; >> >> if (vfio_flags_enabled(quirk->data.flags, quirk->data.read_flags) && >> @@ -1349,8 +1349,8 @@ static void vfio_generic_quirk_write(void *opaque, hwaddr addr, >> { >> VFIOQuirk *quirk = opaque; >> VFIOPCIDevice *vdev = quirk->vdev; >> - hwaddr base = quirk->data.address_match & TARGET_PAGE_MASK; >> - hwaddr offset = quirk->data.address_match & ~TARGET_PAGE_MASK; >> + hwaddr base = quirk->data.address_match & qemu_real_host_page_mask; >> + hwaddr offset = quirk->data.address_match & ~qemu_real_host_page_mask; >> >> if (vfio_flags_enabled(quirk->data.flags, quirk->data.write_flags) && >> ranges_overlap(addr, size, offset, quirk->data.address_mask + 1)) { >> @@ -1650,9 +1650,9 @@ static void vfio_probe_ati_bar2_4000_quirk(VFIOPCIDevice *vdev, int nr) >> >> memory_region_init_io(&quirk->mem, OBJECT(vdev), &vfio_generic_quirk, quirk, >> "vfio-ati-bar2-4000-quirk", >> - TARGET_PAGE_ALIGN(quirk->data.address_mask + 1)); >> + REAL_HOST_PAGE_ALIGN(quirk->data.address_mask + 1)); >> memory_region_add_subregion_overlap(&vdev->bars[nr].region.mem, >> - quirk->data.address_match & TARGET_PAGE_MASK, >> + quirk->data.address_match & qemu_real_host_page_mask, >> &quirk->mem, 1); >> >> QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next); >> @@ -1888,7 +1888,7 @@ static void vfio_nvidia_88000_quirk_write(void *opaque, hwaddr addr, >> VFIOQuirk *quirk = opaque; >> VFIOPCIDevice *vdev = quirk->vdev; >> PCIDevice *pdev = &vdev->pdev; >> - hwaddr base = quirk->data.address_match & TARGET_PAGE_MASK; >> + hwaddr base = quirk->data.address_match & qemu_real_host_page_mask; >> >> vfio_generic_quirk_write(opaque, addr, data, size); >> >> @@ -1943,9 +1943,9 @@ static void vfio_probe_nvidia_bar0_88000_quirk(VFIOPCIDevice *vdev, int nr) >> >> memory_region_init_io(&quirk->mem, OBJECT(vdev), &vfio_nvidia_88000_quirk, >> quirk, "vfio-nvidia-bar0-88000-quirk", >> - TARGET_PAGE_ALIGN(quirk->data.address_mask + 1)); >> + REAL_HOST_PAGE_ALIGN(quirk->data.address_mask + 1)); >> memory_region_add_subregion_overlap(&vdev->bars[nr].region.mem, >> - quirk->data.address_match & TARGET_PAGE_MASK, >> + quirk->data.address_match & qemu_real_host_page_mask, >> &quirk->mem, 1); >> >> QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next); >> @@ -1980,9 +1980,9 @@ static void vfio_probe_nvidia_bar0_1800_quirk(VFIOPCIDevice *vdev, int nr) >> >> memory_region_init_io(&quirk->mem, OBJECT(vdev), &vfio_generic_quirk, quirk, >> "vfio-nvidia-bar0-1800-quirk", >> - TARGET_PAGE_ALIGN(quirk->data.address_mask + 1)); >> + REAL_HOST_PAGE_ALIGN(quirk->data.address_mask + 1)); >> memory_region_add_subregion_overlap(&vdev->bars[nr].region.mem, >> - quirk->data.address_match & TARGET_PAGE_MASK, >> + quirk->data.address_match & qemu_real_host_page_mask, >> &quirk->mem, 1); >> >> QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next); > > > -- Alexey