Patchwork [10/21] memory: make section size a 128-bit integer

login
register
mail settings
Submitter Paolo Bonzini
Date June 7, 2013, 1:09 a.m.
Message ID <51B132C1.8010406@redhat.com>
Download mbox | patch
Permalink /patch/249575/
State New
Headers show

Comments

Paolo Bonzini - June 7, 2013, 1:09 a.m.
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:

Paolo
Alexey Kardashevskiy - June 7, 2013, 1:23 a.m.
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.

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.

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.

Am I missing a bigger picture?
Paolo Bonzini - June 7, 2013, 2:39 a.m.
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

Patch

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.