From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38165) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VErwz-0002sk-HB for qemu-devel@nongnu.org; Wed, 28 Aug 2013 22:27:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VErwt-00024j-Bz for qemu-devel@nongnu.org; Wed, 28 Aug 2013 22:27:09 -0400 Received: from mail-pa0-f52.google.com ([209.85.220.52]:40870) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VErwt-00024f-5E for qemu-devel@nongnu.org; Wed, 28 Aug 2013 22:27:03 -0400 Received: by mail-pa0-f52.google.com with SMTP id kq13so261595pab.25 for ; Wed, 28 Aug 2013 19:27:02 -0700 (PDT) Message-ID: <521EB16F.9010303@ozlabs.ru> Date: Thu, 29 Aug 2013 12:26:55 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1377170965-9905-1-git-send-email-aik@ozlabs.ru> <1377170965-9905-4-git-send-email-aik@ozlabs.ru> <1377703099.10408.71.camel@ul30vt.home> <521E9DF5.9080709@ozlabs.ru> <1377740545.10408.145.camel@ul30vt.home> In-Reply-To: <1377740545.10408.145.camel@ul30vt.home> Content-Type: text/plain; charset=KOI8-R Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 3/3] vfio: Fix 128 bit handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: Paolo Bonzini , qemu-devel@nongnu.org, Peter Maydell On 08/29/2013 11:42 AM, Alex Williamson wrote: > On Thu, 2013-08-29 at 11:03 +1000, Alexey Kardashevskiy wrote: >> On 08/29/2013 01:18 AM, Alex Williamson wrote: >>> On Thu, 2013-08-22 at 21:29 +1000, Alexey Kardashevskiy wrote: >>>> Upcoming VFIO on SPAPR PPC64 support will initialize the IOMMU >>>> memory region with UINT64_MAX (2^64 bytes) size so int128_get64() >>>> will assert. >>>> >>>> The patch takes care of this check. The existing type1 IOMMU code >>>> is not expected to map all 64 bits of RAM so the patch does not >>>> touch that part. >>>> >>>> Signed-off-by: Alexey Kardashevskiy >>>> --- >>>> Changes: >>>> v2: >>>> * used new function int128_exts64() >>>> --- >>>> hw/misc/vfio.c | 11 ++++++++--- >>>> 1 file changed, 8 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c >>>> index dfe3a80..3878fc7 100644 >>>> --- a/hw/misc/vfio.c >>>> +++ b/hw/misc/vfio.c >>>> @@ -1920,6 +1920,7 @@ static void vfio_listener_region_add(MemoryListener *listener, >>>> VFIOContainer *container = container_of(listener, VFIOContainer, >>>> iommu_data.listener); >>>> hwaddr iova, end; >>>> + Int128 llend; >>>> void *vaddr; >>>> int ret; >>>> >>>> @@ -1940,13 +1941,17 @@ static void vfio_listener_region_add(MemoryListener *listener, >>>> } >>>> >>>> iova = TARGET_PAGE_ALIGN(section->offset_within_address_space); >>>> - end = (section->offset_within_address_space + int128_get64(section->size)) & >>>> - TARGET_PAGE_MASK; >>>> + llend = int128_make64(section->offset_within_address_space); >>>> + llend = int128_add(llend, section->size); >>>> + llend = int128_and(llend, int128_exts64(TARGET_PAGE_MASK)); >>>> >>>> - if (iova >= end) { >>>> + if (int128_ge(int128_make64(iova), llend)) { >>>> return; >>>> } >>>> >>>> + end = (section->offset_within_address_space + int128_get64(section->size)) & >>>> + TARGET_PAGE_MASK; >>>> + >>> >>> I'm confused, we build an Int128 version of end above for the >>> comparison, why isn't this just: >>> >>> end = int128_get64(llend); >> >> >> section->size for IOMMU memory region I have on spapr-vfio is 2^64 so >> int128_get64() fails. > > But you're leaving code that does int128_get64(section->size)... how > does that not assert? Right. I was planning to add my IOMMU stuff right before calculating @end. > This patch is not maintainable. We're seemingly > calculating the same value twice with no comment as to why. A hwaddr > type end should be calculated from the Int128 rather than paying > attention to rollover in one place but not another. Thanks, Yes, not maintainable... I guess I just have to convert @end to 128 bit as well to have things consistent. -- Alexey