| Message ID | 1427468115-2224-5-git-send-email-aik@ozlabs.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Benjamin Herrenschmidt |
| Headers | show |
On Sat, 2015-03-28 at 01:54 +1100, Alexey Kardashevskiy wrote: > This makes use of the it_page_size from the iommu_table struct > as page size can differ. > > This replaces missing IOMMU_PAGE_SHIFT macro in commented debug code > as recently introduced IOMMU_PAGE_XXX macros do not include > IOMMU_PAGE_SHIFT. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > --- > drivers/vfio/vfio_iommu_spapr_tce.c | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c > index f835e63..8bbee22 100644 > --- a/drivers/vfio/vfio_iommu_spapr_tce.c > +++ b/drivers/vfio/vfio_iommu_spapr_tce.c > @@ -91,7 +91,7 @@ static int tce_iommu_enable(struct tce_container *container) > * enforcing the limit based on the max that the guest can map. > */ > down_write(¤t->mm->mmap_sem); > - npages = (tbl->it_size << IOMMU_PAGE_SHIFT_4K) >> PAGE_SHIFT; > + npages = (tbl->it_size << tbl->it_page_shift) >> PAGE_SHIFT; > locked = current->mm->locked_vm + npages; > lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; > if (locked > lock_limit && !capable(CAP_IPC_LOCK)) { > @@ -120,7 +120,7 @@ static void tce_iommu_disable(struct tce_container *container) > > down_write(¤t->mm->mmap_sem); > current->mm->locked_vm -= (container->tbl->it_size << > - IOMMU_PAGE_SHIFT_4K) >> PAGE_SHIFT; > + container->tbl->it_page_shift) >> PAGE_SHIFT; > up_write(¤t->mm->mmap_sem); > } > > @@ -222,7 +222,7 @@ static long tce_iommu_build(struct tce_container *container, > tce, ret); > break; > } > - tce += IOMMU_PAGE_SIZE_4K; > + tce += IOMMU_PAGE_SIZE(tbl); Is PAGE_SIZE ever smaller than IOMMU_PAGE_SIZE(tbl)? IOW, can the page we got from get_user_pages_fast() ever not completely fill the tce entry? (Have I asked this before? Sorry if so)
On 04/02/2015 08:48 AM, Alex Williamson wrote: > On Sat, 2015-03-28 at 01:54 +1100, Alexey Kardashevskiy wrote: >> This makes use of the it_page_size from the iommu_table struct >> as page size can differ. >> >> This replaces missing IOMMU_PAGE_SHIFT macro in commented debug code >> as recently introduced IOMMU_PAGE_XXX macros do not include >> IOMMU_PAGE_SHIFT. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> >> --- >> drivers/vfio/vfio_iommu_spapr_tce.c | 26 +++++++++++++------------- >> 1 file changed, 13 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c >> index f835e63..8bbee22 100644 >> --- a/drivers/vfio/vfio_iommu_spapr_tce.c >> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c >> @@ -91,7 +91,7 @@ static int tce_iommu_enable(struct tce_container *container) >> * enforcing the limit based on the max that the guest can map. >> */ >> down_write(¤t->mm->mmap_sem); >> - npages = (tbl->it_size << IOMMU_PAGE_SHIFT_4K) >> PAGE_SHIFT; >> + npages = (tbl->it_size << tbl->it_page_shift) >> PAGE_SHIFT; >> locked = current->mm->locked_vm + npages; >> lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; >> if (locked > lock_limit && !capable(CAP_IPC_LOCK)) { >> @@ -120,7 +120,7 @@ static void tce_iommu_disable(struct tce_container *container) >> >> down_write(¤t->mm->mmap_sem); >> current->mm->locked_vm -= (container->tbl->it_size << >> - IOMMU_PAGE_SHIFT_4K) >> PAGE_SHIFT; >> + container->tbl->it_page_shift) >> PAGE_SHIFT; >> up_write(¤t->mm->mmap_sem); >> } >> >> @@ -222,7 +222,7 @@ static long tce_iommu_build(struct tce_container *container, >> tce, ret); >> break; >> } >> - tce += IOMMU_PAGE_SIZE_4K; >> + tce += IOMMU_PAGE_SIZE(tbl); > > > Is PAGE_SIZE ever smaller than IOMMU_PAGE_SIZE(tbl)? IOW, can the page > we got from get_user_pages_fast() ever not completely fill the tce > entry? Yes. IOMMU_PAGE_SIZE is 4K/64K/16M (16M is with huge pages enabled in QEMU with -mempath), PAGE_SIZE is 4K/64K (normally 64K). > (Have I asked this before? Sorry if so) :)
On Thu, 2015-04-02 at 13:30 +1100, Alexey Kardashevskiy wrote: > On 04/02/2015 08:48 AM, Alex Williamson wrote: > > On Sat, 2015-03-28 at 01:54 +1100, Alexey Kardashevskiy wrote: > >> This makes use of the it_page_size from the iommu_table struct > >> as page size can differ. > >> > >> This replaces missing IOMMU_PAGE_SHIFT macro in commented debug code > >> as recently introduced IOMMU_PAGE_XXX macros do not include > >> IOMMU_PAGE_SHIFT. > >> > >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > >> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > >> --- > >> drivers/vfio/vfio_iommu_spapr_tce.c | 26 +++++++++++++------------- > >> 1 file changed, 13 insertions(+), 13 deletions(-) > >> > >> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c > >> index f835e63..8bbee22 100644 > >> --- a/drivers/vfio/vfio_iommu_spapr_tce.c > >> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c > >> @@ -91,7 +91,7 @@ static int tce_iommu_enable(struct tce_container *container) > >> * enforcing the limit based on the max that the guest can map. > >> */ > >> down_write(¤t->mm->mmap_sem); > >> - npages = (tbl->it_size << IOMMU_PAGE_SHIFT_4K) >> PAGE_SHIFT; > >> + npages = (tbl->it_size << tbl->it_page_shift) >> PAGE_SHIFT; > >> locked = current->mm->locked_vm + npages; > >> lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; > >> if (locked > lock_limit && !capable(CAP_IPC_LOCK)) { > >> @@ -120,7 +120,7 @@ static void tce_iommu_disable(struct tce_container *container) > >> > >> down_write(¤t->mm->mmap_sem); > >> current->mm->locked_vm -= (container->tbl->it_size << > >> - IOMMU_PAGE_SHIFT_4K) >> PAGE_SHIFT; > >> + container->tbl->it_page_shift) >> PAGE_SHIFT; > >> up_write(¤t->mm->mmap_sem); > >> } > >> > >> @@ -222,7 +222,7 @@ static long tce_iommu_build(struct tce_container *container, > >> tce, ret); > >> break; > >> } > >> - tce += IOMMU_PAGE_SIZE_4K; > >> + tce += IOMMU_PAGE_SIZE(tbl); > > > > > > Is PAGE_SIZE ever smaller than IOMMU_PAGE_SIZE(tbl)? IOW, can the page > > we got from get_user_pages_fast() ever not completely fill the tce > > entry? > > > Yes. IOMMU_PAGE_SIZE is 4K/64K/16M (16M is with huge pages enabled in QEMU > with -mempath), PAGE_SIZE is 4K/64K (normally 64K). Isn't that a problem then that you're filling the tce with processor page sizes via get_user_pages_fast(), but incrementing the tce by by IOMMU page size? For example, if PAGE_SIZE = 4K and IOMMU_PAGE_SIZE != 4K have we really pinned all of the memory backed by the tce? Where do you make sure the 4K page is really contiguous for the IOMMU page?
On 04/02/2015 01:50 PM, Alex Williamson wrote: > On Thu, 2015-04-02 at 13:30 +1100, Alexey Kardashevskiy wrote: >> On 04/02/2015 08:48 AM, Alex Williamson wrote: >>> On Sat, 2015-03-28 at 01:54 +1100, Alexey Kardashevskiy wrote: >>>> This makes use of the it_page_size from the iommu_table struct >>>> as page size can differ. >>>> >>>> This replaces missing IOMMU_PAGE_SHIFT macro in commented debug code >>>> as recently introduced IOMMU_PAGE_XXX macros do not include >>>> IOMMU_PAGE_SHIFT. >>>> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> >>>> --- >>>> drivers/vfio/vfio_iommu_spapr_tce.c | 26 +++++++++++++------------- >>>> 1 file changed, 13 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c >>>> index f835e63..8bbee22 100644 >>>> --- a/drivers/vfio/vfio_iommu_spapr_tce.c >>>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c >>>> @@ -91,7 +91,7 @@ static int tce_iommu_enable(struct tce_container *container) >>>> * enforcing the limit based on the max that the guest can map. >>>> */ >>>> down_write(¤t->mm->mmap_sem); >>>> - npages = (tbl->it_size << IOMMU_PAGE_SHIFT_4K) >> PAGE_SHIFT; >>>> + npages = (tbl->it_size << tbl->it_page_shift) >> PAGE_SHIFT; >>>> locked = current->mm->locked_vm + npages; >>>> lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; >>>> if (locked > lock_limit && !capable(CAP_IPC_LOCK)) { >>>> @@ -120,7 +120,7 @@ static void tce_iommu_disable(struct tce_container *container) >>>> >>>> down_write(¤t->mm->mmap_sem); >>>> current->mm->locked_vm -= (container->tbl->it_size << >>>> - IOMMU_PAGE_SHIFT_4K) >> PAGE_SHIFT; >>>> + container->tbl->it_page_shift) >> PAGE_SHIFT; >>>> up_write(¤t->mm->mmap_sem); >>>> } >>>> >>>> @@ -222,7 +222,7 @@ static long tce_iommu_build(struct tce_container *container, >>>> tce, ret); >>>> break; >>>> } >>>> - tce += IOMMU_PAGE_SIZE_4K; >>>> + tce += IOMMU_PAGE_SIZE(tbl); >>> >>> >>> Is PAGE_SIZE ever smaller than IOMMU_PAGE_SIZE(tbl)? IOW, can the page >>> we got from get_user_pages_fast() ever not completely fill the tce >>> entry? >> >> >> Yes. IOMMU_PAGE_SIZE is 4K/64K/16M (16M is with huge pages enabled in QEMU >> with -mempath), PAGE_SIZE is 4K/64K (normally 64K). > > Isn't that a problem then that you're filling the tce with processor > page sizes via get_user_pages_fast(), but incrementing the tce by by > IOMMU page size? For example, if PAGE_SIZE = 4K and IOMMU_PAGE_SIZE != > 4K have we really pinned all of the memory backed by the tce?Where do > you make sure the 4K page is really contiguous for the IOMMU page? Aaaah. This is just not supported. Instead, after the previous patch ("vfio: powerpc/spapr: Check that TCE page size is equal to it_page_size", which need fixed subject), tce_page_is_contained(page4K, 64K) will return false and the caller - tce_iommu_build() - will return -EPERM.
On Thu, 2015-04-02 at 14:40 +1100, Alexey Kardashevskiy wrote: > On 04/02/2015 01:50 PM, Alex Williamson wrote: > > On Thu, 2015-04-02 at 13:30 +1100, Alexey Kardashevskiy wrote: > >> On 04/02/2015 08:48 AM, Alex Williamson wrote: > >>> On Sat, 2015-03-28 at 01:54 +1100, Alexey Kardashevskiy wrote: > >>>> This makes use of the it_page_size from the iommu_table struct > >>>> as page size can differ. > >>>> > >>>> This replaces missing IOMMU_PAGE_SHIFT macro in commented debug code > >>>> as recently introduced IOMMU_PAGE_XXX macros do not include > >>>> IOMMU_PAGE_SHIFT. > >>>> > >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > >>>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > >>>> --- > >>>> drivers/vfio/vfio_iommu_spapr_tce.c | 26 +++++++++++++------------- > >>>> 1 file changed, 13 insertions(+), 13 deletions(-) > >>>> > >>>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c > >>>> index f835e63..8bbee22 100644 > >>>> --- a/drivers/vfio/vfio_iommu_spapr_tce.c > >>>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c > >>>> @@ -91,7 +91,7 @@ static int tce_iommu_enable(struct tce_container *container) > >>>> * enforcing the limit based on the max that the guest can map. > >>>> */ > >>>> down_write(¤t->mm->mmap_sem); > >>>> - npages = (tbl->it_size << IOMMU_PAGE_SHIFT_4K) >> PAGE_SHIFT; > >>>> + npages = (tbl->it_size << tbl->it_page_shift) >> PAGE_SHIFT; > >>>> locked = current->mm->locked_vm + npages; > >>>> lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; > >>>> if (locked > lock_limit && !capable(CAP_IPC_LOCK)) { > >>>> @@ -120,7 +120,7 @@ static void tce_iommu_disable(struct tce_container *container) > >>>> > >>>> down_write(¤t->mm->mmap_sem); > >>>> current->mm->locked_vm -= (container->tbl->it_size << > >>>> - IOMMU_PAGE_SHIFT_4K) >> PAGE_SHIFT; > >>>> + container->tbl->it_page_shift) >> PAGE_SHIFT; > >>>> up_write(¤t->mm->mmap_sem); > >>>> } > >>>> > >>>> @@ -222,7 +222,7 @@ static long tce_iommu_build(struct tce_container *container, > >>>> tce, ret); > >>>> break; > >>>> } > >>>> - tce += IOMMU_PAGE_SIZE_4K; > >>>> + tce += IOMMU_PAGE_SIZE(tbl); > >>> > >>> > >>> Is PAGE_SIZE ever smaller than IOMMU_PAGE_SIZE(tbl)? IOW, can the page > >>> we got from get_user_pages_fast() ever not completely fill the tce > >>> entry? > >> > >> > >> Yes. IOMMU_PAGE_SIZE is 4K/64K/16M (16M is with huge pages enabled in QEMU > >> with -mempath), PAGE_SIZE is 4K/64K (normally 64K). > > > > Isn't that a problem then that you're filling the tce with processor > > page sizes via get_user_pages_fast(), but incrementing the tce by by > > IOMMU page size? For example, if PAGE_SIZE = 4K and IOMMU_PAGE_SIZE != > > 4K have we really pinned all of the memory backed by the tce?Where do > > you make sure the 4K page is really contiguous for the IOMMU page? > > > Aaaah. This is just not supported. Instead, after the previous patch > ("vfio: powerpc/spapr: Check that TCE page size is equal to it_page_size", > which need fixed subject), tce_page_is_contained(page4K, 64K) will return > false and the caller - tce_iommu_build() - will return -EPERM. Ok, makes sense. Thanks
diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c index f835e63..8bbee22 100644 --- a/drivers/vfio/vfio_iommu_spapr_tce.c +++ b/drivers/vfio/vfio_iommu_spapr_tce.c @@ -91,7 +91,7 @@ static int tce_iommu_enable(struct tce_container *container) * enforcing the limit based on the max that the guest can map. */ down_write(¤t->mm->mmap_sem); - npages = (tbl->it_size << IOMMU_PAGE_SHIFT_4K) >> PAGE_SHIFT; + npages = (tbl->it_size << tbl->it_page_shift) >> PAGE_SHIFT; locked = current->mm->locked_vm + npages; lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; if (locked > lock_limit && !capable(CAP_IPC_LOCK)) { @@ -120,7 +120,7 @@ static void tce_iommu_disable(struct tce_container *container) down_write(¤t->mm->mmap_sem); current->mm->locked_vm -= (container->tbl->it_size << - IOMMU_PAGE_SHIFT_4K) >> PAGE_SHIFT; + container->tbl->it_page_shift) >> PAGE_SHIFT; up_write(¤t->mm->mmap_sem); } @@ -222,7 +222,7 @@ static long tce_iommu_build(struct tce_container *container, tce, ret); break; } - tce += IOMMU_PAGE_SIZE_4K; + tce += IOMMU_PAGE_SIZE(tbl); } if (ret) @@ -267,8 +267,8 @@ static long tce_iommu_ioctl(void *iommu_data, if (info.argsz < minsz) return -EINVAL; - info.dma32_window_start = tbl->it_offset << IOMMU_PAGE_SHIFT_4K; - info.dma32_window_size = tbl->it_size << IOMMU_PAGE_SHIFT_4K; + info.dma32_window_start = tbl->it_offset << tbl->it_page_shift; + info.dma32_window_size = tbl->it_size << tbl->it_page_shift; info.flags = 0; if (copy_to_user((void __user *)arg, &info, minsz)) @@ -298,8 +298,8 @@ static long tce_iommu_ioctl(void *iommu_data, VFIO_DMA_MAP_FLAG_WRITE)) return -EINVAL; - if ((param.size & ~IOMMU_PAGE_MASK_4K) || - (param.vaddr & ~IOMMU_PAGE_MASK_4K)) + if ((param.size & ~IOMMU_PAGE_MASK(tbl)) || + (param.vaddr & ~IOMMU_PAGE_MASK(tbl))) return -EINVAL; /* iova is checked by the IOMMU API */ @@ -314,8 +314,8 @@ static long tce_iommu_ioctl(void *iommu_data, return ret; ret = tce_iommu_build(container, tbl, - param.iova >> IOMMU_PAGE_SHIFT_4K, - tce, param.size >> IOMMU_PAGE_SHIFT_4K); + param.iova >> tbl->it_page_shift, + tce, param.size >> tbl->it_page_shift); iommu_flush_tce(tbl); @@ -341,17 +341,17 @@ static long tce_iommu_ioctl(void *iommu_data, if (param.flags) return -EINVAL; - if (param.size & ~IOMMU_PAGE_MASK_4K) + if (param.size & ~IOMMU_PAGE_MASK(tbl)) return -EINVAL; ret = iommu_tce_clear_param_check(tbl, param.iova, 0, - param.size >> IOMMU_PAGE_SHIFT_4K); + param.size >> tbl->it_page_shift); if (ret) return ret; ret = tce_iommu_clear(container, tbl, - param.iova >> IOMMU_PAGE_SHIFT_4K, - param.size >> IOMMU_PAGE_SHIFT_4K); + param.iova >> tbl->it_page_shift, + param.size >> tbl->it_page_shift); iommu_flush_tce(tbl); return ret;