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

[Qemu-devel] [PATCH v3 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:
v3:
* "int128: add int128_exts64()" updated

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 v3 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>
---
Changes:
v3:
* (.hi = (a >> 63) ? -1 : 0) changed to (.hi = (a < 0) ? -1 : 0)
---
 include/qemu/int128.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/qemu/int128.h b/include/qemu/int128.h
index 9ed47aa..ef87e5e 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 < 0) ? -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

[Qemu-devel] [PATCH v3 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 v3 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 v3 0/3] vfio: fixes for better support for 128 bit memory section sizes

[Alexey Kardashevskiy]

On 08/22/2013 09:29 PM, Alexey Kardashevskiy wrote:
> I made a couple of small patches while debugging VFIO on SPAPR
> which uses IOMMU MemoryRegion 2^64 bytes long.
> 
> Changes:
> v3:
> * "int128: add int128_exts64()" updated
> 
> 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(-)


Ping? I fixed everything I was told to, is there anything left? Or we need
to decide through which tree this should go? :) Thanks!




-- 
Alexey

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

[Paolo Bonzini]

Il 28/08/2013 11:46, Alexey Kardashevskiy ha scritto:
> On 08/22/2013 09:29 PM, Alexey Kardashevskiy wrote:
>> I made a couple of small patches while debugging VFIO on SPAPR
>> which uses IOMMU MemoryRegion 2^64 bytes long.
>>
>> Changes:
>> v3:
>> * "int128: add int128_exts64()" updated
>>
>> 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(-)
> 
> Ping? I fixed everything I was told to, is there anything left? Or we need
> to decide through which tree this should go? :) Thanks!

I think it should go through Alex's tree, but he may be busy due to the
impending opening of the Linux merge window.

Paolo

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

[Alex Williamson]

On Wed, 2013-08-28 at 13:09 +0200, Paolo Bonzini wrote:
> Il 28/08/2013 11:46, Alexey Kardashevskiy ha scritto:
> > On 08/22/2013 09:29 PM, Alexey Kardashevskiy wrote:
> >> I made a couple of small patches while debugging VFIO on SPAPR
> >> which uses IOMMU MemoryRegion 2^64 bytes long.
> >>
> >> Changes:
> >> v3:
> >> * "int128: add int128_exts64()" updated
> >>
> >> 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(-)
> > 
> > Ping? I fixed everything I was told to, is there anything left? Or we need
> > to decide through which tree this should go? :) Thanks!
> 
> I think it should go through Alex's tree, but he may be busy due to the
> impending opening of the Linux merge window.

I can take it if you could ACK the first patch.  Thanks,

Alex

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

[Paolo Bonzini]

Il 28/08/2013 16:10, Alex Williamson ha scritto:
> On Wed, 2013-08-28 at 13:09 +0200, Paolo Bonzini wrote:
>> Il 28/08/2013 11:46, Alexey Kardashevskiy ha scritto:
>>> On 08/22/2013 09:29 PM, Alexey Kardashevskiy wrote:
>>>> I made a couple of small patches while debugging VFIO on SPAPR
>>>> which uses IOMMU MemoryRegion 2^64 bytes long.
>>>>
>>>> Changes:
>>>> v3:
>>>> * "int128: add int128_exts64()" updated
>>>>
>>>> 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(-)
>>>
>>> Ping? I fixed everything I was told to, is there anything left? Or we need
>>> to decide through which tree this should go? :) Thanks!
>>
>> I think it should go through Alex's tree, but he may be busy due to the
>> impending opening of the Linux merge window.
> 
> I can take it if you could ACK the first patch.  Thanks,

Sure, consider it acked. :)

Paolo

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

[Alex Williamson]

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 <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;
> +

I'm confused, we build an Int128 version of end above for the
comparison, why isn't this just:

end = int128_get64(llend);

here?  Thanks,

Alex

>      vaddr = memory_region_get_ram_ptr(section->mr) +
>              section->offset_within_region +
>              (iova - section->offset_within_address_space);

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

[Alexey Kardashevskiy]

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 <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;
>> +
> 
> 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.


> here?  Thanks,
> 
> Alex
> 
>>      vaddr = memory_region_get_ram_ptr(section->mr) +
>>              section->offset_within_region +
>>              (iova - section->offset_within_address_space);
> 
> 
> 


-- 
Alexey

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

[Alex Williamson]

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 <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;
> >> +
> > 
> > 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?  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,

Alex

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

[Alexey Kardashevskiy]

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 <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;
>>>> +
>>>
>>> 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

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

[Paolo Bonzini]

Il 29/08/2013 04:26, Alexey Kardashevskiy ha scritto:
> 
> Right. I was planning to add my IOMMU stuff right before calculating @end.

But then the non-IOMMU stuff can just use int128_get64, no?  So even if
this patch simply uses int128_get64, it is still a suitable basis for
adding IOMMU stuff.

Paolo

> 
>> > 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.

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

[Alexey Kardashevskiy]

On 08/29/2013 04:29 PM, Paolo Bonzini wrote:
> Il 29/08/2013 04:26, Alexey Kardashevskiy ha scritto:
>>
>> Right. I was planning to add my IOMMU stuff right before calculating @end.
> 
> But then the non-IOMMU stuff can just use int128_get64, no?  So even if
> this patch simply uses int128_get64, it is still a suitable basis for
> adding IOMMU stuff.

Suitable but ugly. What if before calling int128_get64, I test
section->size if it is <2^64 and only then do RAM part of this function?


> 
> Paolo
> 
>>
>>>> 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

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

[Paolo Bonzini]

Il 29/08/2013 08:58, Alexey Kardashevskiy ha scritto:
> On 08/29/2013 04:29 PM, Paolo Bonzini wrote:
>> Il 29/08/2013 04:26, Alexey Kardashevskiy ha scritto:
>>>
>>> Right. I was planning to add my IOMMU stuff right before calculating @end.
>>
>> But then the non-IOMMU stuff can just use int128_get64, no?  So even if
>> this patch simply uses int128_get64, it is still a suitable basis for
>> adding IOMMU stuff.
> 
> Suitable but ugly. What if before calling int128_get64, I test
> section->size if it is <2^64 and only then do RAM part of this function?

What if you just merge the two series together?

Paolo

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

[Alexey Kardashevskiy]

On 08/29/2013 06:50 PM, Paolo Bonzini wrote:
> Il 29/08/2013 08:58, Alexey Kardashevskiy ha scritto:
>> On 08/29/2013 04:29 PM, Paolo Bonzini wrote:
>>> Il 29/08/2013 04:26, Alexey Kardashevskiy ha scritto:
>>>>
>>>> Right. I was planning to add my IOMMU stuff right before calculating @end.
>>>
>>> But then the non-IOMMU stuff can just use int128_get64, no?  So even if
>>> this patch simply uses int128_get64, it is still a suitable basis for
>>> adding IOMMU stuff.
>>
>> Suitable but ugly. What if before calling int128_get64, I test
>> section->size if it is <2^64 and only then do RAM part of this function?
> 
> What if you just merge the two series together?

It will still be a function which can accept sections bigger than 2^64 and
theoretically call int128_get64() and assert. I would think that every time
when anyone calls int128_get64(), the value should be checked for <2^64. It
is like division by zero :)



-- 
Alexey

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

[Paolo Bonzini]

Il 30/08/2013 08:15, Alexey Kardashevskiy ha scritto:
>> > What if you just merge the two series together?
> It will still be a function which can accept sections bigger than 2^64 and
> theoretically call int128_get64() and assert. I would think that every time
> when anyone calls int128_get64(), the value should be checked for <2^64. It
> is like division by zero :)

I understood that int128_get64() would be called only for RAM sections,
not for IOMMUs (and RAM sections cannot be 2^64-bytes large).  Is this
wrong?

Paolo

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

[Alexey Kardashevskiy]

On 08/30/2013 04:39 PM, Paolo Bonzini wrote:
> Il 30/08/2013 08:15, Alexey Kardashevskiy ha scritto:
>>>> What if you just merge the two series together?
>> It will still be a function which can accept sections bigger than 2^64 and
>> theoretically call int128_get64() and assert. I would think that every time
>> when anyone calls int128_get64(), the value should be checked for <2^64. It
>> is like division by zero :)
> 
> I understood that int128_get64() would be called only for RAM sections,
> not for IOMMUs (and RAM sections cannot be 2^64-bytes large).  Is this
> wrong?


This is correct but for people who do not know when and in what state it is
called, it can be confusing. Ok, I'll merge this with my vfio patches.



-- 
Alexey

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

[Alexey Kardashevskiy]

On 08/29/2013 12:29 AM, Paolo Bonzini wrote:
> Il 28/08/2013 16:10, Alex Williamson ha scritto:
>> On Wed, 2013-08-28 at 13:09 +0200, Paolo Bonzini wrote:
>>> Il 28/08/2013 11:46, Alexey Kardashevskiy ha scritto:
>>>> On 08/22/2013 09:29 PM, Alexey Kardashevskiy wrote:
>>>>> I made a couple of small patches while debugging VFIO on SPAPR
>>>>> which uses IOMMU MemoryRegion 2^64 bytes long.
>>>>>
>>>>> Changes:
>>>>> v3:
>>>>> * "int128: add int128_exts64()" updated
>>>>>
>>>>> 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(-)
>>>>
>>>> Ping? I fixed everything I was told to, is there anything left? Or we need
>>>> to decide through which tree this should go? :) Thanks!
>>>
>>> I think it should go through Alex's tree, but he may be busy due to the
>>> impending opening of the Linux merge window.
>>
>> I can take it if you could ACK the first patch.  Thanks,
> 
> Sure, consider it acked. :)


Soooo, Alex, can I consider them (first two I guess) taken by you to your
tree? I cannot find them in any tree...


-- 
Alexey

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

[Paolo Bonzini]

Il 10/09/2013 06:23, Alexey Kardashevskiy ha scritto:
> On 08/29/2013 12:29 AM, Paolo Bonzini wrote:
>> Il 28/08/2013 16:10, Alex Williamson ha scritto:
>>> On Wed, 2013-08-28 at 13:09 +0200, Paolo Bonzini wrote:
>>>> Il 28/08/2013 11:46, Alexey Kardashevskiy ha scritto:
>>>>> On 08/22/2013 09:29 PM, Alexey Kardashevskiy wrote:
>>>>>> I made a couple of small patches while debugging VFIO on SPAPR
>>>>>> which uses IOMMU MemoryRegion 2^64 bytes long.
>>>>>>
>>>>>> Changes:
>>>>>> v3:
>>>>>> * "int128: add int128_exts64()" updated
>>>>>>
>>>>>> 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(-)
>>>>>
>>>>> Ping? I fixed everything I was told to, is there anything left? Or we need
>>>>> to decide through which tree this should go? :) Thanks!
>>>>
>>>> I think it should go through Alex's tree, but he may be busy due to the
>>>> impending opening of the Linux merge window.
>>>
>>> I can take it if you could ACK the first patch.  Thanks,
>>
>> Sure, consider it acked. :)
> 
> Soooo, Alex, can I consider them (first two I guess) taken by you to your
> tree? I cannot find them in any tree...

See http://article.gmane.org/gmane.comp.emulators.qemu/232179

Paolo