* [Qemu-devel] [PATCH] vfio: convert to 128 bit arithmetic calculations when adding mem regions
@ 2016-03-23 20:57 Bandan Das
2016-03-23 23:00 ` Alex Williamson
0 siblings, 1 reply; 3+ messages in thread
From: Bandan Das @ 2016-03-23 20:57 UTC (permalink / raw)
To: qemu-devel, alex.williamson
vfio_listener_region_add for a iommu mr results in
an overflow assert since vt-d memory region is initialized
with UINT64_MAX. Convert calculations to 128 bit arithmetic
for such regions and let int128_get64 assert for non iommu
regions only.
Suggested-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Bandan Das <bsd@redhat.com>
---
This is v2 of http://patchwork.ozlabs.org/patch/600364/
hw/vfio/common.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index fb588d8..9489ff3 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -323,7 +323,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
{
VFIOContainer *container = container_of(listener, VFIOContainer, listener);
hwaddr iova, end;
- Int128 llend;
+ Int128 llend, endaddr;
void *vaddr;
int ret;
@@ -349,14 +349,15 @@ static void vfio_listener_region_add(MemoryListener *listener,
if (int128_ge(int128_make64(iova), llend)) {
return;
}
- end = int128_get64(llend);
+ endaddr = int128_sub(llend, int128_one());
- if ((iova < container->min_iova) || ((end - 1) > container->max_iova)) {
- error_report("vfio: IOMMU container %p can't map guest IOVA region"
- " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx,
- container, iova, end - 1);
- ret = -EFAULT;
- goto fail;
+ if ((iova < container->min_iova) || (int128_get64(endaddr) >
+ container->max_iova)) {
+ error_report("vfio: IOMMU container %p can't map guest IOVA region"
+ " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx,
+ container, iova, int128_get64(endaddr));
+ ret = -EFAULT;
+ goto fail;
}
memory_region_ref(section->mr);
@@ -364,7 +365,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
if (memory_region_is_iommu(section->mr)) {
VFIOGuestIOMMU *giommu;
- trace_vfio_listener_region_add_iommu(iova, end - 1);
+ trace_vfio_listener_region_add_iommu(iova, int128_get64(endaddr));
/*
* FIXME: We should do some checking to see if the
* capabilities of the host VFIO IOMMU are adequate to model
@@ -389,6 +390,8 @@ static void vfio_listener_region_add(MemoryListener *listener,
return;
}
+ end = int128_get64(llend);
+
/* Here we assume that memory_region_is_ram(section->mr)==true */
vaddr = memory_region_get_ram_ptr(section->mr) +
--
2.5.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] vfio: convert to 128 bit arithmetic calculations when adding mem regions
2016-03-23 20:57 [Qemu-devel] [PATCH] vfio: convert to 128 bit arithmetic calculations when adding mem regions Bandan Das
@ 2016-03-23 23:00 ` Alex Williamson
2016-03-24 0:20 ` Bandan Das
0 siblings, 1 reply; 3+ messages in thread
From: Alex Williamson @ 2016-03-23 23:00 UTC (permalink / raw)
To: Bandan Das; +Cc: qemu-devel
On Wed, 23 Mar 2016 16:57:35 -0400
Bandan Das <bsd@redhat.com> wrote:
> vfio_listener_region_add for a iommu mr results in
> an overflow assert since vt-d memory region is initialized
> with UINT64_MAX. Convert calculations to 128 bit arithmetic
> for such regions and let int128_get64 assert for non iommu
> regions only.
>
> Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
> This is v2 of http://patchwork.ozlabs.org/patch/600364/
> hw/vfio/common.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index fb588d8..9489ff3 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -323,7 +323,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
> {
> VFIOContainer *container = container_of(listener, VFIOContainer, listener);
> hwaddr iova, end;
> - Int128 llend;
> + Int128 llend, endaddr;
> void *vaddr;
> int ret;
>
> @@ -349,14 +349,15 @@ static void vfio_listener_region_add(MemoryListener *listener,
> if (int128_ge(int128_make64(iova), llend)) {
> return;
> }
> - end = int128_get64(llend);
> + endaddr = int128_sub(llend, int128_one());
I like where this is headed, but there's maybe a little polish left.
endaddr here can never be greater than UINT64_MAX, so why don't we make
it a hwaddr and wrap the whole right side of the equation in an
int128_get64(). Then the (end - 1) changes below don't need the
extra int128 ops.
>
> - if ((iova < container->min_iova) || ((end - 1) > container->max_iova)) {
> - error_report("vfio: IOMMU container %p can't map guest IOVA region"
> - " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx,
> - container, iova, end - 1);
> - ret = -EFAULT;
> - goto fail;
> + if ((iova < container->min_iova) || (int128_get64(endaddr) >
> + container->max_iova)) {
> + error_report("vfio: IOMMU container %p can't map guest IOVA region"
> + " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx,
> + container, iova, int128_get64(endaddr));
> + ret = -EFAULT;
> + goto fail;
Bogus whitespace changes
> }
>
> memory_region_ref(section->mr);
> @@ -364,7 +365,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
> if (memory_region_is_iommu(section->mr)) {
> VFIOGuestIOMMU *giommu;
>
> - trace_vfio_listener_region_add_iommu(iova, end - 1);
> + trace_vfio_listener_region_add_iommu(iova, int128_get64(endaddr));
> /*
> * FIXME: We should do some checking to see if the
> * capabilities of the host VFIO IOMMU are adequate to model
> @@ -389,6 +390,8 @@ static void vfio_listener_region_add(MemoryListener *listener,
> return;
> }
>
> + end = int128_get64(llend);
> +
> /* Here we assume that memory_region_is_ram(section->mr)==true */
>
> vaddr = memory_region_get_ram_ptr(section->mr) +
For this last chunk I'd do something like this:
+ llsize = int128_sub(llend, int128_make64(iova));
- ret = vfio_dma_map(container, iova, end - iova, vaddr, section->readonly);
+ ret = vfio_dma_map(container, iova,
+ int128_get64(llsize), vaddr, section->readonly);
if (ret) {
error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
"0x%"HWADDR_PRIx", %p) = %d (%m)",
- container, iova, end - iova, vaddr, ret);
+ container, iova, int128_get64(llsize), vaddr, ret);
goto fail;
}
This maintains the 128bit math until exactly the point where it will
fall apart when vfio_dma_map can't pass an int128 to the vfio API.
Thanks,
Alex
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] vfio: convert to 128 bit arithmetic calculations when adding mem regions
2016-03-23 23:00 ` Alex Williamson
@ 2016-03-24 0:20 ` Bandan Das
0 siblings, 0 replies; 3+ messages in thread
From: Bandan Das @ 2016-03-24 0:20 UTC (permalink / raw)
To: Alex Williamson; +Cc: qemu-devel
Alex Williamson <alex.williamson@redhat.com> writes:
...
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index fb588d8..9489ff3 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -323,7 +323,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
>> {
>> VFIOContainer *container = container_of(listener, VFIOContainer, listener);
>> hwaddr iova, end;
>> - Int128 llend;
>> + Int128 llend, endaddr;
>> void *vaddr;
>> int ret;
>>
>> @@ -349,14 +349,15 @@ static void vfio_listener_region_add(MemoryListener *listener,
>> if (int128_ge(int128_make64(iova), llend)) {
>> return;
>> }
>> - end = int128_get64(llend);
>> + endaddr = int128_sub(llend, int128_one());
>
> I like where this is headed, but there's maybe a little polish left.
>
> endaddr here can never be greater than UINT64_MAX, so why don't we make
> it a hwaddr and wrap the whole right side of the equation in an
> int128_get64(). Then the (end - 1) changes below don't need the
> extra int128 ops.
Sounds good.
>>
>> - if ((iova < container->min_iova) || ((end - 1) > container->max_iova)) {
>> - error_report("vfio: IOMMU container %p can't map guest IOVA region"
>> - " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx,
>> - container, iova, end - 1);
>> - ret = -EFAULT;
>> - goto fail;
>> + if ((iova < container->min_iova) || (int128_get64(endaddr) >
>> + container->max_iova)) {
>> + error_report("vfio: IOMMU container %p can't map guest IOVA region"
>> + " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx,
>> + container, iova, int128_get64(endaddr));
>> + ret = -EFAULT;
>> + goto fail;
>
> Bogus whitespace changes
Oops, sorry!
>> }
>>
>> memory_region_ref(section->mr);
>> @@ -364,7 +365,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
>> if (memory_region_is_iommu(section->mr)) {
>> VFIOGuestIOMMU *giommu;
>>
>> - trace_vfio_listener_region_add_iommu(iova, end - 1);
>> + trace_vfio_listener_region_add_iommu(iova, int128_get64(endaddr));
>> /*
>> * FIXME: We should do some checking to see if the
>> * capabilities of the host VFIO IOMMU are adequate to model
>> @@ -389,6 +390,8 @@ static void vfio_listener_region_add(MemoryListener *listener,
>> return;
>> }
>>
>> + end = int128_get64(llend);
>> +
>> /* Here we assume that memory_region_is_ram(section->mr)==true */
>>
>> vaddr = memory_region_get_ram_ptr(section->mr) +
>
> For this last chunk I'd do something like this:
>
> + llsize = int128_sub(llend, int128_make64(iova));
>
> - ret = vfio_dma_map(container, iova, end - iova, vaddr, section->readonly);
> + ret = vfio_dma_map(container, iova,
> + int128_get64(llsize), vaddr, section->readonly);
> if (ret) {
> error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
> "0x%"HWADDR_PRIx", %p) = %d (%m)",
> - container, iova, end - iova, vaddr, ret);
> + container, iova, int128_get64(llsize), vaddr, ret);
> goto fail;
> }
>
> This maintains the 128bit math until exactly the point where it will
> fall apart when vfio_dma_map can't pass an int128 to the vfio API.
Yep, sounds good. v2 coming up shortly.
> Thanks,
>
> Alex
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-03-24 0:20 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-23 20:57 [Qemu-devel] [PATCH] vfio: convert to 128 bit arithmetic calculations when adding mem regions Bandan Das
2016-03-23 23:00 ` Alex Williamson
2016-03-24 0:20 ` Bandan Das
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).