Re: [Qemu-devel] [PATCH 10/21] memory: make section size a 128-bit integer

[Paolo Bonzini]

Il 06/06/2013 21:23, Alexey Kardashevskiy ha scritto:
> On 06/07/2013 11:09 AM, Paolo Bonzini wrote:
>> Il 06/06/2013 04:36, Alexey Kardashevskiy ha scritto:
>>>>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
>>>>> index 693a9ff..c89676b 100644
>>>>> --- a/hw/misc/vfio.c
>>>>> +++ b/hw/misc/vfio.c
>>>>> @@ -1953,7 +1953,7 @@ static void vfio_listener_region_add(MemoryListener 
>>>>> *listener,
>>>>>      }
>>>>>  
>>>>>      iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
>>>>> -    end = (section->offset_within_address_space + section->size) &
>>>>> +    end = (section->offset_within_address_space + 
>>>>> int128_get64(section->size)) &
>>>>>            TARGET_PAGE_MASK;
>>>
>>>
>>> Another problem with this patch. Here is some more context (***):
>>
>> By the time you get here, this should have already crashed at this
>> code that patch 13 adds:
>>
>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
>> index c89676b..52fb036 100644
>> --- a/hw/misc/vfio.c
>> +++ b/hw/misc/vfio.c
>> @@ -1939,6 +1939,8 @@ static void vfio_listener_region_add(MemoryListener 
>> *listener,
>>      void *vaddr;
>>      int ret;
>>  
>> +    assert(!memory_region_is_iommu(section->mr));
>> +
>>
>> so it seems like a bug in your VFIO patches.
> 
> 
> No, I have David's patches which fix all of this, I'm planning to post them
> soon.

Then you have to change this code to use int128 throughout.

> My question is rather what is the whole point of calling
> memory_region_init_iommu with size==UINT64_MAX?
> 
> mem_add() tries to do register_subpage() when size is not aligned
> (UINT64_MAX is not) and fails.

UINT64_MAX represent a 2^64-byte region, not 2^64-1.  mem_add does not
see an unaligned size.

> So if we want to init memory region with the size as big as possible on
> 64bit systems, we either have to replace all 64bit sizes with 64bit end
> addresses (and then use 0-ffffffffffffffff) or support int128 sizes
> everywhere (even if it is just hi=1, lo=0) or stop initializing memory
> regions with sizes way bigger than we really need in next 5 years.

The previous version of the patches limited the address space to 62
bits, but s390 guys preferred to have 2^64.

Paolo