[Qemu-devel] [PATCH 0/2] vfio: fixes for better support for 128 bit memory section sizes

[Qemu-devel] [PATCH 0/2] vfio: fixes for better support for 128 bit memory section sizes

[Alexey Kardashevskiy]

I made a couple of small patches while debugging VFIO on SPAPR
which uses IOMMU MemoryRegion 2^64 bytes long.

I was specifically fixing faults with the 2^64 bytes case while there
may be other proglems.


Alexey Kardashevskiy (2):
  vfio: Fix debug output for int128 values
  vfio: Fix 128 bit handling

 hw/misc/vfio.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

-- 
1.8.4.rc4

[Qemu-devel] [PATCH 1/2] vfio: Fix debug output for int128 values

[Alexey Kardashevskiy]

Memory regions can easily be 2^64 byte long and therefore overflow
for just a bit but that is enough for int128_get64() to assert.

This takes care of debug printing of huge section sizes.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/misc/vfio.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index adcd23d..e917f03 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -1928,7 +1928,8 @@ static void vfio_listener_region_add(MemoryListener *listener,
     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);
+                section->offset_within_address_space +
+                int128_get64(int128_sub(section->size, int128_one())));
         return;
     }
 
@@ -1973,7 +1974,8 @@ static void vfio_listener_region_del(MemoryListener *listener,
     if (vfio_listener_skipped_section(section)) {
         DPRINTF("SKIPPING region_del %"HWADDR_PRIx" - %"PRIx64"\n",
                 section->offset_within_address_space,
-                section->offset_within_address_space + section->size - 1);
+                section->offset_within_address_space +
+                int128_get64(int128_sub(section->size, int128_one())));
         return;
     }
 
-- 
1.8.4.rc4

[Qemu-devel] [PATCH 2/2] vfio: Fix 128 bit handling

[Alexey Kardashevskiy]

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 <aik@ozlabs.ru>
---
 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 e917f03..1889225 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);
+    int128_addto(&llend, section->size);
+    llend.lo &= 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;
+
     vaddr = memory_region_get_ram_ptr(section->mr) +
             section->offset_within_region +
             (iova - section->offset_within_address_space);
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH 2/2] vfio: Fix 128 bit handling

[Paolo Bonzini]

Il 21/08/2013 11:28, Alexey Kardashevskiy ha scritto:
> 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 <aik@ozlabs.ru>
> ---
>  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 e917f03..1889225 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);
> +    int128_addto(&llend, section->size);

Please use int128_add.

> +    llend.lo &= TARGET_PAGE_MASK;

Int128 is opaque, please use int128_and.  To build the constant you have
three choices (from my preferred to IMHO worst):

- add a new int128_exts64 function that sign-extends an int64_t

- use int128_neg(int128_make64(TARGET_PAGE_SIZE)) or something like that

- add a new int128_make function that takes a low/high pair and use
int128_make(TARGET_PAGE_MASK, -1)

Otherwise looks good!

Paolo

>  
> -    if (iova >= end) {
> +    if (int128_ge(int128_make64(iova), llend)) {
>          return;
>      }
>  
> +    end = (section->offset_within_address_space + int128_get64(section->size)) &
> +          TARGET_PAGE_MASK;
> +
>      vaddr = memory_region_get_ram_ptr(section->mr) +
>              section->offset_within_region +
>              (iova - section->offset_within_address_space);
> 

Re: [Qemu-devel] [PATCH 2/2] vfio: Fix 128 bit handling

[Alexey Kardashevskiy]

On 08/21/2013 08:07 PM, Paolo Bonzini wrote:
> Il 21/08/2013 11:28, Alexey Kardashevskiy ha scritto:
>> 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 <aik@ozlabs.ru>
>> ---
>>  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 e917f03..1889225 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);
>> +    int128_addto(&llend, section->size);
> 
> Please use int128_add.
> 
>> +    llend.lo &= TARGET_PAGE_MASK;
> 
> Int128 is opaque, please use int128_and.  To build the constant you have
> three choices (from my preferred to IMHO worst):
> 
> - add a new int128_exts64 function that sign-extends an int64_t

Like this? I am really scared to screw here :)

static inline Int128 int128_exts64(int64_t a)
{
    return (Int128) { .lo = a, .hi = (a < 0) ? -1 : 0 };
}


> - use int128_neg(int128_make64(TARGET_PAGE_SIZE)) or something like that


Did you actually mean TARGET_PAGE_SIZE-1 (with -1)? I'll better use this
for now.


> - add a new int128_make function that takes a low/high pair and use
> int128_make(TARGET_PAGE_MASK, -1)

I liked this one actually but you called it "worst" :)


> Otherwise looks good!
> 
> Paolo
> 
>>  
>> -    if (iova >= end) {
>> +    if (int128_ge(int128_make64(iova), llend)) {
>>          return;
>>      }
>>  
>> +    end = (section->offset_within_address_space + int128_get64(section->size)) &
>> +          TARGET_PAGE_MASK;
>> +
>>      vaddr = memory_region_get_ram_ptr(section->mr) +
>>              section->offset_within_region +
>>              (iova - section->offset_within_address_space);
>>
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH 2/2] vfio: Fix 128 bit handling

[Paolo Bonzini]

Il 22/08/2013 04:02, Alexey Kardashevskiy ha scritto:
>> Int128 is opaque, please use int128_and.  To build the constant you have
>> three choices (from my preferred to IMHO worst):
>>
>> - add a new int128_exts64 function that sign-extends an int64_t
> 
> Like this? I am really scared to screw here :)
> 
> static inline Int128 int128_exts64(int64_t a)
> {
>     return (Int128) { .lo = a, .hi = (a < 0) ? -1 : 0 };
> }

Yes, or just a >> 63.

>> - use int128_neg(int128_make64(TARGET_PAGE_SIZE)) or something like that
> 
> Did you actually mean TARGET_PAGE_SIZE-1 (with -1)? I'll better use this
> for now.

it would be either ~(TARGET_PAGE_SIZE-1) or -TARGET_PAGE_SIZE, I think.

>> - add a new int128_make function that takes a low/high pair and use
>> int128_make(TARGET_PAGE_MASK, -1)
> 
> I liked this one actually but you called it "worst" :)

It is really the same as #1 but inlined, which is why I called it the worst.

#2 is ugly for a different reason, namely because it changes the code
more substantially, from using TARGET_PAGE_MASK pre-patch to
TARGET_PAGE_SIZE post-patch.

Paolo