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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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