| Message ID | 51B132C1.8010406@redhat.com |
|---|---|
| State | New |
| Headers | show |
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?
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
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.