[2/2] vfio: Fix 128 bit handling

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(-)

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

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

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