On 08/29/2013 11:42 AM, Alex Williamson wrote:
> On Thu, 2013-08-29 at 11:03 +1000, Alexey Kardashevskiy wrote:
>> On 08/29/2013 01:18 AM, Alex Williamson wrote:
>>> On Thu, 2013-08-22 at 21:29 +1000, Alexey Kardashevskiy wrote:
>>>> 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 <a...@ozlabs.ru>
>>>> ---
>>>> Changes:
>>>> v2:
>>>> * used new function int128_exts64()
>>>> ---
>>>> 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 dfe3a80..3878fc7 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);
>>>> + llend = int128_add(llend, section->size);
>>>> + llend = int128_and(llend, int128_exts64(TARGET_PAGE_MASK));
>>>>
>>>> - if (iova >= end) {
>>>> + if (int128_ge(int128_make64(iova), llend)) {
>>>> return;
>>>> }
>>>>
>>>> + end = (section->offset_within_address_space +
>>>> int128_get64(section->size)) &
>>>> + TARGET_PAGE_MASK;
>>>> +
>>>
>>> I'm confused, we build an Int128 version of end above for the
>>> comparison, why isn't this just:
>>>
>>> end = int128_get64(llend);
>>
>>
>> section->size for IOMMU memory region I have on spapr-vfio is 2^64 so
>> int128_get64() fails.
>
> But you're leaving code that does int128_get64(section->size)... how
> does that not assert?
Right. I was planning to add my IOMMU stuff right before calculating @end.
> This patch is not maintainable. We're seemingly
> calculating the same value twice with no comment as to why. A hwaddr
> type end should be calculated from the Int128 rather than paying
> attention to rollover in one place but not another. Thanks,
Yes, not maintainable... I guess I just have to convert @end to 128 bit as
well to have things consistent.
--
Alexey
