| Message ID | 1377170965-9905-4-git-send-email-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
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 <aik@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); here? Thanks, Alex > vaddr = memory_region_get_ram_ptr(section->mr) + > section->offset_within_region + > (iova - section->offset_within_address_space);
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 <aik@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. > here? Thanks, > > Alex > >> vaddr = memory_region_get_ram_ptr(section->mr) + >> section->offset_within_region + >> (iova - section->offset_within_address_space); > > >
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 <aik@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
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 <aik@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.
Il 29/08/2013 04:26, Alexey Kardashevskiy ha scritto: > > Right. I was planning to add my IOMMU stuff right before calculating @end. But then the non-IOMMU stuff can just use int128_get64, no? So even if this patch simply uses int128_get64, it is still a suitable basis for adding IOMMU stuff. Paolo > >> > 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.
On 08/29/2013 04:29 PM, Paolo Bonzini wrote: > Il 29/08/2013 04:26, Alexey Kardashevskiy ha scritto: >> >> Right. I was planning to add my IOMMU stuff right before calculating @end. > > But then the non-IOMMU stuff can just use int128_get64, no? So even if > this patch simply uses int128_get64, it is still a suitable basis for > adding IOMMU stuff. Suitable but ugly. What if before calling int128_get64, I test section->size if it is <2^64 and only then do RAM part of this function? > > Paolo > >> >>>> 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. >
Il 29/08/2013 08:58, Alexey Kardashevskiy ha scritto: > On 08/29/2013 04:29 PM, Paolo Bonzini wrote: >> Il 29/08/2013 04:26, Alexey Kardashevskiy ha scritto: >>> >>> Right. I was planning to add my IOMMU stuff right before calculating @end. >> >> But then the non-IOMMU stuff can just use int128_get64, no? So even if >> this patch simply uses int128_get64, it is still a suitable basis for >> adding IOMMU stuff. > > Suitable but ugly. What if before calling int128_get64, I test > section->size if it is <2^64 and only then do RAM part of this function? What if you just merge the two series together? Paolo
On 08/29/2013 06:50 PM, Paolo Bonzini wrote: > Il 29/08/2013 08:58, Alexey Kardashevskiy ha scritto: >> On 08/29/2013 04:29 PM, Paolo Bonzini wrote: >>> Il 29/08/2013 04:26, Alexey Kardashevskiy ha scritto: >>>> >>>> Right. I was planning to add my IOMMU stuff right before calculating @end. >>> >>> But then the non-IOMMU stuff can just use int128_get64, no? So even if >>> this patch simply uses int128_get64, it is still a suitable basis for >>> adding IOMMU stuff. >> >> Suitable but ugly. What if before calling int128_get64, I test >> section->size if it is <2^64 and only then do RAM part of this function? > > What if you just merge the two series together? It will still be a function which can accept sections bigger than 2^64 and theoretically call int128_get64() and assert. I would think that every time when anyone calls int128_get64(), the value should be checked for <2^64. It is like division by zero :)
Il 30/08/2013 08:15, Alexey Kardashevskiy ha scritto: >> > What if you just merge the two series together? > It will still be a function which can accept sections bigger than 2^64 and > theoretically call int128_get64() and assert. I would think that every time > when anyone calls int128_get64(), the value should be checked for <2^64. It > is like division by zero :) I understood that int128_get64() would be called only for RAM sections, not for IOMMUs (and RAM sections cannot be 2^64-bytes large). Is this wrong? Paolo
On 08/30/2013 04:39 PM, Paolo Bonzini wrote: > Il 30/08/2013 08:15, Alexey Kardashevskiy ha scritto: >>>> What if you just merge the two series together? >> It will still be a function which can accept sections bigger than 2^64 and >> theoretically call int128_get64() and assert. I would think that every time >> when anyone calls int128_get64(), the value should be checked for <2^64. It >> is like division by zero :) > > I understood that int128_get64() would be called only for RAM sections, > not for IOMMUs (and RAM sections cannot be 2^64-bytes large). Is this > wrong? This is correct but for people who do not know when and in what state it is called, it can be confusing. Ok, I'll merge this with my vfio patches.
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; + vaddr = memory_region_get_ram_ptr(section->mr) + section->offset_within_region + (iova - section->offset_within_address_space);
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 <aik@ozlabs.ru> --- Changes: v2: * used new function int128_exts64() --- hw/misc/vfio.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)