* [PATCH] dma-mapping: benchmark: check the validity of dma mask bits @ 2020-12-12 10:18 Barry Song 2020-12-18 18:09 ` Robin Murphy 0 siblings, 1 reply; 4+ messages in thread From: Barry Song @ 2020-12-12 10:18 UTC (permalink / raw) To: hch, m.szyprowski, robin.murphy; +Cc: iommu, linuxarm, Dan Carpenter While dma_mask_bits is larger than 64, the bahvaiour is undefined. On the other hand, dma_mask_bits which is smaller than 20 (1MB) makes no sense in real hardware. Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Barry Song <song.bao.hua@hisilicon.com> --- kernel/dma/map_benchmark.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c index b1496e744c68..19f661692073 100644 --- a/kernel/dma/map_benchmark.c +++ b/kernel/dma/map_benchmark.c @@ -214,6 +214,12 @@ static long map_benchmark_ioctl(struct file *file, unsigned int cmd, return -EINVAL; } + if (map->bparam.dma_bits < 20 || + map->bparam.dma_bits > 64) { + pr_err("invalid dma_bits\n"); + return -EINVAL; + } + if (map->bparam.node != NUMA_NO_NODE && !node_possible(map->bparam.node)) { pr_err("invalid numa node\n"); -- 2.25.1 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] dma-mapping: benchmark: check the validity of dma mask bits 2020-12-12 10:18 [PATCH] dma-mapping: benchmark: check the validity of dma mask bits Barry Song @ 2020-12-18 18:09 ` Robin Murphy 2020-12-19 3:15 ` Song Bao Hua (Barry Song) 0 siblings, 1 reply; 4+ messages in thread From: Robin Murphy @ 2020-12-18 18:09 UTC (permalink / raw) To: Barry Song, hch, m.szyprowski; +Cc: iommu, linuxarm, Dan Carpenter On 2020-12-12 10:18, Barry Song wrote: > While dma_mask_bits is larger than 64, the bahvaiour is undefined. On the > other hand, dma_mask_bits which is smaller than 20 (1MB) makes no sense > in real hardware. > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Barry Song <song.bao.hua@hisilicon.com> > --- > kernel/dma/map_benchmark.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c > index b1496e744c68..19f661692073 100644 > --- a/kernel/dma/map_benchmark.c > +++ b/kernel/dma/map_benchmark.c > @@ -214,6 +214,12 @@ static long map_benchmark_ioctl(struct file *file, unsigned int cmd, > return -EINVAL; > } > > + if (map->bparam.dma_bits < 20 || FWIW I don't think we need to bother with a lower limit here - it's unsigned, and a pointlessly small value will fail gracefully when we come to actually set the mask anyway. We only need to protect kernel code from going wrong, not userspace from being stupid to its own detriment. Robin. > + map->bparam.dma_bits > 64) { > + pr_err("invalid dma_bits\n"); > + return -EINVAL; > + } > + > if (map->bparam.node != NUMA_NO_NODE && > !node_possible(map->bparam.node)) { > pr_err("invalid numa node\n"); > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] dma-mapping: benchmark: check the validity of dma mask bits 2020-12-18 18:09 ` Robin Murphy @ 2020-12-19 3:15 ` Song Bao Hua (Barry Song) 2020-12-21 13:25 ` Robin Murphy 0 siblings, 1 reply; 4+ messages in thread From: Song Bao Hua (Barry Song) @ 2020-12-19 3:15 UTC (permalink / raw) To: Robin Murphy, hch, m.szyprowski; +Cc: iommu, Linuxarm, Dan Carpenter > -----Original Message----- > From: Robin Murphy [mailto:robin.murphy@arm.com] > Sent: Saturday, December 19, 2020 7:10 AM > To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; hch@lst.de; > m.szyprowski@samsung.com > Cc: iommu@lists.linux-foundation.org; Linuxarm <linuxarm@huawei.com>; Dan > Carpenter <dan.carpenter@oracle.com> > Subject: Re: [PATCH] dma-mapping: benchmark: check the validity of dma mask > bits > > On 2020-12-12 10:18, Barry Song wrote: > > While dma_mask_bits is larger than 64, the bahvaiour is undefined. On the > > other hand, dma_mask_bits which is smaller than 20 (1MB) makes no sense > > in real hardware. > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Barry Song <song.bao.hua@hisilicon.com> > > --- > > kernel/dma/map_benchmark.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c > > index b1496e744c68..19f661692073 100644 > > --- a/kernel/dma/map_benchmark.c > > +++ b/kernel/dma/map_benchmark.c > > @@ -214,6 +214,12 @@ static long map_benchmark_ioctl(struct file *file, > unsigned int cmd, > > return -EINVAL; > > } > > > > + if (map->bparam.dma_bits < 20 || > > FWIW I don't think we need to bother with a lower limit here - it's > unsigned, and a pointlessly small value will fail gracefully when we > come to actually set the mask anyway. We only need to protect kernel > code from going wrong, not userspace from being stupid to its own detriment. I am not sure if kernel driver can reject small dma mask bit if drivers don't handle it properly. As a month ago, when I was debugging dma map benchmark, I set a value less than 32 to devices behind arm-smmu-v3, it could always succeed. But dma_map_single() was always failing. At that time, I didn't debug this issue. Not sure the latest status of iommu driver. drivers/iommu/intel/iommu.c used to have a dma_supported() to reject small dma_mask: static const struct dma_map_ops bounce_dma_ops = { ... .dma_supported = dma_direct_supported, }; > > Robin. > > > + map->bparam.dma_bits > 64) { > > + pr_err("invalid dma_bits\n"); > > + return -EINVAL; > > + } > > + > > if (map->bparam.node != NUMA_NO_NODE && > > !node_possible(map->bparam.node)) { > > pr_err("invalid numa node\n"); > > Thanks Barry _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] dma-mapping: benchmark: check the validity of dma mask bits 2020-12-19 3:15 ` Song Bao Hua (Barry Song) @ 2020-12-21 13:25 ` Robin Murphy 0 siblings, 0 replies; 4+ messages in thread From: Robin Murphy @ 2020-12-21 13:25 UTC (permalink / raw) To: Song Bao Hua (Barry Song), hch, m.szyprowski Cc: iommu, Linuxarm, Dan Carpenter On 2020-12-19 03:15, Song Bao Hua (Barry Song) wrote: > > >> -----Original Message----- >> From: Robin Murphy [mailto:robin.murphy@arm.com] >> Sent: Saturday, December 19, 2020 7:10 AM >> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; hch@lst.de; >> m.szyprowski@samsung.com >> Cc: iommu@lists.linux-foundation.org; Linuxarm <linuxarm@huawei.com>; Dan >> Carpenter <dan.carpenter@oracle.com> >> Subject: Re: [PATCH] dma-mapping: benchmark: check the validity of dma mask >> bits >> >> On 2020-12-12 10:18, Barry Song wrote: >>> While dma_mask_bits is larger than 64, the bahvaiour is undefined. On the >>> other hand, dma_mask_bits which is smaller than 20 (1MB) makes no sense >>> in real hardware. >>> >>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >>> Signed-off-by: Barry Song <song.bao.hua@hisilicon.com> >>> --- >>> kernel/dma/map_benchmark.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c >>> index b1496e744c68..19f661692073 100644 >>> --- a/kernel/dma/map_benchmark.c >>> +++ b/kernel/dma/map_benchmark.c >>> @@ -214,6 +214,12 @@ static long map_benchmark_ioctl(struct file *file, >> unsigned int cmd, >>> return -EINVAL; >>> } >>> >>> + if (map->bparam.dma_bits < 20 || >> >> FWIW I don't think we need to bother with a lower limit here - it's >> unsigned, and a pointlessly small value will fail gracefully when we >> come to actually set the mask anyway. We only need to protect kernel >> code from going wrong, not userspace from being stupid to its own detriment. > > I am not sure if kernel driver can reject small dma mask bit if drivers > don't handle it properly. > As a month ago, when I was debugging dma map benchmark, I set a value > less than 32 to devices behind arm-smmu-v3, it could always succeed. > But dma_map_single() was always failing. > At that time, I didn't debug this issue. Not sure the latest status of > iommu driver. FWIW, dma-direct should reject a mask if it doesn't cover at least the whole of ZONE_DMA; iommu-dma does allow anything, but that's because in principle it can make any mask down to PAGE_SIZE (or possibly even lower depending on the IOMMU) work. It's just that in that case the driver is liable to fill up the usable address space really really quickly :) (I suppose technically it should be checking that masks at least cover more than the reserved PFN at IOVA 0, but meh...) Either way, it still has little bearing on the benchmark itself. Say the user successfully sets an "acceptable" 21-bit DMA mask, but with 64K pages and >32 threads - the dma_map operations are still likely to start failing, and that failure is handled anyway, so why bother having an arbitrary and meaningless limit that only serves to make some unworkable cases fail slightly differently to others? Anyway, this doesn't really matter - I see the patch is in -next already - it's just one of those things I can't help calling out on principle :) Robin. _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-12-21 13:25 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-12 10:18 [PATCH] dma-mapping: benchmark: check the validity of dma mask bits Barry Song 2020-12-18 18:09 ` Robin Murphy 2020-12-19 3:15 ` Song Bao Hua (Barry Song) 2020-12-21 13:25 ` Robin Murphy
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).