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? 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,
Alex
