Re: [PATCH v16 Kernel 5/7] vfio iommu: Update UNMAP_DMA ioctl to get dirty bitmap before unmap
On 3/30/2020 7:45 AM, Yan Zhao wrote:
On Fri, Mar 27, 2020 at 12:42:43PM +0800, Kirti Wankhede wrote:
On 3/27/2020 5:34 AM, Yan Zhao wrote:
On Fri, Mar 27, 2020 at 05:39:44AM +0800, Kirti Wankhede wrote:
On 3/25/2020 7:48 AM, Yan Zhao wrote:
On Wed, Mar 25, 2020 at 03:32:37AM +0800, Kirti Wankhede wrote:
DMA mapped pages, including those pinned by mdev vendor drivers, might
get unpinned and unmapped while migration is active and device is still
running. For example, in pre-copy phase while guest driver could access
those pages, host device or vendor driver can dirty these mapped pages.
Such pages should be marked dirty so as to maintain memory consistency
for a user making use of dirty page tracking.
To get bitmap during unmap, user should allocate memory for bitmap, set
size of allocated memory, set page size to be considered for bitmap and
set flag VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP.
Signed-off-by: Kirti Wankhede
Reviewed-by: Neo Jia
---
drivers/vfio/vfio_iommu_type1.c | 54
++---
include/uapi/linux/vfio.h | 10
2 files changed, 60 insertions(+), 4 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 27ed069c5053..b98a8d79e13a 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -982,7 +982,8 @@ static int verify_bitmap_size(uint64_t npages, uint64_t
bitmap_size)
}
static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
-struct vfio_iommu_type1_dma_unmap *unmap)
+struct vfio_iommu_type1_dma_unmap *unmap,
+struct vfio_bitmap *bitmap)
{
uint64_t mask;
struct vfio_dma *dma, *dma_last = NULL;
@@ -1033,6 +1034,10 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
* will be returned if these conditions are not met. The v2 interface
* will only return success and a size of zero if there were no
* mappings within the range.
+*
+* When VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP flag is set, unmap request
+* must be for single mapping. Multiple mappings with this flag set is
+* not supported.
*/
if (iommu->v2) {
dma = vfio_find_dma(iommu, unmap->iova, 1);
@@ -1040,6 +1045,13 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
ret = -EINVAL;
goto unlock;
}
+
+ if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
+ (dma->iova != unmap->iova || dma->size != unmap->size)) {
potential NULL pointer!
And could you address the comments in v14?
How to handle DSI unmaps in vIOMMU
(https://lore.kernel.org/kvm/20200323011041.GB5456@joy-OptiPlex-7040/)
Sorry, I drafted reply to it, but I missed to send, it remained in my drafts
>
> it happens in vIOMMU Domain level invalidation of IOTLB
> (domain-selective invalidation, see vtd_iotlb_domain_invalidate() in
qemu).
> common in VTD lazy mode, and NOT just happening once at boot time.
> rather than invalidate page by page, it batches the page invalidation.
> so, when this invalidation takes place, even higher level page tables
> have been invalid and therefore it has to invalidate a bigger
combined range.
> That's why we see IOVAs are mapped in 4k pages, but are unmapped in 2M
> pages.
>
> I think those UNMAPs should also have GET_DIRTY_BIMTAP flag on, right?
vtd_iotlb_domain_invalidate()
vtd_sync_shadow_page_table()
vtd_sync_shadow_page_table_range(vtd_as, &ce, 0, UINT64_MAX)
vtd_page_walk()
vtd_page_walk_level() - walk over specific level for IOVA range
vtd_page_walk_one()
memory_region_notify_iommu()
...
vfio_iommu_map_notify()
In the above trace, isn't page walk will take care of creating proper
IOTLB entry which should be same as created during mapping for that
IOTLB entry?
No. It does walk the page table, but as it's dsi (delay & batched unmap),
pages table entry for a whole 2M (the higher level, not last level for 4K)
range is invalid, so the iotlb->addr_mask what vfio_iommu_map_notify()
receives is (2M - 1), not the same as the size for map.
When do this happen? during my testing I never hit this case. How can I
hit this case?
Just common settings to enable vIOMMU:
Qemu: -device intel-iommu,caching-mode=true
guest kernel parameter: intel_iommu=on
(intel_iommu=on turns on lazy mode by default)
In lazy mode, guest notifies DMA MAP on page level, but notifies DMA UNMAPs
in batch.
with a pass-through NVMe, there are 89 DSI unmaps in 1 second for a typical fio.
With a pass-through GPU, there 22 DSI unmaps in total for benchmark openArena
(lasting around 55 secs)
In this case, will adjacent whole vfio_dmas will be clubbed together or
will there be any intersectio
Re: [PATCH v16 Kernel 5/7] vfio iommu: Update UNMAP_DMA ioctl to get dirty bitmap before unmap
On Fri, Mar 27, 2020 at 12:42:43PM +0800, Kirti Wankhede wrote:
>
>
> On 3/27/2020 5:34 AM, Yan Zhao wrote:
> > On Fri, Mar 27, 2020 at 05:39:44AM +0800, Kirti Wankhede wrote:
> >>
> >>
> >> On 3/25/2020 7:48 AM, Yan Zhao wrote:
> >>> On Wed, Mar 25, 2020 at 03:32:37AM +0800, Kirti Wankhede wrote:
> DMA mapped pages, including those pinned by mdev vendor drivers, might
> get unpinned and unmapped while migration is active and device is still
> running. For example, in pre-copy phase while guest driver could access
> those pages, host device or vendor driver can dirty these mapped pages.
> Such pages should be marked dirty so as to maintain memory consistency
> for a user making use of dirty page tracking.
>
> To get bitmap during unmap, user should allocate memory for bitmap, set
> size of allocated memory, set page size to be considered for bitmap and
> set flag VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP.
>
> Signed-off-by: Kirti Wankhede
> Reviewed-by: Neo Jia
> ---
> drivers/vfio/vfio_iommu_type1.c | 54
> ++---
> include/uapi/linux/vfio.h | 10
> 2 files changed, 60 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c
> b/drivers/vfio/vfio_iommu_type1.c
> index 27ed069c5053..b98a8d79e13a 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -982,7 +982,8 @@ static int verify_bitmap_size(uint64_t npages,
> uint64_t bitmap_size)
> }
>
> static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> - struct vfio_iommu_type1_dma_unmap *unmap)
> + struct vfio_iommu_type1_dma_unmap *unmap,
> + struct vfio_bitmap *bitmap)
> {
> uint64_t mask;
> struct vfio_dma *dma, *dma_last = NULL;
> @@ -1033,6 +1034,10 @@ static int vfio_dma_do_unmap(struct vfio_iommu
> *iommu,
> * will be returned if these conditions are not met. The v2
> interface
> * will only return success and a size of zero if there were no
> * mappings within the range.
> + *
> + * When VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP flag is set, unmap
> request
> + * must be for single mapping. Multiple mappings with this flag
> set is
> + * not supported.
> */
> if (iommu->v2) {
> dma = vfio_find_dma(iommu, unmap->iova, 1);
> @@ -1040,6 +1045,13 @@ static int vfio_dma_do_unmap(struct vfio_iommu
> *iommu,
> ret = -EINVAL;
> goto unlock;
> }
> +
> +if ((unmap->flags &
> VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
> +(dma->iova != unmap->iova || dma->size !=
> unmap->size)) {
> >>> potential NULL pointer!
> >>>
> >>> And could you address the comments in v14?
> >>> How to handle DSI unmaps in vIOMMU
> >>> (https://lore.kernel.org/kvm/20200323011041.GB5456@joy-OptiPlex-7040/)
> >>>
> >>
> >> Sorry, I drafted reply to it, but I missed to send, it remained in my
> >> drafts
> >>
> >> >
> >> > it happens in vIOMMU Domain level invalidation of IOTLB
> >> > (domain-selective invalidation, see vtd_iotlb_domain_invalidate() in
> >> qemu).
> >> > common in VTD lazy mode, and NOT just happening once at boot time.
> >> > rather than invalidate page by page, it batches the page invalidation.
> >> > so, when this invalidation takes place, even higher level page tables
> >> > have been invalid and therefore it has to invalidate a bigger
> >> combined range.
> >> > That's why we see IOVAs are mapped in 4k pages, but are unmapped in 2M
> >> > pages.
> >> >
> >> > I think those UNMAPs should also have GET_DIRTY_BIMTAP flag on, right?
> >>
> >>
> >> vtd_iotlb_domain_invalidate()
> >> vtd_sync_shadow_page_table()
> >> vtd_sync_shadow_page_table_range(vtd_as, &ce, 0, UINT64_MAX)
> >> vtd_page_walk()
> >> vtd_page_walk_level() - walk over specific level for IOVA range
> >> vtd_page_walk_one()
> >> memory_region_notify_iommu()
> >> ...
> >> vfio_iommu_map_notify()
> >>
> >> In the above trace, isn't page walk will take care of creating proper
> >> IOTLB entry which should be same as created during mapping for that
> >> IOTLB entry?
> >>
> > No. It does walk the page table, but as it's dsi (delay & batched unmap),
> > pages table entry for a whole 2M (the higher level, not last level for 4K)
> > range is invalid, so the iotlb->addr_mask what vfio_iommu_map_notify()
> > receives is (2M - 1), not the same as the size for map.
> >
>
> When do this happen?
Re: [PATCH v16 Kernel 5/7] vfio iommu: Update UNMAP_DMA ioctl to get dirty bitmap before unmap
On 3/27/2020 5:34 AM, Yan Zhao wrote:
On Fri, Mar 27, 2020 at 05:39:44AM +0800, Kirti Wankhede wrote:
On 3/25/2020 7:48 AM, Yan Zhao wrote:
On Wed, Mar 25, 2020 at 03:32:37AM +0800, Kirti Wankhede wrote:
DMA mapped pages, including those pinned by mdev vendor drivers, might
get unpinned and unmapped while migration is active and device is still
running. For example, in pre-copy phase while guest driver could access
those pages, host device or vendor driver can dirty these mapped pages.
Such pages should be marked dirty so as to maintain memory consistency
for a user making use of dirty page tracking.
To get bitmap during unmap, user should allocate memory for bitmap, set
size of allocated memory, set page size to be considered for bitmap and
set flag VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP.
Signed-off-by: Kirti Wankhede
Reviewed-by: Neo Jia
---
drivers/vfio/vfio_iommu_type1.c | 54
++---
include/uapi/linux/vfio.h | 10
2 files changed, 60 insertions(+), 4 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 27ed069c5053..b98a8d79e13a 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -982,7 +982,8 @@ static int verify_bitmap_size(uint64_t npages, uint64_t
bitmap_size)
}
static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
-struct vfio_iommu_type1_dma_unmap *unmap)
+struct vfio_iommu_type1_dma_unmap *unmap,
+struct vfio_bitmap *bitmap)
{
uint64_t mask;
struct vfio_dma *dma, *dma_last = NULL;
@@ -1033,6 +1034,10 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
* will be returned if these conditions are not met. The v2 interface
* will only return success and a size of zero if there were no
* mappings within the range.
+*
+* When VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP flag is set, unmap request
+* must be for single mapping. Multiple mappings with this flag set is
+* not supported.
*/
if (iommu->v2) {
dma = vfio_find_dma(iommu, unmap->iova, 1);
@@ -1040,6 +1045,13 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
ret = -EINVAL;
goto unlock;
}
+
+ if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
+ (dma->iova != unmap->iova || dma->size != unmap->size)) {
potential NULL pointer!
And could you address the comments in v14?
How to handle DSI unmaps in vIOMMU
(https://lore.kernel.org/kvm/20200323011041.GB5456@joy-OptiPlex-7040/)
Sorry, I drafted reply to it, but I missed to send, it remained in my drafts
>
> it happens in vIOMMU Domain level invalidation of IOTLB
> (domain-selective invalidation, see vtd_iotlb_domain_invalidate() in
qemu).
> common in VTD lazy mode, and NOT just happening once at boot time.
> rather than invalidate page by page, it batches the page invalidation.
> so, when this invalidation takes place, even higher level page tables
> have been invalid and therefore it has to invalidate a bigger
combined range.
> That's why we see IOVAs are mapped in 4k pages, but are unmapped in 2M
> pages.
>
> I think those UNMAPs should also have GET_DIRTY_BIMTAP flag on, right?
vtd_iotlb_domain_invalidate()
vtd_sync_shadow_page_table()
vtd_sync_shadow_page_table_range(vtd_as, &ce, 0, UINT64_MAX)
vtd_page_walk()
vtd_page_walk_level() - walk over specific level for IOVA range
vtd_page_walk_one()
memory_region_notify_iommu()
...
vfio_iommu_map_notify()
In the above trace, isn't page walk will take care of creating proper
IOTLB entry which should be same as created during mapping for that
IOTLB entry?
No. It does walk the page table, but as it's dsi (delay & batched unmap),
pages table entry for a whole 2M (the higher level, not last level for 4K)
range is invalid, so the iotlb->addr_mask what vfio_iommu_map_notify()
receives is (2M - 1), not the same as the size for map.
When do this happen? during my testing I never hit this case. How can I
hit this case?
In this case, will adjacent whole vfio_dmas will be clubbed together or
will there be any intersection of vfio_dmas?
Thanks,
Kirti
Re: [PATCH v16 Kernel 5/7] vfio iommu: Update UNMAP_DMA ioctl to get dirty bitmap before unmap
On Fri, Mar 27, 2020 at 05:39:44AM +0800, Kirti Wankhede wrote:
>
>
> On 3/25/2020 7:48 AM, Yan Zhao wrote:
> > On Wed, Mar 25, 2020 at 03:32:37AM +0800, Kirti Wankhede wrote:
> >> DMA mapped pages, including those pinned by mdev vendor drivers, might
> >> get unpinned and unmapped while migration is active and device is still
> >> running. For example, in pre-copy phase while guest driver could access
> >> those pages, host device or vendor driver can dirty these mapped pages.
> >> Such pages should be marked dirty so as to maintain memory consistency
> >> for a user making use of dirty page tracking.
> >>
> >> To get bitmap during unmap, user should allocate memory for bitmap, set
> >> size of allocated memory, set page size to be considered for bitmap and
> >> set flag VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP.
> >>
> >> Signed-off-by: Kirti Wankhede
> >> Reviewed-by: Neo Jia
> >> ---
> >> drivers/vfio/vfio_iommu_type1.c | 54
> >> ++---
> >> include/uapi/linux/vfio.h | 10
> >> 2 files changed, 60 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/vfio/vfio_iommu_type1.c
> >> b/drivers/vfio/vfio_iommu_type1.c
> >> index 27ed069c5053..b98a8d79e13a 100644
> >> --- a/drivers/vfio/vfio_iommu_type1.c
> >> +++ b/drivers/vfio/vfio_iommu_type1.c
> >> @@ -982,7 +982,8 @@ static int verify_bitmap_size(uint64_t npages,
> >> uint64_t bitmap_size)
> >> }
> >>
> >> static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> >> - struct vfio_iommu_type1_dma_unmap *unmap)
> >> + struct vfio_iommu_type1_dma_unmap *unmap,
> >> + struct vfio_bitmap *bitmap)
> >> {
> >>uint64_t mask;
> >>struct vfio_dma *dma, *dma_last = NULL;
> >> @@ -1033,6 +1034,10 @@ static int vfio_dma_do_unmap(struct vfio_iommu
> >> *iommu,
> >> * will be returned if these conditions are not met. The v2 interface
> >> * will only return success and a size of zero if there were no
> >> * mappings within the range.
> >> + *
> >> + * When VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP flag is set, unmap request
> >> + * must be for single mapping. Multiple mappings with this flag set is
> >> + * not supported.
> >> */
> >>if (iommu->v2) {
> >>dma = vfio_find_dma(iommu, unmap->iova, 1);
> >> @@ -1040,6 +1045,13 @@ static int vfio_dma_do_unmap(struct vfio_iommu
> >> *iommu,
> >>ret = -EINVAL;
> >>goto unlock;
> >>}
> >> +
> >> + if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
> >> + (dma->iova != unmap->iova || dma->size != unmap->size)) {
> > potential NULL pointer!
> >
> > And could you address the comments in v14?
> > How to handle DSI unmaps in vIOMMU
> > (https://lore.kernel.org/kvm/20200323011041.GB5456@joy-OptiPlex-7040/)
> >
>
> Sorry, I drafted reply to it, but I missed to send, it remained in my drafts
>
> >
> > it happens in vIOMMU Domain level invalidation of IOTLB
> > (domain-selective invalidation, see vtd_iotlb_domain_invalidate() in
> qemu).
> > common in VTD lazy mode, and NOT just happening once at boot time.
> > rather than invalidate page by page, it batches the page invalidation.
> > so, when this invalidation takes place, even higher level page tables
> > have been invalid and therefore it has to invalidate a bigger
> combined range.
> > That's why we see IOVAs are mapped in 4k pages, but are unmapped in 2M
> > pages.
> >
> > I think those UNMAPs should also have GET_DIRTY_BIMTAP flag on, right?
>
>
> vtd_iotlb_domain_invalidate()
>vtd_sync_shadow_page_table()
> vtd_sync_shadow_page_table_range(vtd_as, &ce, 0, UINT64_MAX)
>vtd_page_walk()
> vtd_page_walk_level() - walk over specific level for IOVA range
>vtd_page_walk_one()
> memory_region_notify_iommu()
> ...
>vfio_iommu_map_notify()
>
> In the above trace, isn't page walk will take care of creating proper
> IOTLB entry which should be same as created during mapping for that
> IOTLB entry?
>
No. It does walk the page table, but as it's dsi (delay & batched unmap),
pages table entry for a whole 2M (the higher level, not last level for 4K)
range is invalid, so the iotlb->addr_mask what vfio_iommu_map_notify()
receives is (2M - 1), not the same as the size for map.
>
> >>>
> >>> Such unmap would callback vfio_iommu_map_notify() in QEMU. In
> >>> vfio_iommu_map_notify(), unmap is called on same range >>> iotlb->addr_mask + 1> which was used for map. Secondly unmap with
> bitmap
> >>> will be called only when device state has _SAVING flag set.
> >>
> > in this case, iotlb->addr_mask in unmap is 0x20 -1.
> > different than 0x1000 -1 used for map.
> >> It might be helpful for Yan, and everyone else, to see the latest QEMU
> >> patch series. Thanks,
> >>
> > yes, please. also curious of log_sync part for vI
Re: [PATCH v16 Kernel 5/7] vfio iommu: Update UNMAP_DMA ioctl to get dirty bitmap before unmap
On 3/25/2020 7:48 AM, Yan Zhao wrote:
On Wed, Mar 25, 2020 at 03:32:37AM +0800, Kirti Wankhede wrote:
DMA mapped pages, including those pinned by mdev vendor drivers, might
get unpinned and unmapped while migration is active and device is still
running. For example, in pre-copy phase while guest driver could access
those pages, host device or vendor driver can dirty these mapped pages.
Such pages should be marked dirty so as to maintain memory consistency
for a user making use of dirty page tracking.
To get bitmap during unmap, user should allocate memory for bitmap, set
size of allocated memory, set page size to be considered for bitmap and
set flag VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP.
Signed-off-by: Kirti Wankhede
Reviewed-by: Neo Jia
---
drivers/vfio/vfio_iommu_type1.c | 54 ++---
include/uapi/linux/vfio.h | 10
2 files changed, 60 insertions(+), 4 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 27ed069c5053..b98a8d79e13a 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -982,7 +982,8 @@ static int verify_bitmap_size(uint64_t npages, uint64_t
bitmap_size)
}
static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
-struct vfio_iommu_type1_dma_unmap *unmap)
+struct vfio_iommu_type1_dma_unmap *unmap,
+struct vfio_bitmap *bitmap)
{
uint64_t mask;
struct vfio_dma *dma, *dma_last = NULL;
@@ -1033,6 +1034,10 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
* will be returned if these conditions are not met. The v2 interface
* will only return success and a size of zero if there were no
* mappings within the range.
+*
+* When VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP flag is set, unmap request
+* must be for single mapping. Multiple mappings with this flag set is
+* not supported.
*/
if (iommu->v2) {
dma = vfio_find_dma(iommu, unmap->iova, 1);
@@ -1040,6 +1045,13 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
ret = -EINVAL;
goto unlock;
}
+
+ if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
+ (dma->iova != unmap->iova || dma->size != unmap->size)) {
potential NULL pointer!
And could you address the comments in v14?
How to handle DSI unmaps in vIOMMU
(https://lore.kernel.org/kvm/20200323011041.GB5456@joy-OptiPlex-7040/)
Sorry, I drafted reply to it, but I missed to send, it remained in my drafts
>
> it happens in vIOMMU Domain level invalidation of IOTLB
> (domain-selective invalidation, see vtd_iotlb_domain_invalidate() in
qemu).
> common in VTD lazy mode, and NOT just happening once at boot time.
> rather than invalidate page by page, it batches the page invalidation.
> so, when this invalidation takes place, even higher level page tables
> have been invalid and therefore it has to invalidate a bigger
combined range.
> That's why we see IOVAs are mapped in 4k pages, but are unmapped in 2M
> pages.
>
> I think those UNMAPs should also have GET_DIRTY_BIMTAP flag on, right?
vtd_iotlb_domain_invalidate()
vtd_sync_shadow_page_table()
vtd_sync_shadow_page_table_range(vtd_as, &ce, 0, UINT64_MAX)
vtd_page_walk()
vtd_page_walk_level() - walk over specific level for IOVA range
vtd_page_walk_one()
memory_region_notify_iommu()
...
vfio_iommu_map_notify()
In the above trace, isn't page walk will take care of creating proper
IOTLB entry which should be same as created during mapping for that
IOTLB entry?
>>>
>>> Such unmap would callback vfio_iommu_map_notify() in QEMU. In
>>> vfio_iommu_map_notify(), unmap is called on same range >>> iotlb->addr_mask + 1> which was used for map. Secondly unmap with
bitmap
>>> will be called only when device state has _SAVING flag set.
>>
> in this case, iotlb->addr_mask in unmap is 0x20 -1.
> different than 0x1000 -1 used for map.
>> It might be helpful for Yan, and everyone else, to see the latest QEMU
>> patch series. Thanks,
>>
> yes, please. also curious of log_sync part for vIOMMU. given most
IOVAs in
> address space are unmapped and therefore no IOTLBs are able to be found.
>
Qemu patches compatible with v16 version are at:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg691806.html
Hope that helps.
Thanks,
Kirti
Re: [PATCH v16 Kernel 5/7] vfio iommu: Update UNMAP_DMA ioctl to get dirty bitmap before unmap
On Wed, Mar 25, 2020 at 03:32:37AM +0800, Kirti Wankhede wrote:
> DMA mapped pages, including those pinned by mdev vendor drivers, might
> get unpinned and unmapped while migration is active and device is still
> running. For example, in pre-copy phase while guest driver could access
> those pages, host device or vendor driver can dirty these mapped pages.
> Such pages should be marked dirty so as to maintain memory consistency
> for a user making use of dirty page tracking.
>
> To get bitmap during unmap, user should allocate memory for bitmap, set
> size of allocated memory, set page size to be considered for bitmap and
> set flag VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP.
>
> Signed-off-by: Kirti Wankhede
> Reviewed-by: Neo Jia
> ---
> drivers/vfio/vfio_iommu_type1.c | 54
> ++---
> include/uapi/linux/vfio.h | 10
> 2 files changed, 60 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 27ed069c5053..b98a8d79e13a 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -982,7 +982,8 @@ static int verify_bitmap_size(uint64_t npages, uint64_t
> bitmap_size)
> }
>
> static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> - struct vfio_iommu_type1_dma_unmap *unmap)
> + struct vfio_iommu_type1_dma_unmap *unmap,
> + struct vfio_bitmap *bitmap)
> {
> uint64_t mask;
> struct vfio_dma *dma, *dma_last = NULL;
> @@ -1033,6 +1034,10 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>* will be returned if these conditions are not met. The v2 interface
>* will only return success and a size of zero if there were no
>* mappings within the range.
> + *
> + * When VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP flag is set, unmap request
> + * must be for single mapping. Multiple mappings with this flag set is
> + * not supported.
>*/
> if (iommu->v2) {
> dma = vfio_find_dma(iommu, unmap->iova, 1);
> @@ -1040,6 +1045,13 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> ret = -EINVAL;
> goto unlock;
> }
> +
> + if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
> + (dma->iova != unmap->iova || dma->size != unmap->size)) {
potential NULL pointer!
And could you address the comments in v14?
How to handle DSI unmaps in vIOMMU
(https://lore.kernel.org/kvm/20200323011041.GB5456@joy-OptiPlex-7040/)
> + ret = -EINVAL;
> + goto unlock;
> + }
> +
> dma = vfio_find_dma(iommu, unmap->iova + unmap->size - 1, 0);
> if (dma && dma->iova + dma->size != unmap->iova + unmap->size) {
> ret = -EINVAL;
> @@ -1057,6 +1069,11 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> if (dma->task->mm != current->mm)
> break;
>
> + if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
> + iommu->dirty_page_tracking)
> + vfio_iova_dirty_bitmap(iommu, dma->iova, dma->size,
> + bitmap->pgsize, bitmap->data);
> +
> if (!RB_EMPTY_ROOT(&dma->pfn_list)) {
> struct vfio_iommu_type1_dma_unmap nb_unmap;
>
> @@ -2418,17 +2435,46 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>
> } else if (cmd == VFIO_IOMMU_UNMAP_DMA) {
> struct vfio_iommu_type1_dma_unmap unmap;
> - long ret;
> + struct vfio_bitmap bitmap = { 0 };
> + int ret;
>
> minsz = offsetofend(struct vfio_iommu_type1_dma_unmap, size);
>
> if (copy_from_user(&unmap, (void __user *)arg, minsz))
> return -EFAULT;
>
> - if (unmap.argsz < minsz || unmap.flags)
> + if (unmap.argsz < minsz ||
> + unmap.flags & ~VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP)
> return -EINVAL;
>
> - ret = vfio_dma_do_unmap(iommu, &unmap);
> + if (unmap.flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) {
> + unsigned long pgshift;
> + uint64_t iommu_pgsize =
> + 1 << __ffs(vfio_pgsize_bitmap(iommu));
> +
> + if (unmap.argsz < (minsz + sizeof(bitmap)))
> + return -EINVAL;
> +
> + if (copy_from_user(&bitmap,
> +(void __user *)(arg + minsz),
> +sizeof(bitmap)))
> + return -EFAULT;
> +
> + /* allow only min supported pgsize */
> + if (bitmap.pgsize !=
[PATCH v16 Kernel 5/7] vfio iommu: Update UNMAP_DMA ioctl to get dirty bitmap before unmap
DMA mapped pages, including those pinned by mdev vendor drivers, might
get unpinned and unmapped while migration is active and device is still
running. For example, in pre-copy phase while guest driver could access
those pages, host device or vendor driver can dirty these mapped pages.
Such pages should be marked dirty so as to maintain memory consistency
for a user making use of dirty page tracking.
To get bitmap during unmap, user should allocate memory for bitmap, set
size of allocated memory, set page size to be considered for bitmap and
set flag VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP.
Signed-off-by: Kirti Wankhede
Reviewed-by: Neo Jia
---
drivers/vfio/vfio_iommu_type1.c | 54 ++---
include/uapi/linux/vfio.h | 10
2 files changed, 60 insertions(+), 4 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 27ed069c5053..b98a8d79e13a 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -982,7 +982,8 @@ static int verify_bitmap_size(uint64_t npages, uint64_t
bitmap_size)
}
static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
-struct vfio_iommu_type1_dma_unmap *unmap)
+struct vfio_iommu_type1_dma_unmap *unmap,
+struct vfio_bitmap *bitmap)
{
uint64_t mask;
struct vfio_dma *dma, *dma_last = NULL;
@@ -1033,6 +1034,10 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
* will be returned if these conditions are not met. The v2 interface
* will only return success and a size of zero if there were no
* mappings within the range.
+*
+* When VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP flag is set, unmap request
+* must be for single mapping. Multiple mappings with this flag set is
+* not supported.
*/
if (iommu->v2) {
dma = vfio_find_dma(iommu, unmap->iova, 1);
@@ -1040,6 +1045,13 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
ret = -EINVAL;
goto unlock;
}
+
+ if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
+ (dma->iova != unmap->iova || dma->size != unmap->size)) {
+ ret = -EINVAL;
+ goto unlock;
+ }
+
dma = vfio_find_dma(iommu, unmap->iova + unmap->size - 1, 0);
if (dma && dma->iova + dma->size != unmap->iova + unmap->size) {
ret = -EINVAL;
@@ -1057,6 +1069,11 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
if (dma->task->mm != current->mm)
break;
+ if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
+iommu->dirty_page_tracking)
+ vfio_iova_dirty_bitmap(iommu, dma->iova, dma->size,
+ bitmap->pgsize, bitmap->data);
+
if (!RB_EMPTY_ROOT(&dma->pfn_list)) {
struct vfio_iommu_type1_dma_unmap nb_unmap;
@@ -2418,17 +2435,46 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
} else if (cmd == VFIO_IOMMU_UNMAP_DMA) {
struct vfio_iommu_type1_dma_unmap unmap;
- long ret;
+ struct vfio_bitmap bitmap = { 0 };
+ int ret;
minsz = offsetofend(struct vfio_iommu_type1_dma_unmap, size);
if (copy_from_user(&unmap, (void __user *)arg, minsz))
return -EFAULT;
- if (unmap.argsz < minsz || unmap.flags)
+ if (unmap.argsz < minsz ||
+ unmap.flags & ~VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP)
return -EINVAL;
- ret = vfio_dma_do_unmap(iommu, &unmap);
+ if (unmap.flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) {
+ unsigned long pgshift;
+ uint64_t iommu_pgsize =
+1 << __ffs(vfio_pgsize_bitmap(iommu));
+
+ if (unmap.argsz < (minsz + sizeof(bitmap)))
+ return -EINVAL;
+
+ if (copy_from_user(&bitmap,
+ (void __user *)(arg + minsz),
+ sizeof(bitmap)))
+ return -EFAULT;
+
+ /* allow only min supported pgsize */
+ if (bitmap.pgsize != iommu_pgsize)
+ return -EINVAL;
+ if (!access_ok((void __user *)bitmap.data, bitmap.size))
+ return -EINVAL;
+
+ pgshift = __ffs(bitmap.pgsize);
+ ret = verify_bitmap_size(unmap.size >> pgshift,
+
