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

[Qemu-devel] [PATCH v2 0/3] 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.

Changes:
v2:
* added int128_exts64() function as a separate patch and used in
"vfio: Fix 128 bit handling".


Alexey Kardashevskiy (3):
  int128: add int128_exts64()
  vfio: Fix debug output for int128 values
  vfio: Fix 128 bit handling

 hw/misc/vfio.c        | 19 +++++++++++++------
 include/qemu/int128.h |  5 +++++
 2 files changed, 18 insertions(+), 6 deletions(-)

-- 
1.8.4.rc4

[Qemu-devel] [PATCH v2 1/3] int128: add int128_exts64()

[Alexey Kardashevskiy]

This adds macro to extend signed 64bit value to signed 128bit value.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 include/qemu/int128.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/qemu/int128.h b/include/qemu/int128.h
index 9ed47aa..987a1a9 100644
--- a/include/qemu/int128.h
+++ b/include/qemu/int128.h
@@ -38,6 +38,11 @@ static inline Int128 int128_2_64(void)
     return (Int128) { 0, 1 };
 }
 
+static inline Int128 int128_exts64(int64_t a)
+{
+    return (Int128) { .lo = a, .hi = (a >> 63) ? -1 : 0 };
+}
+
 static inline Int128 int128_and(Int128 a, Int128 b)
 {
     return (Int128) { a.lo & b.lo, a.hi & b.hi };
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH v2 1/3] int128: add int128_exts64()

[Paolo Bonzini]

Il 22/08/2013 10:20, Alexey Kardashevskiy ha scritto:
> This adds macro to extend signed 64bit value to signed 128bit value.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  include/qemu/int128.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/qemu/int128.h b/include/qemu/int128.h
> index 9ed47aa..987a1a9 100644
> --- a/include/qemu/int128.h
> +++ b/include/qemu/int128.h
> @@ -38,6 +38,11 @@ static inline Int128 int128_2_64(void)
>      return (Int128) { 0, 1 };
>  }
>  
> +static inline Int128 int128_exts64(int64_t a)
> +{
> +    return (Int128) { .lo = a, .hi = (a >> 63) ? -1 : 0 };
> +}

The "? -1 : 0" is not necessary, but the compiler will remove it at -O1
or more (interestingly, or -O0 it will remove the shift and leave the
conditional!).

>  static inline Int128 int128_and(Int128 a, Int128 b)
>  {
>      return (Int128) { a.lo & b.lo, a.hi & b.hi };
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH v2 1/3] int128: add int128_exts64()

[Peter Maydell]

On 22 August 2013 10:09, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 22/08/2013 10:20, Alexey Kardashevskiy ha scritto:
>> +static inline Int128 int128_exts64(int64_t a)
>> +{
>> +    return (Int128) { .lo = a, .hi = (a >> 63) ? -1 : 0 };
>> +}
>
> The "? -1 : 0" is not necessary, but the compiler will remove it at -O1
> or more (interestingly, or -O0 it will remove the shift and leave the
> conditional!).

We can avoid relying on implementation defined
behaviour here by using
  .hi = (a < 0) ? -1 : 0;

(I know we allow ourselves to assume right-shift of signed
ints is arithmetic shift, but I think it's nicer to avoid it unless
it really makes the code better.)

-- PMM

Re: [Qemu-devel] [PATCH v2 1/3] int128: add int128_exts64()

[Paolo Bonzini]

Il 22/08/2013 11:47, Peter Maydell ha scritto:
> On 22 August 2013 10:09, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 22/08/2013 10:20, Alexey Kardashevskiy ha scritto:
>>> +static inline Int128 int128_exts64(int64_t a)
>>> +{
>>> +    return (Int128) { .lo = a, .hi = (a >> 63) ? -1 : 0 };
>>> +}
>>
>> The "? -1 : 0" is not necessary, but the compiler will remove it at -O1
>> or more (interestingly, or -O0 it will remove the shift and leave the
>> conditional!).
> 
> We can avoid relying on implementation defined
> behaviour here by using
>   .hi = (a < 0) ? -1 : 0;
> 
> (I know we allow ourselves to assume right-shift of signed
> ints is arithmetic shift, but I think it's nicer to avoid it unless
> it really makes the code better.)

This is what Alexey proposed.  I suggested (a >> 63) without the ?: but
he misunderstood my (probably not clear enough) suggestion.

Paolo

Re: [Qemu-devel] [PATCH v2 1/3] int128: add int128_exts64()

[Peter Maydell]

On 22 August 2013 10:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 22/08/2013 11:47, Peter Maydell ha scritto:
>> We can avoid relying on implementation defined
>> behaviour here by using
>>   .hi = (a < 0) ? -1 : 0;
>>
>> (I know we allow ourselves to assume right-shift of signed
>> ints is arithmetic shift, but I think it's nicer to avoid it unless
>> it really makes the code better.)
>
> This is what Alexey proposed.  I suggested (a >> 63) without the ?: but
> he misunderstood my (probably not clear enough) suggestion.

Yes, I found that email thread after sending this. I think
the (a < 0) variant is better than using a shift (with or without
the ?: operator).

-- PMM

Re: [Qemu-devel] [PATCH v2 1/3] int128: add int128_exts64()

[Alexey Kardashevskiy]

On 08/22/2013 07:48 PM, Paolo Bonzini wrote:
> Il 22/08/2013 11:47, Peter Maydell ha scritto:
>> On 22 August 2013 10:09, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> Il 22/08/2013 10:20, Alexey Kardashevskiy ha scritto:
>>>> +static inline Int128 int128_exts64(int64_t a)
>>>> +{
>>>> +    return (Int128) { .lo = a, .hi = (a >> 63) ? -1 : 0 };
>>>> +}
>>>
>>> The "? -1 : 0" is not necessary, but the compiler will remove it at -O1
>>> or more (interestingly, or -O0 it will remove the shift and leave the
>>> conditional!).
>>
>> We can avoid relying on implementation defined
>> behaviour here by using
>>   .hi = (a < 0) ? -1 : 0;
>>
>> (I know we allow ourselves to assume right-shift of signed
>> ints is arithmetic shift, but I think it's nicer to avoid it unless
>> it really makes the code better.)
> 
> This is what Alexey proposed.  I suggested (a >> 63) without the ?: but
> he misunderstood my (probably not clear enough) suggestion.

Yes, I misunderstood. It was not obvious to me that (signed long
long)-1>>63 will be still -1. I really (really) envy people who can easily
read stuff like but I cannot :(

1) return (Int128) { .lo = a, .hi = (a < 0) ? -1 : 0 };
2) return (Int128) { .lo = a, .hi = (a < 0) };
3) return (Int128) { .lo = a, .hi = a >> 63 };

So with which one should I repost the patch?


-- 
Alexey

Re: [Qemu-devel] [PATCH v2 1/3] int128: add int128_exts64()

[Paolo Bonzini]

Il 22/08/2013 12:50, Alexey Kardashevskiy ha scritto:
> 1) return (Int128) { .lo = a, .hi = (a < 0) ? -1 : 0 };
> 2) return (Int128) { .lo = a, .hi = (a < 0) };
> 3) return (Int128) { .lo = a, .hi = a >> 63 };
> 
> So with which one should I repost the patch?

The second would be "-(a < 0)" actually.  Peter wants (1) I think, so go
for it.

Paolo

[Qemu-devel] [PATCH v2 2/3] 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 017e693..dfe3a80 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 v2 3/3] 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>
---
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;
+
     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 v2 3/3] vfio: Fix 128 bit handling

[Paolo Bonzini]

Il 22/08/2013 10:20, Alexey Kardashevskiy ha scritto:
>      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;
> +

This can still fail for section->size = 2^64.  Do your IOMMU patches
take care of it?

Paolo

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

[Alexey Kardashevskiy]

On 08/22/2013 07:11 PM, Paolo Bonzini wrote:
> Il 22/08/2013 10:20, Alexey Kardashevskiy ha scritto:
>>      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;
>> +
> 
> This can still fail for section->size = 2^64.  Do your IOMMU patches
> take care of it?

Nope. That part works for IOMMU mapped to RAM which is smaller than 2^64
bytes and therefore I do not see why we would need 2^64 bits sizes there.
Either way, I cannot test it quick (yes, I know, I should have some x86
VFIO setup by hand as everyone has a lot of x86, etc...) so I decided to
leave to the moment when x86 folks hit the problem :)


-- 
Alexey

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

[Paolo Bonzini]

Il 22/08/2013 12:41, Alexey Kardashevskiy ha scritto:
> On 08/22/2013 07:11 PM, Paolo Bonzini wrote:
>> Il 22/08/2013 10:20, Alexey Kardashevskiy ha scritto:
>>>      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;
>>> +
>>
>> This can still fail for section->size = 2^64.  Do your IOMMU patches
>> take care of it?
> 
> Nope. That part works for IOMMU mapped to RAM which is smaller than 2^64
> bytes and therefore I do not see why we would need 2^64 bits sizes there.

Understood.  So the IOMMU patches take care of it because this code is
only used for non-IOMMU regions.  Thanks,

Paolo

> Either way, I cannot test it quick (yes, I know, I should have some x86
> VFIO setup by hand as everyone has a lot of x86, etc...) so I decided to
> leave to the moment when x86 folks hit the problem :)
> 
>