| Message ID | 20231020222804.21850-17-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IOMMUFD Dirty Tracking | expand |
On Fri, Oct 20, 2023 at 11:28:02PM +0100, Joao Martins wrote: > +static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, > + unsigned int mockpt_id, unsigned long iova, > + size_t length, unsigned long page_size, > + void __user *uptr, u32 flags) > +{ > + unsigned long i, max = length / page_size; > + struct iommu_test_cmd *cmd = ucmd->cmd; > + struct iommufd_hw_pagetable *hwpt; > + struct mock_iommu_domain *mock; > + int rc, count = 0; > + > + if (iova % page_size || length % page_size || > + (uintptr_t)uptr % page_size) > + return -EINVAL; > + > + hwpt = get_md_pagetable(ucmd, mockpt_id, &mock); > + if (IS_ERR(hwpt)) > + return PTR_ERR(hwpt); > + > + if (!(mock->flags & MOCK_DIRTY_TRACK)) { > + rc = -EINVAL; > + goto out_put; > + } > + > + for (i = 0; i < max; i++) { > + unsigned long cur = iova + i * page_size; > + void *ent, *old; > + > + if (!test_bit(i, (unsigned long *) uptr)) > + continue; Is it okay to test_bit on a user pointer/page? Should we call get_user_pages or so? Thanks Nicolin
On 23/10/2023 21:08, Nicolin Chen wrote: > On Fri, Oct 20, 2023 at 11:28:02PM +0100, Joao Martins wrote: > >> +static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, >> + unsigned int mockpt_id, unsigned long iova, >> + size_t length, unsigned long page_size, >> + void __user *uptr, u32 flags) >> +{ >> + unsigned long i, max = length / page_size; >> + struct iommu_test_cmd *cmd = ucmd->cmd; >> + struct iommufd_hw_pagetable *hwpt; >> + struct mock_iommu_domain *mock; >> + int rc, count = 0; >> + >> + if (iova % page_size || length % page_size || >> + (uintptr_t)uptr % page_size) >> + return -EINVAL; >> + >> + hwpt = get_md_pagetable(ucmd, mockpt_id, &mock); >> + if (IS_ERR(hwpt)) >> + return PTR_ERR(hwpt); >> + >> + if (!(mock->flags & MOCK_DIRTY_TRACK)) { >> + rc = -EINVAL; >> + goto out_put; >> + } >> + >> + for (i = 0; i < max; i++) { >> + unsigned long cur = iova + i * page_size; >> + void *ent, *old; >> + >> + if (!test_bit(i, (unsigned long *) uptr)) >> + continue; > > Is it okay to test_bit on a user pointer/page? Should we call > get_user_pages or so? > Arggh, let me fix that. This is where it is failing the selftest for you? If so, I should paste a snippet for you to test.
On Mon, Oct 23, 2023 at 09:15:32PM +0100, Joao Martins wrote: > External email: Use caution opening links or attachments > > > On 23/10/2023 21:08, Nicolin Chen wrote: > > On Fri, Oct 20, 2023 at 11:28:02PM +0100, Joao Martins wrote: > > > >> +static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, > >> + unsigned int mockpt_id, unsigned long iova, > >> + size_t length, unsigned long page_size, > >> + void __user *uptr, u32 flags) > >> +{ > >> + unsigned long i, max = length / page_size; > >> + struct iommu_test_cmd *cmd = ucmd->cmd; > >> + struct iommufd_hw_pagetable *hwpt; > >> + struct mock_iommu_domain *mock; > >> + int rc, count = 0; > >> + > >> + if (iova % page_size || length % page_size || > >> + (uintptr_t)uptr % page_size) > >> + return -EINVAL; > >> + > >> + hwpt = get_md_pagetable(ucmd, mockpt_id, &mock); > >> + if (IS_ERR(hwpt)) > >> + return PTR_ERR(hwpt); > >> + > >> + if (!(mock->flags & MOCK_DIRTY_TRACK)) { > >> + rc = -EINVAL; > >> + goto out_put; > >> + } > >> + > >> + for (i = 0; i < max; i++) { > >> + unsigned long cur = iova + i * page_size; > >> + void *ent, *old; > >> + > >> + if (!test_bit(i, (unsigned long *) uptr)) > >> + continue; > > > > Is it okay to test_bit on a user pointer/page? Should we call > > get_user_pages or so? > > > Arggh, let me fix that. > > This is where it is failing the selftest for you? > > If so, I should paste a snippet for you to test. Yea, the crash seems to be caused by this. Possibly some memory debugging feature that I turned on caught this? I tried a test fix and the crash is gone (attaching at EOM). However, I still see other failures: # # RUN iommufd_dirty_tracking.domain_dirty128M.get_dirty_bitmap ... # # iommufd_utils.h:292:get_dirty_bitmap:Expected nr (32768) == out_dirty (13648) # # get_dirty_bitmap: Test terminated by assertion # # FAIL iommufd_dirty_tracking.domain_dirty128M.get_dirty_bitmap # not ok 147 iommufd_dirty_tracking.domain_dirty128M.get_dirty_bitmap # # RUN iommufd_dirty_tracking.domain_dirty256M.enforce_dirty ... # # OK iommufd_dirty_tracking.domain_dirty256M.enforce_dirty # ok 148 iommufd_dirty_tracking.domain_dirty256M.enforce_dirty # # RUN iommufd_dirty_tracking.domain_dirty256M.set_dirty_tracking ... # # OK iommufd_dirty_tracking.domain_dirty256M.set_dirty_tracking # ok 149 iommufd_dirty_tracking.domain_dirty256M.set_dirty_tracking # # RUN iommufd_dirty_tracking.domain_dirty256M.device_dirty_capability ... # # OK iommufd_dirty_tracking.domain_dirty256M.device_dirty_capability # ok 150 iommufd_dirty_tracking.domain_dirty256M.device_dirty_capability # # RUN iommufd_dirty_tracking.domain_dirty256M.get_dirty_bitmap ... # # iommufd_utils.h:292:get_dirty_bitmap:Expected nr (65536) == out_dirty (8923) Maybe page_size isn't the right size? -------------attaching copy_from_user------------ diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c index 8a2c7df85441..daa198809d61 100644 --- a/drivers/iommu/iommufd/selftest.c +++ b/drivers/iommu/iommufd/selftest.c @@ -1103,8 +1103,9 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id, struct iommufd_hw_pagetable *hwpt; struct mock_iommu_domain *mock; int rc, count = 0; + void *tmp; - if (iova % page_size || length % page_size || + if (iova % page_size || length % page_size || !uptr || (uintptr_t)uptr % page_size) return -EINVAL; @@ -1117,11 +1118,22 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id, goto out_put; } + tmp = kvzalloc(page_size, GFP_KERNEL_ACCOUNT); + if (!tmp) { + rc = -ENOMEM; + goto out_put; + } + for (i = 0; i < max; i++) { unsigned long cur = iova + i * page_size; void *ent, *old; - if (!test_bit(i, (unsigned long *)uptr)) + if (copy_from_user(tmp, uptr, page_size)) { + rc = -EFAULT; + goto out_free; + } + + if (!test_bit(i, (unsigned long *)tmp)) continue; ent = xa_load(&mock->pfns, cur / page_size); @@ -1138,6 +1150,8 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id, cmd->dirty.out_nr_dirty = count; rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd)); +out_free: + kvfree(tmp); out_put: iommufd_put_object(&hwpt->obj); return rc;
On 23/10/2023 21:37, Nicolin Chen wrote: > On Mon, Oct 23, 2023 at 09:15:32PM +0100, Joao Martins wrote: >> External email: Use caution opening links or attachments >> >> >> On 23/10/2023 21:08, Nicolin Chen wrote: >>> On Fri, Oct 20, 2023 at 11:28:02PM +0100, Joao Martins wrote: >>> >>>> +static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, >>>> + unsigned int mockpt_id, unsigned long iova, >>>> + size_t length, unsigned long page_size, >>>> + void __user *uptr, u32 flags) >>>> +{ >>>> + unsigned long i, max = length / page_size; >>>> + struct iommu_test_cmd *cmd = ucmd->cmd; >>>> + struct iommufd_hw_pagetable *hwpt; >>>> + struct mock_iommu_domain *mock; >>>> + int rc, count = 0; >>>> + >>>> + if (iova % page_size || length % page_size || >>>> + (uintptr_t)uptr % page_size) >>>> + return -EINVAL; >>>> + >>>> + hwpt = get_md_pagetable(ucmd, mockpt_id, &mock); >>>> + if (IS_ERR(hwpt)) >>>> + return PTR_ERR(hwpt); >>>> + >>>> + if (!(mock->flags & MOCK_DIRTY_TRACK)) { >>>> + rc = -EINVAL; >>>> + goto out_put; >>>> + } >>>> + >>>> + for (i = 0; i < max; i++) { >>>> + unsigned long cur = iova + i * page_size; >>>> + void *ent, *old; >>>> + >>>> + if (!test_bit(i, (unsigned long *) uptr)) >>>> + continue; >>> >>> Is it okay to test_bit on a user pointer/page? Should we call >>> get_user_pages or so? >>> >> Arggh, let me fix that. >> >> This is where it is failing the selftest for you? >> >> If so, I should paste a snippet for you to test. > > Yea, the crash seems to be caused by this. Possibly some memory > debugging feature that I turned on caught this? > > I tried a test fix and the crash is gone (attaching at EOM). > > However, I still see other failures: > # # RUN iommufd_dirty_tracking.domain_dirty128M.get_dirty_bitmap ... > # # iommufd_utils.h:292:get_dirty_bitmap:Expected nr (32768) == out_dirty (13648) > # # get_dirty_bitmap: Test terminated by assertion > # # FAIL iommufd_dirty_tracking.domain_dirty128M.get_dirty_bitmap > # not ok 147 iommufd_dirty_tracking.domain_dirty128M.get_dirty_bitmap > # # RUN iommufd_dirty_tracking.domain_dirty256M.enforce_dirty ... > # # OK iommufd_dirty_tracking.domain_dirty256M.enforce_dirty > # ok 148 iommufd_dirty_tracking.domain_dirty256M.enforce_dirty > # # RUN iommufd_dirty_tracking.domain_dirty256M.set_dirty_tracking ... > # # OK iommufd_dirty_tracking.domain_dirty256M.set_dirty_tracking > # ok 149 iommufd_dirty_tracking.domain_dirty256M.set_dirty_tracking > # # RUN iommufd_dirty_tracking.domain_dirty256M.device_dirty_capability ... > # # OK iommufd_dirty_tracking.domain_dirty256M.device_dirty_capability > # ok 150 iommufd_dirty_tracking.domain_dirty256M.device_dirty_capability > # # RUN iommufd_dirty_tracking.domain_dirty256M.get_dirty_bitmap ... > # # iommufd_utils.h:292:get_dirty_bitmap:Expected nr (65536) == out_dirty (8923) > > > Maybe page_size isn't the right size? > You are probably just not copying it right. The bitmap APIs treat the pointer as one big array of ulongs and set the right word of it, so your copy_from_user needs to make sure it is copying from the right offset. Given that the tests (of different sizes) exercise the boundaries of the bitmap it eventually exposes. The 256M specifically it could be that I an testing the 2 PAGE_SIZE bitmap, that I offset on purpose (as part of the test). Let me play with it in the meantime and I will paste an diff based on yours. Thanks a lot for digging it through > -------------attaching copy_from_user------------ > diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c > index 8a2c7df85441..daa198809d61 100644 > --- a/drivers/iommu/iommufd/selftest.c > +++ b/drivers/iommu/iommufd/selftest.c > @@ -1103,8 +1103,9 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id, > struct iommufd_hw_pagetable *hwpt; > struct mock_iommu_domain *mock; > int rc, count = 0; > + void *tmp; > > - if (iova % page_size || length % page_size || > + if (iova % page_size || length % page_size || !uptr || > (uintptr_t)uptr % page_size) > return -EINVAL; > > @@ -1117,11 +1118,22 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id, > goto out_put; > } > > + tmp = kvzalloc(page_size, GFP_KERNEL_ACCOUNT); > + if (!tmp) { > + rc = -ENOMEM; > + goto out_put; > + } > + > for (i = 0; i < max; i++) { > unsigned long cur = iova + i * page_size; > void *ent, *old; > > - if (!test_bit(i, (unsigned long *)uptr)) > + if (copy_from_user(tmp, uptr, page_size)) { > + rc = -EFAULT; > + goto out_free; > + } > + > + if (!test_bit(i, (unsigned long *)tmp)) > continue; > > ent = xa_load(&mock->pfns, cur / page_size); > @@ -1138,6 +1150,8 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id, > > cmd->dirty.out_nr_dirty = count; > rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd)); > +out_free: > + kvfree(tmp); > out_put: > iommufd_put_object(&hwpt->obj); > return rc;
On 23/10/2023 21:50, Joao Martins wrote: > On 23/10/2023 21:37, Nicolin Chen wrote: >> On Mon, Oct 23, 2023 at 09:15:32PM +0100, Joao Martins wrote: >>> On 23/10/2023 21:08, Nicolin Chen wrote: >>>> On Fri, Oct 20, 2023 at 11:28:02PM +0100, Joao Martins wrote: >>>> >>>>> +static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, >>>>> + unsigned int mockpt_id, unsigned long iova, >>>>> + size_t length, unsigned long page_size, >>>>> + void __user *uptr, u32 flags) >>>>> +{ >>>>> + unsigned long i, max = length / page_size; >>>>> + struct iommu_test_cmd *cmd = ucmd->cmd; >>>>> + struct iommufd_hw_pagetable *hwpt; >>>>> + struct mock_iommu_domain *mock; >>>>> + int rc, count = 0; >>>>> + >>>>> + if (iova % page_size || length % page_size || >>>>> + (uintptr_t)uptr % page_size) >>>>> + return -EINVAL; >>>>> + >>>>> + hwpt = get_md_pagetable(ucmd, mockpt_id, &mock); >>>>> + if (IS_ERR(hwpt)) >>>>> + return PTR_ERR(hwpt); >>>>> + >>>>> + if (!(mock->flags & MOCK_DIRTY_TRACK)) { >>>>> + rc = -EINVAL; >>>>> + goto out_put; >>>>> + } >>>>> + >>>>> + for (i = 0; i < max; i++) { >>>>> + unsigned long cur = iova + i * page_size; >>>>> + void *ent, *old; >>>>> + >>>>> + if (!test_bit(i, (unsigned long *) uptr)) >>>>> + continue; >>>> >>>> Is it okay to test_bit on a user pointer/page? Should we call >>>> get_user_pages or so? >>>> >>> Arggh, let me fix that. >>> >>> This is where it is failing the selftest for you? >>> >>> If so, I should paste a snippet for you to test. >> >> Yea, the crash seems to be caused by this. Possibly some memory >> debugging feature that I turned on caught this? >> >> I tried a test fix and the crash is gone (attaching at EOM). >> >> However, I still see other failures: >> # # RUN iommufd_dirty_tracking.domain_dirty128M.get_dirty_bitmap ... >> # # iommufd_utils.h:292:get_dirty_bitmap:Expected nr (32768) == out_dirty (13648) >> # # get_dirty_bitmap: Test terminated by assertion >> # # FAIL iommufd_dirty_tracking.domain_dirty128M.get_dirty_bitmap >> # not ok 147 iommufd_dirty_tracking.domain_dirty128M.get_dirty_bitmap >> # # RUN iommufd_dirty_tracking.domain_dirty256M.enforce_dirty ... >> # # OK iommufd_dirty_tracking.domain_dirty256M.enforce_dirty >> # ok 148 iommufd_dirty_tracking.domain_dirty256M.enforce_dirty >> # # RUN iommufd_dirty_tracking.domain_dirty256M.set_dirty_tracking ... >> # # OK iommufd_dirty_tracking.domain_dirty256M.set_dirty_tracking >> # ok 149 iommufd_dirty_tracking.domain_dirty256M.set_dirty_tracking >> # # RUN iommufd_dirty_tracking.domain_dirty256M.device_dirty_capability ... >> # # OK iommufd_dirty_tracking.domain_dirty256M.device_dirty_capability >> # ok 150 iommufd_dirty_tracking.domain_dirty256M.device_dirty_capability >> # # RUN iommufd_dirty_tracking.domain_dirty256M.get_dirty_bitmap ... >> # # iommufd_utils.h:292:get_dirty_bitmap:Expected nr (65536) == out_dirty (8923) >> >> >> Maybe page_size isn't the right size? >> > > You are probably just not copying it right. > > The bitmap APIs treat the pointer as one big array of ulongs and set the right > word of it, so your copy_from_user needs to make sure it is copying from the > right offset. > > Given that the tests (of different sizes) exercise the boundaries of the bitmap > it eventually exposes. The 256M specifically it could be that I an testing the 2 > PAGE_SIZE bitmap, that I offset on purpose (as part of the test). > > Let me play with it in the meantime and I will paste an diff based on yours. This is based on your snippet, except that we just copy the whole thing instead of per chunk. Should make it less error-prone than to calculate offsets. Could you try it out and see if it works for you? Meanwhile I found out that I was checking the uptr (bitmap pointer) alignment against page_size which didn't make sense. diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c index 8a2c7df85441..d8551c9d5b6c 100644 --- a/drivers/iommu/iommufd/selftest.c +++ b/drivers/iommu/iommufd/selftest.c @@ -1098,14 +1098,14 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id, unsigned long page_size, void __user *uptr, u32 flags) { - unsigned long i, max = length / page_size; + unsigned long bitmap_size, i, max = length / page_size; struct iommu_test_cmd *cmd = ucmd->cmd; struct iommufd_hw_pagetable *hwpt; struct mock_iommu_domain *mock; int rc, count = 0; + void *tmp; - if (iova % page_size || length % page_size || - (uintptr_t)uptr % page_size) + if (iova % page_size || length % page_size || !uptr) return -EINVAL; hwpt = get_md_pagetable(ucmd, mockpt_id, &mock); @@ -1117,11 +1117,24 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id, goto out_put; } + bitmap_size = max / BITS_PER_BYTE; + + tmp = kvzalloc(bitmap_size, GFP_KERNEL_ACCOUNT); + if (!tmp) { + rc = -ENOMEM; + goto out_put; + } + + if (copy_from_user(tmp, uptr, bitmap_size)) { + rc = -EFAULT; + goto out_free; + } + for (i = 0; i < max; i++) { unsigned long cur = iova + i * page_size; void *ent, *old; - if (!test_bit(i, (unsigned long *)uptr)) + if (!test_bit(i, (unsigned long *)tmp)) continue; ent = xa_load(&mock->pfns, cur / page_size); @@ -1138,6 +1151,8 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id, cmd->dirty.out_nr_dirty = count; rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd)); +out_free: + kvfree(tmp); out_put: iommufd_put_object(&hwpt->obj); return rc;
On Mon, Oct 23, 2023 at 10:46:19PM +0100, Joao Martins wrote: > > You are probably just not copying it right. > > > > The bitmap APIs treat the pointer as one big array of ulongs and set the right > > word of it, so your copy_from_user needs to make sure it is copying from the > > right offset. > > > > Given that the tests (of different sizes) exercise the boundaries of the bitmap > > it eventually exposes. The 256M specifically it could be that I an testing the 2 > > PAGE_SIZE bitmap, that I offset on purpose (as part of the test). > > > > Let me play with it in the meantime and I will paste an diff based on yours. > > This is based on your snippet, except that we just copy the whole thing instead > of per chunk. Should make it less error-prone than to calculate offsets. Could > you try it out and see if it works for you? Meanwhile I found out that I was > checking the uptr (bitmap pointer) alignment against page_size which didn't make > sense. This fixes everything, per my test results. Let's have all of these incremental fixes in v6. Cheers Nicolin > diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c > index 8a2c7df85441..d8551c9d5b6c 100644 > --- a/drivers/iommu/iommufd/selftest.c > +++ b/drivers/iommu/iommufd/selftest.c > @@ -1098,14 +1098,14 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, > unsigned int mockpt_id, > unsigned long page_size, void __user *uptr, > u32 flags) > { > - unsigned long i, max = length / page_size; > + unsigned long bitmap_size, i, max = length / page_size; > struct iommu_test_cmd *cmd = ucmd->cmd; > struct iommufd_hw_pagetable *hwpt; > struct mock_iommu_domain *mock; > int rc, count = 0; > + void *tmp; > > - if (iova % page_size || length % page_size || > - (uintptr_t)uptr % page_size) > + if (iova % page_size || length % page_size || !uptr) > return -EINVAL; > > hwpt = get_md_pagetable(ucmd, mockpt_id, &mock); > @@ -1117,11 +1117,24 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, > unsigned int mockpt_id, > goto out_put; > } > > + bitmap_size = max / BITS_PER_BYTE; > + > + tmp = kvzalloc(bitmap_size, GFP_KERNEL_ACCOUNT); > + if (!tmp) { > + rc = -ENOMEM; > + goto out_put; > + } > + > + if (copy_from_user(tmp, uptr, bitmap_size)) { > + rc = -EFAULT; > + goto out_free; > + } > + > for (i = 0; i < max; i++) { > unsigned long cur = iova + i * page_size; > void *ent, *old; > > - if (!test_bit(i, (unsigned long *)uptr)) > + if (!test_bit(i, (unsigned long *)tmp)) > continue; > > ent = xa_load(&mock->pfns, cur / page_size); > @@ -1138,6 +1151,8 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, > unsigned int mockpt_id, > > cmd->dirty.out_nr_dirty = count; > rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd)); > +out_free: > + kvfree(tmp); > out_put: > iommufd_put_object(&hwpt->obj); > return rc;
diff --git a/drivers/iommu/iommufd/iommufd_test.h b/drivers/iommu/iommufd/iommufd_test.h index 9817edcd8968..1f2e93d3d4e8 100644 --- a/drivers/iommu/iommufd/iommufd_test.h +++ b/drivers/iommu/iommufd/iommufd_test.h @@ -20,6 +20,7 @@ enum { IOMMU_TEST_OP_MOCK_DOMAIN_REPLACE, IOMMU_TEST_OP_ACCESS_REPLACE_IOAS, IOMMU_TEST_OP_MOCK_DOMAIN_FLAGS, + IOMMU_TEST_OP_DIRTY, }; enum { @@ -107,6 +108,14 @@ struct iommu_test_cmd { struct { __u32 ioas_id; } access_replace_ioas; + struct { + __u32 flags; + __aligned_u64 iova; + __aligned_u64 length; + __aligned_u64 page_size; + __aligned_u64 uptr; + __aligned_u64 out_nr_dirty; + } dirty; }; __u32 last; }; diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c index c93b19e20c59..f08376a102e8 100644 --- a/drivers/iommu/iommufd/selftest.c +++ b/drivers/iommu/iommufd/selftest.c @@ -37,6 +37,7 @@ enum { _MOCK_PFN_START = MOCK_PFN_MASK + 1, MOCK_PFN_START_IOVA = _MOCK_PFN_START, MOCK_PFN_LAST_IOVA = _MOCK_PFN_START, + MOCK_PFN_DIRTY_IOVA = _MOCK_PFN_START << 1, }; /* @@ -181,6 +182,31 @@ static int mock_domain_read_and_clear_dirty(struct iommu_domain *domain, unsigned long flags, struct iommu_dirty_bitmap *dirty) { + struct mock_iommu_domain *mock = + container_of(domain, struct mock_iommu_domain, domain); + unsigned long i, max = size / MOCK_IO_PAGE_SIZE; + void *ent, *old; + + if (!(mock->flags & MOCK_DIRTY_TRACK) && dirty->bitmap) + return -EINVAL; + + for (i = 0; i < max; i++) { + unsigned long cur = iova + i * MOCK_IO_PAGE_SIZE; + + ent = xa_load(&mock->pfns, cur / MOCK_IO_PAGE_SIZE); + if (ent && + (xa_to_value(ent) & MOCK_PFN_DIRTY_IOVA)) { + unsigned long val; + + /* Clear dirty */ + val = xa_to_value(ent) & ~MOCK_PFN_DIRTY_IOVA; + old = xa_store(&mock->pfns, cur / MOCK_IO_PAGE_SIZE, + xa_mk_value(val), GFP_KERNEL); + WARN_ON_ONCE(ent != old); + iommu_dirty_bitmap_record(dirty, cur, MOCK_IO_PAGE_SIZE); + } + } + return 0; } @@ -313,7 +339,7 @@ static size_t mock_domain_unmap_pages(struct iommu_domain *domain, for (cur = 0; cur != pgsize; cur += MOCK_IO_PAGE_SIZE) { ent = xa_erase(&mock->pfns, iova / MOCK_IO_PAGE_SIZE); - WARN_ON(!ent); + /* * iommufd generates unmaps that must be a strict * superset of the map's performend So every starting @@ -323,12 +349,12 @@ static size_t mock_domain_unmap_pages(struct iommu_domain *domain, * passed to map_pages */ if (first) { - WARN_ON(!(xa_to_value(ent) & + WARN_ON(ent && !(xa_to_value(ent) & MOCK_PFN_START_IOVA)); first = false; } if (pgcount == 1 && cur + MOCK_IO_PAGE_SIZE == pgsize) - WARN_ON(!(xa_to_value(ent) & + WARN_ON(ent && !(xa_to_value(ent) & MOCK_PFN_LAST_IOVA)); iova += MOCK_IO_PAGE_SIZE; @@ -1056,6 +1082,56 @@ static_assert((unsigned int)MOCK_ACCESS_RW_WRITE == IOMMUFD_ACCESS_RW_WRITE); static_assert((unsigned int)MOCK_ACCESS_RW_SLOW_PATH == __IOMMUFD_ACCESS_RW_SLOW_PATH); +static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, + unsigned int mockpt_id, unsigned long iova, + size_t length, unsigned long page_size, + void __user *uptr, u32 flags) +{ + unsigned long i, max = length / page_size; + struct iommu_test_cmd *cmd = ucmd->cmd; + struct iommufd_hw_pagetable *hwpt; + struct mock_iommu_domain *mock; + int rc, count = 0; + + if (iova % page_size || length % page_size || + (uintptr_t)uptr % page_size) + return -EINVAL; + + hwpt = get_md_pagetable(ucmd, mockpt_id, &mock); + if (IS_ERR(hwpt)) + return PTR_ERR(hwpt); + + if (!(mock->flags & MOCK_DIRTY_TRACK)) { + rc = -EINVAL; + goto out_put; + } + + for (i = 0; i < max; i++) { + unsigned long cur = iova + i * page_size; + void *ent, *old; + + if (!test_bit(i, (unsigned long *) uptr)) + continue; + + ent = xa_load(&mock->pfns, cur / page_size); + if (ent) { + unsigned long val; + + val = xa_to_value(ent) | MOCK_PFN_DIRTY_IOVA; + old = xa_store(&mock->pfns, cur / page_size, + xa_mk_value(val), GFP_KERNEL); + WARN_ON_ONCE(ent != old); + count++; + } + } + + cmd->dirty.out_nr_dirty = count; + rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd)); +out_put: + iommufd_put_object(&hwpt->obj); + return rc; +} + void iommufd_selftest_destroy(struct iommufd_object *obj) { struct selftest_obj *sobj = container_of(obj, struct selftest_obj, obj); @@ -1121,6 +1197,12 @@ int iommufd_test(struct iommufd_ucmd *ucmd) return -EINVAL; iommufd_test_memory_limit = cmd->memory_limit.limit; return 0; + case IOMMU_TEST_OP_DIRTY: + return iommufd_test_dirty( + ucmd, cmd->id, cmd->dirty.iova, + cmd->dirty.length, cmd->dirty.page_size, + u64_to_user_ptr(cmd->dirty.uptr), + cmd->dirty.flags); default: return -EOPNOTSUPP; } diff --git a/tools/testing/selftests/iommu/iommufd.c b/tools/testing/selftests/iommu/iommufd.c index c921b50eddc8..96837369a0aa 100644 --- a/tools/testing/selftests/iommu/iommufd.c +++ b/tools/testing/selftests/iommu/iommufd.c @@ -12,6 +12,7 @@ static unsigned long HUGEPAGE_SIZE; #define MOCK_PAGE_SIZE (PAGE_SIZE / 2) +#define BITS_PER_BYTE 8 static unsigned long get_huge_page_size(void) { @@ -1440,13 +1441,47 @@ FIXTURE(iommufd_dirty_tracking) uint32_t hwpt_id; uint32_t stdev_id; uint32_t idev_id; + unsigned long page_size; + unsigned long bitmap_size; + void *bitmap; + void *buffer; +}; + +FIXTURE_VARIANT(iommufd_dirty_tracking) +{ + unsigned long buffer_size; }; FIXTURE_SETUP(iommufd_dirty_tracking) { + void *vrc; + int rc; + self->fd = open("/dev/iommu", O_RDWR); ASSERT_NE(-1, self->fd); + rc = posix_memalign(&self->buffer, HUGEPAGE_SIZE, variant->buffer_size); + if (rc || !self->buffer) { + SKIP(return, "Skipping buffer_size=%lu due to errno=%d", + variant->buffer_size, rc); + } + + assert((uintptr_t)self->buffer % HUGEPAGE_SIZE == 0); + vrc = mmap(self->buffer, variant->buffer_size, PROT_READ | PROT_WRITE, + MAP_SHARED | MAP_ANONYMOUS | MAP_FIXED, -1, 0); + assert(vrc == self->buffer); + + self->page_size = MOCK_PAGE_SIZE; + self->bitmap_size = variant->buffer_size / + self->page_size / BITS_PER_BYTE; + + /* Provision with an extra (MOCK_PAGE_SIZE) for the unaligned case */ + rc = posix_memalign(&self->bitmap, PAGE_SIZE, + self->bitmap_size + MOCK_PAGE_SIZE); + assert(!rc); + assert(self->bitmap); + assert((uintptr_t)self->bitmap % PAGE_SIZE == 0); + test_ioctl_ioas_alloc(&self->ioas_id); test_cmd_mock_domain(self->ioas_id, &self->stdev_id, &self->hwpt_id, &self->idev_id); @@ -1454,9 +1489,41 @@ FIXTURE_SETUP(iommufd_dirty_tracking) FIXTURE_TEARDOWN(iommufd_dirty_tracking) { + munmap(self->buffer, variant->buffer_size); + munmap(self->bitmap, self->bitmap_size); teardown_iommufd(self->fd, _metadata); } +FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty128k) +{ + /* one u32 index bitmap */ + .buffer_size = 128UL * 1024UL, +}; + +FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty256k) +{ + /* one u64 index bitmap */ + .buffer_size = 256UL * 1024UL, +}; + +FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty640k) +{ + /* two u64 index and trailing end bitmap */ + .buffer_size = 640UL * 1024UL, +}; + +FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty128M) +{ + /* 4K bitmap (128M IOVA range) */ + .buffer_size = 128UL * 1024UL * 1024UL, +}; + +FIXTURE_VARIANT_ADD(iommufd_dirty_tracking, domain_dirty256M) +{ + /* 8K bitmap (256M IOVA range) */ + .buffer_size = 256UL * 1024UL * 1024UL, +}; + TEST_F(iommufd_dirty_tracking, enforce_dirty) { uint32_t ioas_id, stddev_id, idev_id; @@ -1497,6 +1564,38 @@ TEST_F(iommufd_dirty_tracking, set_dirty_tracking) test_ioctl_destroy(hwpt_id); } +TEST_F(iommufd_dirty_tracking, get_dirty_bitmap) +{ + uint32_t stddev_id; + uint32_t hwpt_id; + uint32_t ioas_id; + + test_ioctl_ioas_alloc(&ioas_id); + test_ioctl_ioas_map_fixed_id(ioas_id, self->buffer, + variant->buffer_size, + MOCK_APERTURE_START); + + test_cmd_hwpt_alloc(self->idev_id, ioas_id, + IOMMU_HWPT_ALLOC_DIRTY_TRACKING, &hwpt_id); + test_cmd_mock_domain(hwpt_id, &stddev_id, NULL, NULL); + + test_cmd_set_dirty_tracking(hwpt_id, true); + + test_mock_dirty_bitmaps(hwpt_id, variant->buffer_size, + MOCK_APERTURE_START, + self->page_size, self->bitmap, + self->bitmap_size, _metadata); + + /* PAGE_SIZE unaligned bitmap */ + test_mock_dirty_bitmaps(hwpt_id, variant->buffer_size, + MOCK_APERTURE_START, + self->page_size, self->bitmap + MOCK_PAGE_SIZE, + self->bitmap_size, _metadata); + + test_ioctl_destroy(stddev_id); + test_ioctl_destroy(hwpt_id); +} + /* VFIO compatibility IOCTLs */ TEST_F(iommufd, simple_ioctls) diff --git a/tools/testing/selftests/iommu/iommufd_utils.h b/tools/testing/selftests/iommu/iommufd_utils.h index 757781d5c5a2..390563ff7935 100644 --- a/tools/testing/selftests/iommu/iommufd_utils.h +++ b/tools/testing/selftests/iommu/iommufd_utils.h @@ -9,6 +9,8 @@ #include <sys/ioctl.h> #include <stdint.h> #include <assert.h> +#include <linux/bitmap.h> +#include <linux/bitops.h> #include "../kselftest_harness.h" #include "../../../../drivers/iommu/iommufd/iommufd_test.h" @@ -197,6 +199,102 @@ static int _test_cmd_set_dirty_tracking(int fd, __u32 hwpt_id, bool enabled) #define test_cmd_set_dirty_tracking(hwpt_id, enabled) \ ASSERT_EQ(0, _test_cmd_set_dirty_tracking(self->fd, hwpt_id, enabled)) +static int _test_cmd_get_dirty_bitmap(int fd, __u32 hwpt_id, size_t length, + __u64 iova, size_t page_size, __u64 *bitmap) +{ + struct iommu_hwpt_get_dirty_bitmap cmd = { + .size = sizeof(cmd), + .hwpt_id = hwpt_id, + .iova = iova, + .length = length, + .page_size = page_size, + .data = bitmap, + }; + int ret; + + ret = ioctl(fd, IOMMU_HWPT_GET_DIRTY_BITMAP, &cmd); + if (ret) + return ret; + return 0; +} + +#define test_cmd_get_dirty_bitmap(fd, hwpt_id, length, iova, page_size, bitmap) \ + ASSERT_EQ(0, _test_cmd_get_dirty_bitmap(fd, hwpt_id, length, \ + iova, page_size, bitmap)) + +static int _test_cmd_mock_domain_set_dirty(int fd, __u32 hwpt_id, size_t length, + __u64 iova, size_t page_size, + __u64 *bitmap, __u64 *dirty) +{ + struct iommu_test_cmd cmd = { + .size = sizeof(cmd), + .op = IOMMU_TEST_OP_DIRTY, + .id = hwpt_id, + .dirty = { + .iova = iova, + .length = length, + .page_size = page_size, + .uptr = (uintptr_t) bitmap, + } + }; + int ret; + + ret = ioctl(fd, _IOMMU_TEST_CMD(IOMMU_TEST_OP_DIRTY), &cmd); + if (ret) + return -ret; + if (dirty) + *dirty = cmd.dirty.out_nr_dirty; + return 0; +} + +#define test_cmd_mock_domain_set_dirty(fd, hwpt_id, length, iova, page_size, bitmap, nr) \ + ASSERT_EQ(0, _test_cmd_mock_domain_set_dirty(fd, hwpt_id, \ + length, iova, \ + page_size, bitmap, \ + nr)) + +static int _test_mock_dirty_bitmaps(int fd, __u32 hwpt_id, size_t length, + __u64 iova, size_t page_size, + __u64 *bitmap, __u64 bitmap_size, + struct __test_metadata *_metadata) +{ + unsigned long i, count, nbits = bitmap_size * BITS_PER_BYTE; + unsigned long nr = nbits / 2; + __u64 out_dirty = 0; + + /* Mark all even bits as dirty in the mock domain */ + for (count = 0, i = 0; i < nbits; count += !(i%2), i++) + if (!(i % 2)) + __set_bit(i, (unsigned long *) bitmap); + ASSERT_EQ(nr, count); + + test_cmd_mock_domain_set_dirty(fd, hwpt_id, length, iova, page_size, + bitmap, &out_dirty); + ASSERT_EQ(nr, out_dirty); + + /* Expect all even bits as dirty in the user bitmap */ + memset(bitmap, 0, bitmap_size); + test_cmd_get_dirty_bitmap(fd, hwpt_id, length, iova, page_size, bitmap); + for (count = 0, i = 0; i < nbits; count += !(i%2), i++) + ASSERT_EQ(!(i % 2), test_bit(i, (unsigned long *) bitmap)); + ASSERT_EQ(count, out_dirty); + + memset(bitmap, 0, bitmap_size); + test_cmd_get_dirty_bitmap(fd, hwpt_id, length, iova, page_size, bitmap); + + /* It as read already -- expect all zeroes */ + for (i = 0; i < nbits; i++) + ASSERT_EQ(0, test_bit(i, (unsigned long *) bitmap)); + + return 0; +} +#define test_mock_dirty_bitmaps(hwpt_id, length, iova, page_size, bitmap, \ + bitmap_size, _metadata) \ + ASSERT_EQ(0, _test_mock_dirty_bitmaps(self->fd, hwpt_id, \ + length, iova, \ + page_size, bitmap, \ + bitmap_size, _metadata)) + static int _test_cmd_create_access(int fd, unsigned int ioas_id, __u32 *access_id, unsigned int flags) { @@ -321,6 +419,17 @@ static int _test_ioctl_ioas_map(int fd, unsigned int ioas_id, void *buffer, IOMMU_IOAS_MAP_READABLE)); \ }) +#define test_ioctl_ioas_map_fixed_id(ioas_id, buffer, length, iova) \ + ({ \ + __u64 __iova = iova; \ + ASSERT_EQ(0, _test_ioctl_ioas_map( \ + self->fd, ioas_id, buffer, length, \ + &__iova, \ + IOMMU_IOAS_MAP_FIXED_IOVA | \ + IOMMU_IOAS_MAP_WRITEABLE | \ + IOMMU_IOAS_MAP_READABLE)); \ + }) + #define test_err_ioctl_ioas_map_fixed(_errno, buffer, length, iova) \ ({ \ __u64 __iova = iova; \
Add a new test ioctl for simulating the dirty IOVAs in the mock domain, and implement the mock iommu domain ops that get the dirty tracking supported. The selftest exercises the usual main workflow of: 1) Setting dirty tracking from the iommu domain 2) Read and clear dirty IOPTEs Different fixtures will test different IOVA range sizes, that exercise corner cases of the bitmaps. Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- drivers/iommu/iommufd/iommufd_test.h | 9 ++ drivers/iommu/iommufd/selftest.c | 88 +++++++++++++- tools/testing/selftests/iommu/iommufd.c | 99 ++++++++++++++++ tools/testing/selftests/iommu/iommufd_utils.h | 109 ++++++++++++++++++ 4 files changed, 302 insertions(+), 3 deletions(-)