[PATCH v8 5/5] mmc: queue: Use bigger segments if DMA MAP layer can merge the segments
When the max_segs of a mmc host is smaller than 512, the mmc
subsystem tries to use 512 segments if DMA MAP layer can merge
the segments, and then the mmc subsystem exposes such information
to the block layer by using blk_queue_can_use_dma_map_merging().
Signed-off-by: Yoshihiro Shimoda
Reviewed-by: Christoph Hellwig
Reviewed-by: Ulf Hansson
---
drivers/mmc/core/queue.c | 35 ---
include/linux/mmc/host.h | 1 +
2 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
index e327f80..ce337b2 100644
--- a/drivers/mmc/core/queue.c
+++ b/drivers/mmc/core/queue.c
@@ -20,6 +20,8 @@
#include "card.h"
#include "host.h"
+#define MMC_DMA_MAP_MERGE_SEGMENTS 512
+
static inline bool mmc_cqe_dcmd_busy(struct mmc_queue *mq)
{
/* Allow only 1 DCMD at a time */
@@ -192,6 +194,12 @@ static void mmc_queue_setup_discard(struct request_queue
*q,
blk_queue_flag_set(QUEUE_FLAG_SECERASE, q);
}
+static unsigned int mmc_get_max_segments(struct mmc_host *host)
+{
+ return host->can_dma_map_merge ? MMC_DMA_MAP_MERGE_SEGMENTS :
+host->max_segs;
+}
+
/**
* mmc_init_request() - initialize the MMC-specific per-request data
* @q: the request queue
@@ -205,7 +213,7 @@ static int __mmc_init_request(struct mmc_queue *mq, struct
request *req,
struct mmc_card *card = mq->card;
struct mmc_host *host = card->host;
- mq_rq->sg = mmc_alloc_sg(host->max_segs, gfp);
+ mq_rq->sg = mmc_alloc_sg(mmc_get_max_segments(host), gfp);
if (!mq_rq->sg)
return -ENOMEM;
@@ -361,13 +369,23 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct
mmc_card *card)
blk_queue_bounce_limit(mq->queue, BLK_BOUNCE_HIGH);
blk_queue_max_hw_sectors(mq->queue,
min(host->max_blk_count, host->max_req_size / 512));
- blk_queue_max_segments(mq->queue, host->max_segs);
+ if (host->can_dma_map_merge)
+ WARN(!blk_queue_can_use_dma_map_merging(mq->queue,
+ mmc_dev(host)),
+"merging was advertised but not possible");
+ blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
if (mmc_card_mmc(card))
block_size = card->ext_csd.data_sector_size;
blk_queue_logical_block_size(mq->queue, block_size);
- blk_queue_max_segment_size(mq->queue,
+ /*
+* After blk_queue_can_use_dma_map_merging() was called with succeed,
+* since it calls blk_queue_virt_boundary(), the mmc should not call
+* both blk_queue_max_segment_size().
+*/
+ if (host->can_dma_map_merge)
+ blk_queue_max_segment_size(mq->queue,
round_down(host->max_seg_size, block_size));
dma_set_max_seg_size(mmc_dev(host), queue_max_segment_size(mq->queue));
@@ -417,6 +435,17 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card
*card)
mq->tag_set.cmd_size = sizeof(struct mmc_queue_req);
mq->tag_set.driver_data = mq;
+ /*
+* Since blk_mq_alloc_tag_set() calls .init_request() of mmc_mq_ops,
+* the host->can_dma_map_merge should be set before to get max_segs
+* from mmc_get_max_segments().
+*/
+ if (host->max_segs < MMC_DMA_MAP_MERGE_SEGMENTS &&
+ dma_get_merge_boundary(mmc_dev(host)))
+ host->can_dma_map_merge = 1;
+ else
+ host->can_dma_map_merge = 0;
+
ret = blk_mq_alloc_tag_set(&mq->tag_set);
if (ret)
return ret;
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 4a351cb..c5662b3 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -396,6 +396,7 @@ struct mmc_host {
unsigned intretune_paused:1; /* re-tuning is temporarily
disabled */
unsigned intuse_blk_mq:1; /* use blk-mq */
unsigned intretune_crc_disable:1; /* don't trigger retune
upon crc */
+ unsigned intcan_dma_map_merge:1; /* merging can be used */
int rescan_disable; /* disable card detection */
int rescan_entered; /* used with nonremovable
devices */
--
2.7.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v8 3/5] block: sort headers on blk-setting.c
This patch sorts the headers in alphabetic order to ease the maintenance for this part. Signed-off-by: Yoshihiro Shimoda Reviewed-by: Wolfram Sang --- block/blk-settings.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/block/blk-settings.c b/block/blk-settings.c index 2ae348c..45f2c52 100644 --- a/block/blk-settings.c +++ b/block/blk-settings.c @@ -2,16 +2,16 @@ /* * Functions related to setting various queue properties from drivers */ -#include -#include -#include #include #include -#include /* for max_pfn/max_low_pfn */ #include -#include -#include #include +#include +#include +#include +#include +#include /* for max_pfn/max_low_pfn */ +#include #include "blk.h" #include "blk-wbt.h" -- 2.7.4 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v8 1/5] dma: Introduce dma_get_merge_boundary()
This patch adds a new DMA API "dma_get_merge_boundary". This function
returns the DMA merge boundary if the DMA layer can merge the segments.
This patch also adds the implementation for a new dma_map_ops pointer.
Signed-off-by: Yoshihiro Shimoda
Reviewed-by: Christoph Hellwig
---
Documentation/DMA-API.txt | 8
include/linux/dma-mapping.h | 6 ++
kernel/dma/mapping.c| 11 +++
3 files changed, 25 insertions(+)
diff --git a/Documentation/DMA-API.txt b/Documentation/DMA-API.txt
index e47c63b..9c4dd3d 100644
--- a/Documentation/DMA-API.txt
+++ b/Documentation/DMA-API.txt
@@ -204,6 +204,14 @@ Returns the maximum size of a mapping for the device. The
size parameter
of the mapping functions like dma_map_single(), dma_map_page() and
others should not be larger than the returned value.
+::
+
+ unsigned long
+ dma_get_merge_boundary(struct device *dev);
+
+Returns the DMA merge boundary. If the device cannot merge any the DMA address
+segments, the function returns 0.
+
Part Id - Streaming DMA mappings
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index e11b115..f700f8a 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -131,6 +131,7 @@ struct dma_map_ops {
int (*dma_supported)(struct device *dev, u64 mask);
u64 (*get_required_mask)(struct device *dev);
size_t (*max_mapping_size)(struct device *dev);
+ unsigned long (*get_merge_boundary)(struct device *dev);
};
#define DMA_MAPPING_ERROR (~(dma_addr_t)0)
@@ -467,6 +468,7 @@ int dma_set_mask(struct device *dev, u64 mask);
int dma_set_coherent_mask(struct device *dev, u64 mask);
u64 dma_get_required_mask(struct device *dev);
size_t dma_max_mapping_size(struct device *dev);
+unsigned long dma_get_merge_boundary(struct device *dev);
#else /* CONFIG_HAS_DMA */
static inline dma_addr_t dma_map_page_attrs(struct device *dev,
struct page *page, size_t offset, size_t size,
@@ -572,6 +574,10 @@ static inline size_t dma_max_mapping_size(struct device
*dev)
{
return 0;
}
+static inline unsigned long dma_get_merge_boundary(struct device *dev)
+{
+ return 0;
+}
#endif /* CONFIG_HAS_DMA */
static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
index 1f628e7..e8da02e 100644
--- a/kernel/dma/mapping.c
+++ b/kernel/dma/mapping.c
@@ -379,3 +379,14 @@ size_t dma_max_mapping_size(struct device *dev)
return size;
}
EXPORT_SYMBOL_GPL(dma_max_mapping_size);
+
+unsigned long dma_get_merge_boundary(struct device *dev)
+{
+ const struct dma_map_ops *ops = get_dma_ops(dev);
+
+ if (!ops || !ops->get_merge_boundary)
+ return 0; /* can't merge */
+
+ return ops->get_merge_boundary(dev);
+}
+EXPORT_SYMBOL_GPL(dma_get_merge_boundary);
--
2.7.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v8 4/5] block: add a helper function to merge the segments
This patch adds a helper function whether a queue can merge
the segments by the DMA MAP layer (e.g. via IOMMU).
Signed-off-by: Yoshihiro Shimoda
Reviewed-by: Christoph Hellwig
---
block/blk-settings.c | 22 ++
include/linux/blkdev.h | 2 ++
2 files changed, 24 insertions(+)
diff --git a/block/blk-settings.c b/block/blk-settings.c
index 45f2c52..6a78ea0 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -4,6 +4,7 @@
*/
#include
#include
+#include
#include
#include
#include
@@ -831,6 +832,27 @@ void blk_queue_write_cache(struct request_queue *q, bool
wc, bool fua)
}
EXPORT_SYMBOL_GPL(blk_queue_write_cache);
+/**
+ * blk_queue_can_use_dma_map_merging - configure queue for merging segments.
+ * @q: the request queue for the device
+ * @dev: the device pointer for dma
+ *
+ * Tell the block layer about merging the segments by dma map of @q.
+ */
+bool blk_queue_can_use_dma_map_merging(struct request_queue *q,
+ struct device *dev)
+{
+ unsigned long boundary = dma_get_merge_boundary(dev);
+
+ if (!boundary)
+ return false;
+
+ /* No need to update max_segment_size. see blk_queue_virt_boundary() */
+ blk_queue_virt_boundary(q, boundary);
+
+ return true;
+}
+
static int __init blk_settings_init(void)
{
blk_max_low_pfn = max_low_pfn - 1;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 1ef375d..f6d55e2 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1085,6 +1085,8 @@ extern void blk_queue_dma_alignment(struct request_queue
*, int);
extern void blk_queue_update_dma_alignment(struct request_queue *, int);
extern void blk_queue_rq_timeout(struct request_queue *, unsigned int);
extern void blk_queue_write_cache(struct request_queue *q, bool enabled, bool
fua);
+extern bool blk_queue_can_use_dma_map_merging(struct request_queue *q,
+ struct device *dev);
/*
* Number of physical segments as sent to the device.
--
2.7.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v8 0/5] treewide: improve R-Car SDHI performance
This patch series is based on linux-next.git / next-20190722 tag.
Since SDHI host internal DMAC of the R-Car Gen3 cannot handle two or
more segments, the performance rate (especially, eMMC HS400 reading)
is not good. However, if IOMMU is enabled on the DMAC, since IOMMU will
map multiple scatter gather buffers as one contignous iova, the DMAC can
handle the iova as well and then the performance rate is possible to
improve. In fact, I have measured the performance by using bonnie++,
"Sequential Input - block" rate was improved on r8a7795.
To achieve this, this patch series modifies IOMMU and Block subsystem
at first. This patch series is strictly depended on each subsystem
modification, so that I submit it as treewide.
Changes from v7:
- Rebase on next-20190722 (v5.3-rc1 + next branches of subsystems)
- Add some Reviewed-by.
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=135391
Changes from v6:
- [1/5 for DMA MAP] A new patch.
- [2/5 for IOMMU] A new patch.
- [3/5 for BLOCK] Add Reviewed-by.
- [4/5 for BLOCK] Use a new DMA MAP API instead of device_iommu_mapped().
- [5/5 for MMC] Likewise, and some minor fix.
- Remove patch 4/5 of v6 from this v7 patch series.
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=131769
Changes from v5:
- Almost all patches are new code.
- [4/5 for MMC] This is a refactor patch so that I don't add any
{Tested,Reviewed}-by tags.
- [5/5 for MMC] Modify MMC subsystem to use bigger segments instead of
the renesas_sdhi driver.
- [5/5 for MMC] Use BLK_MAX_SEGMENTS (128) instead of local value
SDHI_MAX_SEGS_IN_IOMMU (512). Even if we use BLK_MAX_SEGMENTS,
the performance is still good.
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=127511
Changes from v4:
- [DMA MAPPING] Add a new device_dma_parameters for iova contiguous.
- [IOMMU] Add a new capable for "merging" segments.
- [IOMMU] Add a capable ops into the ipmmu-vmsa driver.
- [MMC] Sort headers in renesas_sdhi_core.c.
- [MMC] Remove the following codes that made on v3 that can be achieved by
DMA MAPPING and IOMMU subsystem:
-- Check if R-Car Gen3 IPMMU is used or not on patch 3.
-- Check if all multiple segment buffers are aligned to PAGE_SIZE on patch 3.
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=125593
Changes from v3:
- Use a helper function device_iommu_mapped on patch 1 and 3.
- Check if R-Car Gen3 IPMMU is used or not on patch 3.
Yoshihiro Shimoda (5):
dma: Introduce dma_get_merge_boundary()
iommu/dma: Add a new dma_map_ops of get_merge_boundary()
block: sort headers on blk-setting.c
block: add a helper function to merge the segments
mmc: queue: Use bigger segments if DMA MAP layer can merge the
segments
Documentation/DMA-API.txt | 8
block/blk-settings.c| 34 --
drivers/iommu/dma-iommu.c | 11 +++
drivers/mmc/core/queue.c| 35 ---
include/linux/blkdev.h | 2 ++
include/linux/dma-mapping.h | 6 ++
include/linux/mmc/host.h| 1 +
kernel/dma/mapping.c| 11 +++
8 files changed, 99 insertions(+), 9 deletions(-)
--
2.7.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v8 2/5] iommu/dma: Add a new dma_map_ops of get_merge_boundary()
This patch adds a new dma_map_ops of get_merge_boundary() to
expose the DMA merge boundary if the domain type is IOMMU_DOMAIN_DMA.
Signed-off-by: Yoshihiro Shimoda
---
drivers/iommu/dma-iommu.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index a7f9c3e..f3e5f2b 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -1085,6 +1085,16 @@ static int iommu_dma_get_sgtable(struct device *dev,
struct sg_table *sgt,
return ret;
}
+static unsigned long iommu_dma_get_merge_boundary(struct device *dev)
+{
+ struct iommu_domain *domain = iommu_get_dma_domain(dev);
+
+ if (domain->type != IOMMU_DOMAIN_DMA)
+ return 0; /* can't merge */
+
+ return (1 << __ffs(domain->pgsize_bitmap)) - 1;
+}
+
static const struct dma_map_ops iommu_dma_ops = {
.alloc = iommu_dma_alloc,
.free = iommu_dma_free,
@@ -1100,6 +1110,7 @@ static const struct dma_map_ops iommu_dma_ops = {
.sync_sg_for_device = iommu_dma_sync_sg_for_device,
.map_resource = iommu_dma_map_resource,
.unmap_resource = iommu_dma_unmap_resource,
+ .get_merge_boundary = iommu_dma_get_merge_boundary,
};
/*
--
2.7.4
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 00/12] Clean up "mediatek,larb" after adding device_link
Hi, Yong:
I've added log in mtk_smi_clk_enable() and mtk_smi_clk_disable(), and I
boot MT8183 with display, the log is
[4.020340] mtk-smi-common 14019000.smi: mtk_smi_clk_enable()
[4.331371] mtk-smi-common 14019000.smi: mtk_smi_clk_disable()
[4.429578] mtk-smi-common 14019000.smi: mtk_smi_clk_enable()
[4.719743] mtk-smi-common 14019000.smi: mtk_smi_clk_disable()
[5.084770] mtk-smi-common 14019000.smi: mtk_smi_clk_enable()
[5.904310] mtk-smi-common 14019000.smi: mtk_smi_clk_disable()
>From the log, the clock is finally turned off, but the display works
normally. This is because scpsys has turn the clock on,
scpsys: syscon@10006000 {
compatible = "mediatek,mt8183-scpsys", "syscon";
#power-domain-cells = <1>;
reg = <0 0x10006000 0 0x1000>;
clocks = <&topckgen CLK_TOP_MUX_AUD_INTBUS>,
<&mmsys CLK_MM_SMI_COMMON>,
<&mmsys CLK_MM_GALS_COMM0>,
<&mmsys CLK_MM_GALS_COMM1>,
clock-names = "audio","mm-0",
"mm-1", "mm-2";
}
I'm worried that for MT8173, scpsys would not turn on subsys clock, this
series would let display work abnormally, so I think smi common should
not depend on scpsys to turn on the clock.
You could simply remove the clock parameter in scpsys device node, and
you would see the display works abnormally.
Regards,
CK
On Mon, 2019-06-10 at 20:55 +0800, Yong Wu wrote:
> MediaTek IOMMU block diagram always like below:
>
> M4U
> |
> smi-common
> |
> -
> | | ...
> | |
> larb1 larb2
> | |
> vdec venc
>
> All the consumer connect with smi-larb, then connect with smi-common.
>
> MediaTek IOMMU don't have its power-domain. When the consumer works,
> it should enable the smi-larb's power which also need enable the smi-common's
> power firstly.
>
> Thus, Firstly, use the device link connect the consumer and the
> smi-larbs. then add device link between the smi-larb and smi-common.
>
> After adding the device_link, then "mediatek,larb" property can be removed.
> the iommu consumer don't need call the mtk_smi_larb_get/put to enable
> the power and clock of smi-larb and smi-common.
>
> This patchset depends on "MT8183 IOMMU SUPPORT"[1].
>
> [1] https://lists.linuxfoundation.org/pipermail/iommu/2019-June/036552.html
>
> Change notes:
> v2:
>1) rebase on v5.2-rc1.
>2) Move adding device_link between the consumer and smi-larb into
> iommu_add_device from Robin.
>3) add DL_FLAG_AUTOREMOVE_CONSUMER even though the smi is built-in from
> Evan.
>4) Remove the shutdown callback in iommu.
>
> v1: https://lists.linuxfoundation.org/pipermail/iommu/2019-January/032387.html
>
> Yong Wu (12):
> dt-binding: mediatek: Get rid of mediatek,larb for multimedia HW
> iommu/mediatek: Add probe_defer for smi-larb
> iommu/mediatek: Add device_link between the consumer and the larb
> devices
> memory: mtk-smi: Add device-link between smi-larb and smi-common
> media: mtk-jpeg: Get rid of mtk_smi_larb_get/put
> media: mtk-mdp: Get rid of mtk_smi_larb_get/put
> media: mtk-vcodec: Get rid of mtk_smi_larb_get/put
> drm/mediatek: Get rid of mtk_smi_larb_get/put
> memory: mtk-smi: Get rid of mtk_smi_larb_get/put
> iommu/mediatek: Use builtin_platform_driver
> arm: dts: mediatek: Get rid of mediatek,larb for MM nodes
> arm64: dts: mediatek: Get rid of mediatek,larb for MM nodes
>
> .../bindings/display/mediatek/mediatek,disp.txt| 9 -
> .../bindings/media/mediatek-jpeg-decoder.txt | 4 --
> .../devicetree/bindings/media/mediatek-mdp.txt | 8
> .../devicetree/bindings/media/mediatek-vcodec.txt | 4 --
> arch/arm/boot/dts/mt2701.dtsi | 1 -
> arch/arm/boot/dts/mt7623.dtsi | 1 -
> arch/arm64/boot/dts/mediatek/mt8173.dtsi | 15 ---
> drivers/gpu/drm/mediatek/mtk_drm_crtc.c| 11 -
> drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c| 26
> drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h| 1 -
> drivers/iommu/mtk_iommu.c | 45 +++--
> drivers/iommu/mtk_iommu_v1.c | 39 +++---
> drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c| 22 --
> drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h| 2 -
> drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 38 -
> drivers/media/platform/mtk-mdp/mtk_mdp_comp.h | 2 -
> drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 1 -
> .../media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c | 21 --
> drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h | 3 --
> drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 1 -
> .../media/plat
Re: [PATCH] powerpc/dma: Fix invalid DMA mmap behavior
Shawn Anastasio writes: > On 7/22/19 7:16 AM, Michael Ellerman wrote: >> Arnd Bergmann writes: >>> On Thu, Jul 18, 2019 at 11:52 AM Christoph Hellwig wrote: On Thu, Jul 18, 2019 at 10:49:34AM +0200, Christoph Hellwig wrote: > On Thu, Jul 18, 2019 at 01:45:16PM +1000, Oliver O'Halloran wrote: >>> Other than m68k, mips, and arm64, everybody else that doesn't have >>> ARCH_NO_COHERENT_DMA_MMAP set uses this default implementation, so >>> I assume this behavior is acceptable on those architectures. >> >> It might be acceptable, but there's no reason to use pgport_noncached >> if the platform supports cache-coherent DMA. >> >> Christoph (+cc) made the change so maybe he saw something we're missing. > > I always found the forcing of noncached access even for coherent > devices a little odd, but this was inherited from the previous > implementation, which surprised me a bit as the different attributes > are usually problematic even on x86. Let me dig into the history a > bit more, but I suspect the righ fix is to default to cached mappings > for coherent devices. Ok, some history: The generic dma mmap implementation, which we are effectively still using today was added by: commit 64ccc9c033c6089b2d426dad3c56477ab066c999 Author: Marek Szyprowski Date: Thu Jun 14 13:03:04 2012 +0200 common: dma-mapping: add support for generic dma_mmap_* calls and unconditionally uses pgprot_noncached in dma_common_mmap, which is then used as the fallback by dma_mmap_attrs if no ->mmap method is present. At that point we already had the powerpc implementation that only uses pgprot_noncached for non-coherent mappings, and the arm one, which uses pgprot_writecombine if DMA_ATTR_WRITE_COMBINE is set and otherwise pgprot_dmacoherent, which seems to be uncached. Arm did support coherent platforms at that time, but they might have been an afterthought and not handled properly. >>> >>> Cache-coherent devices are still very rare on 32-bit ARM. >>> >>> Among the callers of dma_mmap_coherent(), almost all are in platform >>> specific device drivers that only ever run on noncoherent ARM SoCs, >>> which explains why nobody would have noticed problems. >>> >>> There is also a difference in behavior between ARM and PowerPC >>> when dealing with mismatched cacheability attributes: If the same >>> page is mapped as both cached and uncached to, this may >>> cause silent undefined behavior on ARM, while PowerPC should >>> enter a checkstop as soon as it notices. >> >> On newer Power CPUs it's actually more like the ARM behaviour. >> >> I don't know for sure that it will *never* checkstop but there are at >> least cases where it won't. There's some (not much) detail in the >> Power8/9 user manuals. > > The issue was discovered due to sporadic checkstops on P9, so it > seems like it will happen at least sometimes. Yeah true. I wasn't sure if that checkstop was actually caused by a cached/uncached mismatch or something else, but looks like it was, from the hostboot issue (https://github.com/open-power/hostboot/issues/180): 12.47015| Signature Description: pu.ex:k0:n0:s0:p00:c0 (L2FIR[16]) Cache line inhibited hit cacheable space So I'm not really sure how to square that with the documentation in the user manual: If a caching-inhibited load instruction hits in the L1 data cache, the load data is serviced from the L1 data cache and no request is sent to the NCU. If a caching-inhibited store instruction hits in the L1 data cache, the store data is written to the L1 data cache and sent to the NCU. Note that the L1 data cache and L2 cache are no longer coherent. I guess I'm either misinterpreting that section or there's some *other* documentation somewhere I haven't found that says that it will also checkstop. cheers ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] powerpc/dma: Fix invalid DMA mmap behavior
On 7/22/19 7:16 AM, Michael Ellerman wrote: Arnd Bergmann writes: On Thu, Jul 18, 2019 at 11:52 AM Christoph Hellwig wrote: On Thu, Jul 18, 2019 at 10:49:34AM +0200, Christoph Hellwig wrote: On Thu, Jul 18, 2019 at 01:45:16PM +1000, Oliver O'Halloran wrote: Other than m68k, mips, and arm64, everybody else that doesn't have ARCH_NO_COHERENT_DMA_MMAP set uses this default implementation, so I assume this behavior is acceptable on those architectures. It might be acceptable, but there's no reason to use pgport_noncached if the platform supports cache-coherent DMA. Christoph (+cc) made the change so maybe he saw something we're missing. I always found the forcing of noncached access even for coherent devices a little odd, but this was inherited from the previous implementation, which surprised me a bit as the different attributes are usually problematic even on x86. Let me dig into the history a bit more, but I suspect the righ fix is to default to cached mappings for coherent devices. Ok, some history: The generic dma mmap implementation, which we are effectively still using today was added by: commit 64ccc9c033c6089b2d426dad3c56477ab066c999 Author: Marek Szyprowski Date: Thu Jun 14 13:03:04 2012 +0200 common: dma-mapping: add support for generic dma_mmap_* calls and unconditionally uses pgprot_noncached in dma_common_mmap, which is then used as the fallback by dma_mmap_attrs if no ->mmap method is present. At that point we already had the powerpc implementation that only uses pgprot_noncached for non-coherent mappings, and the arm one, which uses pgprot_writecombine if DMA_ATTR_WRITE_COMBINE is set and otherwise pgprot_dmacoherent, which seems to be uncached. Arm did support coherent platforms at that time, but they might have been an afterthought and not handled properly. Cache-coherent devices are still very rare on 32-bit ARM. Among the callers of dma_mmap_coherent(), almost all are in platform specific device drivers that only ever run on noncoherent ARM SoCs, which explains why nobody would have noticed problems. There is also a difference in behavior between ARM and PowerPC when dealing with mismatched cacheability attributes: If the same page is mapped as both cached and uncached to, this may cause silent undefined behavior on ARM, while PowerPC should enter a checkstop as soon as it notices. On newer Power CPUs it's actually more like the ARM behaviour. I don't know for sure that it will *never* checkstop but there are at least cases where it won't. There's some (not much) detail in the Power8/9 user manuals. The issue was discovered due to sporadic checkstops on P9, so it seems like it will happen at least sometimes. cheers ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] dma-mapping: Use dma_get_mask in dma_addressing_limited
On Mon, Jul 22, 2019 at 06:56:49PM +0200, Auger Eric wrote:
> Hi Christoph,
>
> On 7/22/19 6:51 PM, Eric Auger wrote:
> > We currently have cases where the dma_addressing_limited() gets
> > called with dma_mask unset. This causes a NULL pointer dereference.
> >
> > Use dma_get_mask() accessor to prevent the crash.
> >
> > Fixes: b866455423e0 ("dma-mapping: add a dma_addressing_limited helper")
> > Signed-off-by: Eric Auger
>
> As a follow-up of my last email, here is a patch featuring
> dma_get_mask(). But you don't have the WARN_ON_ONCE anymore, pointing
> out suspect users.
OTOH these users then simply become okay so no need for WARN_ON_ONCE
then :)
> Feel free to pick up your preferred approach
>
> Thanks
>
> Eric
> >
> > ---
> >
> > v1 -> v2:
> > - was [PATCH 1/2] dma-mapping: Protect dma_addressing_limited
> > against NULL dma_mask
> > - Use dma_get_mask
> > ---
> > include/linux/dma-mapping.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > index e11b115dd0e4..f7d1eea32c78 100644
> > --- a/include/linux/dma-mapping.h
> > +++ b/include/linux/dma-mapping.h
> > @@ -689,8 +689,8 @@ static inline int dma_coerce_mask_and_coherent(struct
> > device *dev, u64 mask)
> > */
> > static inline bool dma_addressing_limited(struct device *dev)
> > {
> > - return min_not_zero(*dev->dma_mask, dev->bus_dma_mask) <
> > - dma_get_required_mask(dev);
> > + return min_not_zero(dma_get_mask(dev), dev->bus_dma_mask) <
> > + dma_get_required_mask(dev);
> > }
> >
> > #ifdef CONFIG_ARCH_HAS_SETUP_DMA_OPS
> >
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] dma-mapping: Use dma_get_mask in dma_addressing_limited
On Mon, Jul 22, 2019 at 06:51:49PM +0200, Eric Auger wrote:
> We currently have cases where the dma_addressing_limited() gets
> called with dma_mask unset. This causes a NULL pointer dereference.
>
> Use dma_get_mask() accessor to prevent the crash.
>
> Fixes: b866455423e0 ("dma-mapping: add a dma_addressing_limited helper")
> Signed-off-by: Eric Auger
Acked-by: Michael S. Tsirkin
If possible I really prefer this approach: avoids changing all callers
and uses documented interfaces.
> ---
>
> v1 -> v2:
> - was [PATCH 1/2] dma-mapping: Protect dma_addressing_limited
> against NULL dma_mask
> - Use dma_get_mask
> ---
> include/linux/dma-mapping.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index e11b115dd0e4..f7d1eea32c78 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -689,8 +689,8 @@ static inline int dma_coerce_mask_and_coherent(struct
> device *dev, u64 mask)
> */
> static inline bool dma_addressing_limited(struct device *dev)
> {
> - return min_not_zero(*dev->dma_mask, dev->bus_dma_mask) <
> - dma_get_required_mask(dev);
> + return min_not_zero(dma_get_mask(dev), dev->bus_dma_mask) <
> + dma_get_required_mask(dev);
> }
>
> #ifdef CONFIG_ARCH_HAS_SETUP_DMA_OPS
> --
> 2.20.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] dma-mapping: Use dma_get_mask in dma_addressing_limited
Hi Christoph,
On 7/22/19 6:51 PM, Eric Auger wrote:
> We currently have cases where the dma_addressing_limited() gets
> called with dma_mask unset. This causes a NULL pointer dereference.
>
> Use dma_get_mask() accessor to prevent the crash.
>
> Fixes: b866455423e0 ("dma-mapping: add a dma_addressing_limited helper")
> Signed-off-by: Eric Auger
As a follow-up of my last email, here is a patch featuring
dma_get_mask(). But you don't have the WARN_ON_ONCE anymore, pointing
out suspect users.
Feel free to pick up your preferred approach
Thanks
Eric
>
> ---
>
> v1 -> v2:
> - was [PATCH 1/2] dma-mapping: Protect dma_addressing_limited
> against NULL dma_mask
> - Use dma_get_mask
> ---
> include/linux/dma-mapping.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index e11b115dd0e4..f7d1eea32c78 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -689,8 +689,8 @@ static inline int dma_coerce_mask_and_coherent(struct
> device *dev, u64 mask)
> */
> static inline bool dma_addressing_limited(struct device *dev)
> {
> - return min_not_zero(*dev->dma_mask, dev->bus_dma_mask) <
> - dma_get_required_mask(dev);
> + return min_not_zero(dma_get_mask(dev), dev->bus_dma_mask) <
> + dma_get_required_mask(dev);
> }
>
> #ifdef CONFIG_ARCH_HAS_SETUP_DMA_OPS
>
[PATCH v2] dma-mapping: Use dma_get_mask in dma_addressing_limited
We currently have cases where the dma_addressing_limited() gets
called with dma_mask unset. This causes a NULL pointer dereference.
Use dma_get_mask() accessor to prevent the crash.
Fixes: b866455423e0 ("dma-mapping: add a dma_addressing_limited helper")
Signed-off-by: Eric Auger
---
v1 -> v2:
- was [PATCH 1/2] dma-mapping: Protect dma_addressing_limited
against NULL dma_mask
- Use dma_get_mask
---
include/linux/dma-mapping.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index e11b115dd0e4..f7d1eea32c78 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -689,8 +689,8 @@ static inline int dma_coerce_mask_and_coherent(struct
device *dev, u64 mask)
*/
static inline bool dma_addressing_limited(struct device *dev)
{
- return min_not_zero(*dev->dma_mask, dev->bus_dma_mask) <
- dma_get_required_mask(dev);
+ return min_not_zero(dma_get_mask(dev), dev->bus_dma_mask) <
+ dma_get_required_mask(dev);
}
#ifdef CONFIG_ARCH_HAS_SETUP_DMA_OPS
--
2.20.1
Re: [PATCH v2] iommu: add support for drivers that manage iommu explicitly
On Mon, Jul 22, 2019 at 8:48 AM Joerg Roedel wrote: > > On Mon, Jul 22, 2019 at 08:41:34AM -0700, Rob Clark wrote: > > It is set by the driver: > > > > https://patchwork.freedesktop.org/patch/315291/ > > > > (This doesn't really belong in devicetree, since it isn't a > > description of the hardware, so the driver is really the only place to > > set this.. which is fine because it is about a detail of how the > > driver works.) > > It is more a detail about how the firmware works. IIUC the problem is > that the firmware initializes the context mappings for the GPU and the > OS doesn't know anything about that and just overwrites them, causing > the firmware GPU driver to fail badly. > > So I think it is the task of the firmware to tell the OS not to touch > the devices mappings until the OS device driver takes over. On x86 there > is something similar with the RMRR/unity-map tables from the firmware. > Bjorn had a patchset[1] to inherit the config from firmware/bootloader when arm-smmu is probed which handles that part of the problem. My patch is intended to be used on top of his patchset. This seems to me like the best solution, if we don't have control over the firmware. BR, -R [1] https://www.spinics.net/lists/arm-kernel/msg732246.html
Re: [PATCH] iommu/vt-d: Print pasid table entries MSB to LSB in debugfs
On Sun, Jul 21, 2019 at 05:22:07PM -0700, Sai Praneeth Prakhya wrote: > --- > drivers/iommu/intel-iommu-debugfs.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Added a Fixes-tag and applied to iommu/fixes. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/iova: Remove stale cached32_node
On Sat, Jul 20, 2019 at 07:08:48PM +0100, Chris Wilson wrote: > --- > drivers/iommu/iova.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) Applied to iommu/fixes. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu: add support for drivers that manage iommu explicitly
On Mon, Jul 22, 2019 at 08:41:34AM -0700, Rob Clark wrote: > It is set by the driver: > > https://patchwork.freedesktop.org/patch/315291/ > > (This doesn't really belong in devicetree, since it isn't a > description of the hardware, so the driver is really the only place to > set this.. which is fine because it is about a detail of how the > driver works.) It is more a detail about how the firmware works. IIUC the problem is that the firmware initializes the context mappings for the GPU and the OS doesn't know anything about that and just overwrites them, causing the firmware GPU driver to fail badly. So I think it is the task of the firmware to tell the OS not to touch the devices mappings until the OS device driver takes over. On x86 there is something similar with the RMRR/unity-map tables from the firmware. Regards, Joerg
Re: [PATCH 1/2] dma-mapping: Protect dma_addressing_limited against NULL dma_mask
Hi Christoph,
On 7/22/19 5:26 PM, Christoph Hellwig wrote:
>> static inline bool dma_addressing_limited(struct device *dev)
>> {
>> -return min_not_zero(*dev->dma_mask, dev->bus_dma_mask) <
>> -dma_get_required_mask(dev);
>> +return WARN_ON_ONCE(!dev->dma_mask) ? false :
>> +min_not_zero(*dev->dma_mask, dev->bus_dma_mask) <
>> +dma_get_required_mask(dev);
>
> This should really use a separate if statement, but I can fix that
> up when applying it.
>
Just wondering why we don't use the dma_get_mask() accessor which
returns DMA_BIT_MASK(32) in case the dma_mask is not set.
Do you foresee any issue and would it still mandate to add dma_mask
checks on each call sites?
Thanks
Eric
Re: [PATCH 2/2] virtio/virtio_ring: Fix the dma_max_mapping_size call
On Mon, Jul 22, 2019 at 04:36:09PM +0100, Robin Murphy wrote:
> On 22/07/2019 15:55, Eric Auger wrote:
> > Do not call dma_max_mapping_size for devices that have no DMA
> > mask set, otherwise we can hit a NULL pointer dereference.
> >
> > This occurs when a virtio-blk-pci device is protected with
> > a virtual IOMMU.
> >
> > Fixes: e6d6dd6c875e ("virtio: Introduce virtio_max_dma_size()")
> > Signed-off-by: Eric Auger
> > Suggested-by: Christoph Hellwig
> > ---
> > drivers/virtio/virtio_ring.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index c8be1c4f5b55..37c143971211 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -262,7 +262,7 @@ size_t virtio_max_dma_size(struct virtio_device *vdev)
> > {
> > size_t max_segment_size = SIZE_MAX;
> > - if (vring_use_dma_api(vdev))
> > + if (vring_use_dma_api(vdev) && vdev->dev.dma_mask)
>
> Hmm, might it make sense to roll that check up into vring_use_dma_api()
> itself? After all, if the device has no mask then it's likely that other DMA
> API ops wouldn't really work as expected either.
>
> Robin.
Nope, Eric pointed out it's just dma_addressing_limited that is broken.
Other APIs call dma_get_mask which handles the NULL mask case fine.
> > max_segment_size = dma_max_mapping_size(&vdev->dev);
> > return max_segment_size;
> >
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] iommu/vt-d: Don't queue_iova() if there is no flush queue
On Tue, Jul 16, 2019 at 10:38:05PM +0100, Dmitry Safonov wrote: > BUG: spinlock bad magic on CPU#6, swapper/0/1 Applied both patches to iommu/fixes.
Re: [PATCH v2] iommu: add support for drivers that manage iommu explicitly
On Mon, Jul 22, 2019 at 7:28 AM Joerg Roedel wrote:
>
> On Wed, Jul 10, 2019 at 11:28:30AM -0700, Rob Clark wrote:
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -282,7 +282,8 @@ struct device_driver {
> > struct module *owner;
> > const char *mod_name; /* used for built-in modules
> > */
> >
> > - bool suppress_bind_attrs; /* disables bind/unbind via sysfs */
> > + bool suppress_bind_attrs:1; /* disables bind/unbind via sysfs */
> > + bool driver_manages_iommu:1;/* driver manages IOMMU explicitly */
>
> Who will set this bit?
>
It is set by the driver:
https://patchwork.freedesktop.org/patch/315291/
(This doesn't really belong in devicetree, since it isn't a
description of the hardware, so the driver is really the only place to
set this.. which is fine because it is about a detail of how the
driver works.)
BR,
-R
Re: [PATCH 2/2] virtio/virtio_ring: Fix the dma_max_mapping_size call
On Mon, Jul 22, 2019 at 05:27:10PM +0200, Christoph Hellwig wrote:
> On Mon, Jul 22, 2019 at 04:55:09PM +0200, Eric Auger wrote:
> > Do not call dma_max_mapping_size for devices that have no DMA
> > mask set, otherwise we can hit a NULL pointer dereference.
> >
> > This occurs when a virtio-blk-pci device is protected with
> > a virtual IOMMU.
> >
> > Fixes: e6d6dd6c875e ("virtio: Introduce virtio_max_dma_size()")
> > Signed-off-by: Eric Auger
> > Suggested-by: Christoph Hellwig
>
> Looks good. virtio maintainers, let me know if you want to queue
> it up or if I should pick the patch up through the dma-mapping tree.
Personally I dislike this API because I feel presence of dma mask does
not strictly have to reflect max size. And generally the requirement to
check presence of mask feels like an undocumented hack to me. Even
reading code will not help you avoid the warning, everyone will get it
wrong and get the warning splat in their logs. So I would prefer just
v1 of the patch that makes dma API do the right thing for us.
However, if that's not going to be the case, let's fix it up in virtio.
In any case, for both v1 and v2 of the patches, you can merge them
through your tree:
Acked-by: Michael S. Tsirkin
--
MST
Re: [PATCH 2/2] virtio/virtio_ring: Fix the dma_max_mapping_size call
On 22/07/2019 15:55, Eric Auger wrote:
Do not call dma_max_mapping_size for devices that have no DMA
mask set, otherwise we can hit a NULL pointer dereference.
This occurs when a virtio-blk-pci device is protected with
a virtual IOMMU.
Fixes: e6d6dd6c875e ("virtio: Introduce virtio_max_dma_size()")
Signed-off-by: Eric Auger
Suggested-by: Christoph Hellwig
---
drivers/virtio/virtio_ring.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index c8be1c4f5b55..37c143971211 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -262,7 +262,7 @@ size_t virtio_max_dma_size(struct virtio_device *vdev)
{
size_t max_segment_size = SIZE_MAX;
- if (vring_use_dma_api(vdev))
+ if (vring_use_dma_api(vdev) && vdev->dev.dma_mask)
Hmm, might it make sense to roll that check up into vring_use_dma_api()
itself? After all, if the device has no mask then it's likely that other
DMA API ops wouldn't really work as expected either.
Robin.
max_segment_size = dma_max_mapping_size(&vdev->dev);
return max_segment_size;
Re: [PATCH 2/2] virtio/virtio_ring: Fix the dma_max_mapping_size call
On Mon, Jul 22, 2019 at 04:55:09PM +0200, Eric Auger wrote:
> Do not call dma_max_mapping_size for devices that have no DMA
> mask set, otherwise we can hit a NULL pointer dereference.
>
> This occurs when a virtio-blk-pci device is protected with
> a virtual IOMMU.
>
> Fixes: e6d6dd6c875e ("virtio: Introduce virtio_max_dma_size()")
> Signed-off-by: Eric Auger
> Suggested-by: Christoph Hellwig
Christoph, I wonder why did you suggest this?
The connection between dma_mask and dma_max_mapping_size
is far from obvious. The documentation doesn't exist.
Do we really have to teach all users about this hack?
Why not just make dma_max_mapping_size DTRT?
> ---
> drivers/virtio/virtio_ring.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index c8be1c4f5b55..37c143971211 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -262,7 +262,7 @@ size_t virtio_max_dma_size(struct virtio_device *vdev)
> {
> size_t max_segment_size = SIZE_MAX;
>
> - if (vring_use_dma_api(vdev))
> + if (vring_use_dma_api(vdev) && vdev->dev.dma_mask)
> max_segment_size = dma_max_mapping_size(&vdev->dev);
>
> return max_segment_size;
> --
> 2.20.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/2] virtio/virtio_ring: Fix the dma_max_mapping_size call
On Mon, Jul 22, 2019 at 04:55:09PM +0200, Eric Auger wrote:
> Do not call dma_max_mapping_size for devices that have no DMA
> mask set, otherwise we can hit a NULL pointer dereference.
>
> This occurs when a virtio-blk-pci device is protected with
> a virtual IOMMU.
>
> Fixes: e6d6dd6c875e ("virtio: Introduce virtio_max_dma_size()")
> Signed-off-by: Eric Auger
> Suggested-by: Christoph Hellwig
Looks good. virtio maintainers, let me know if you want to queue
it up or if I should pick the patch up through the dma-mapping tree.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] dma-mapping: Protect dma_addressing_limited against NULL dma_mask
> static inline bool dma_addressing_limited(struct device *dev)
> {
> - return min_not_zero(*dev->dma_mask, dev->bus_dma_mask) <
> - dma_get_required_mask(dev);
> + return WARN_ON_ONCE(!dev->dma_mask) ? false :
> + min_not_zero(*dev->dma_mask, dev->bus_dma_mask) <
> + dma_get_required_mask(dev);
This should really use a separate if statement, but I can fix that
up when applying it.
Re: [PATCH] iommu/virtio: Update to most recent specification
On Mon, Jul 22, 2019 at 11:16:08AM -0400, Michael S. Tsirkin wrote: > I'm merging this for this release, unless someone objects. No objections from me. Acked-by: Joerg Roedel ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/virtio: Update to most recent specification
On Mon, Jul 22, 2019 at 03:40:07PM +0100, Jean-Philippe Brucker wrote:
> Following specification review a few things were changed in v8 of the
> virtio-iommu series [1], but have been omitted when merging the base
> driver. Add them now:
>
> * Remove the EXEC flag.
> * Add feature bit for the MMIO flag.
> * Change domain_bits to domain_range.
> * Add NOMEM status flag.
>
> [1]
> https://lore.kernel.org/linux-iommu/20190530170929.19366-1-jean-philippe.bruc...@arm.com/
>
> Fixes: edcd69ab9a32 ("iommu: Add virtio-iommu driver")
> Reported-by: Eric Auger
> Signed-off-by: Jean-Philippe Brucker
I'm merging this for this release, unless someone objects.
> ---
> drivers/iommu/virtio-iommu.c | 40 ++-
> include/uapi/linux/virtio_iommu.h | 32 ++---
> 2 files changed, 47 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 433f4d2ee956..80a740df0737 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -2,7 +2,7 @@
> /*
> * Virtio driver for the paravirtualized IOMMU
> *
> - * Copyright (C) 2018 Arm Limited
> + * Copyright (C) 2019 Arm Limited
> */
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -47,7 +47,10 @@ struct viommu_dev {
> /* Device configuration */
> struct iommu_domain_geometrygeometry;
> u64 pgsize_bitmap;
> - u8 domain_bits;
> + u32 first_domain;
> + u32 last_domain;
> + /* Supported MAP flags */
> + u32 map_flags;
> u32 probe_size;
> };
>
> @@ -62,6 +65,7 @@ struct viommu_domain {
> struct viommu_dev *viommu;
> struct mutexmutex; /* protects viommu pointer */
> unsigned intid;
> + u32 map_flags;
>
> spinlock_t mappings_lock;
> struct rb_root_cached mappings;
> @@ -113,6 +117,8 @@ static int viommu_get_req_errno(void *buf, size_t len)
> return -ENOENT;
> case VIRTIO_IOMMU_S_FAULT:
> return -EFAULT;
> + case VIRTIO_IOMMU_S_NOMEM:
> + return -ENOMEM;
> case VIRTIO_IOMMU_S_IOERR:
> case VIRTIO_IOMMU_S_DEVERR:
> default:
> @@ -607,15 +613,15 @@ static int viommu_domain_finalise(struct viommu_dev
> *viommu,
> {
> int ret;
> struct viommu_domain *vdomain = to_viommu_domain(domain);
> - unsigned int max_domain = viommu->domain_bits > 31 ? ~0 :
> - (1U << viommu->domain_bits) - 1;
>
> vdomain->viommu = viommu;
> + vdomain->map_flags = viommu->map_flags;
>
> domain->pgsize_bitmap = viommu->pgsize_bitmap;
> domain->geometry= viommu->geometry;
>
> - ret = ida_alloc_max(&viommu->domain_ids, max_domain, GFP_KERNEL);
> + ret = ida_alloc_range(&viommu->domain_ids, viommu->first_domain,
> + viommu->last_domain, GFP_KERNEL);
> if (ret >= 0)
> vdomain->id = (unsigned int)ret;
>
> @@ -710,7 +716,7 @@ static int viommu_map(struct iommu_domain *domain,
> unsigned long iova,
> phys_addr_t paddr, size_t size, int prot)
> {
> int ret;
> - int flags;
> + u32 flags;
> struct virtio_iommu_req_map map;
> struct viommu_domain *vdomain = to_viommu_domain(domain);
>
> @@ -718,6 +724,9 @@ static int viommu_map(struct iommu_domain *domain,
> unsigned long iova,
> (prot & IOMMU_WRITE ? VIRTIO_IOMMU_MAP_F_WRITE : 0) |
> (prot & IOMMU_MMIO ? VIRTIO_IOMMU_MAP_F_MMIO : 0);
>
> + if (flags & ~vdomain->map_flags)
> + return -EINVAL;
> +
> ret = viommu_add_mapping(vdomain, iova, paddr, size, flags);
> if (ret)
> return ret;
> @@ -1027,7 +1036,8 @@ static int viommu_probe(struct virtio_device *vdev)
> goto err_free_vqs;
> }
>
> - viommu->domain_bits = 32;
> + viommu->map_flags = VIRTIO_IOMMU_MAP_F_READ | VIRTIO_IOMMU_MAP_F_WRITE;
> + viommu->last_domain = ~0U;
>
> /* Optional features */
> virtio_cread_feature(vdev, VIRTIO_IOMMU_F_INPUT_RANGE,
> @@ -1038,9 +1048,13 @@ static int viommu_probe(struct virtio_device *vdev)
>struct virtio_iommu_config, input_range.end,
>&input_end);
>
> - virtio_cread_feature(vdev, VIRTIO_IOMMU_F_DOMAIN_BITS,
> - struct virtio_iommu_config, domain_bits,
> - &viommu->domain_bits);
> + virtio_cread_feature(vdev, VIRTIO_IOMMU_F_DOMAIN_RANGE,
> + struct virtio_iommu_config, domain_range.start,
> + &viommu->first_domain);
> +
> + virti
Re: [PATCH RFC 1/4] iommu/vt-d: Modify device_def_domain_type() to use at runtime
On Tue, Jul 02, 2019 at 06:53:59PM -0700, Sai Praneeth Prakhya wrote: > device_def_domain_type() determines the domain type a device could have and > it's called only during boot. But, to change the domain of a group through > sysfs, kernel has to call this function during runtime. Hence, add an > argument to the function which lets the function know if it's being called > at boot time or runtime. I don't think it should make a difference when the function is actually called. The sysfs input is just another variable to take into account when the default domain type is determined. What I'd like to see for example is that I can write 'auto' to the file and get back the systems decision for the default domain type. I'd also like to be able to forbid changing the type for e.g. Thunderbolt connected devices. Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/virtio: Update to most recent specification
Hi Jean,
On 7/22/19 4:40 PM, Jean-Philippe Brucker wrote:
> Following specification review a few things were changed in v8 of the
> virtio-iommu series [1], but have been omitted when merging the base
> driver. Add them now:
>
> * Remove the EXEC flag.
> * Add feature bit for the MMIO flag.
> * Change domain_bits to domain_range.
> * Add NOMEM status flag.
>
> [1]
> https://lore.kernel.org/linux-iommu/20190530170929.19366-1-jean-philippe.bruc...@arm.com/
>
> Fixes: edcd69ab9a32 ("iommu: Add virtio-iommu driver")
> Reported-by: Eric Auger
> Signed-off-by: Jean-Philippe Brucker
Reviewed-by: Eric Auger
Tested-by: Eric Auger
Thanks
Eric
> ---
> drivers/iommu/virtio-iommu.c | 40 ++-
> include/uapi/linux/virtio_iommu.h | 32 ++---
> 2 files changed, 47 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 433f4d2ee956..80a740df0737 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -2,7 +2,7 @@
> /*
> * Virtio driver for the paravirtualized IOMMU
> *
> - * Copyright (C) 2018 Arm Limited
> + * Copyright (C) 2019 Arm Limited
> */
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -47,7 +47,10 @@ struct viommu_dev {
> /* Device configuration */
> struct iommu_domain_geometrygeometry;
> u64 pgsize_bitmap;
> - u8 domain_bits;
> + u32 first_domain;
> + u32 last_domain;
> + /* Supported MAP flags */
> + u32 map_flags;
> u32 probe_size;
> };
>
> @@ -62,6 +65,7 @@ struct viommu_domain {
> struct viommu_dev *viommu;
> struct mutexmutex; /* protects viommu pointer */
> unsigned intid;
> + u32 map_flags;
>
> spinlock_t mappings_lock;
> struct rb_root_cached mappings;
> @@ -113,6 +117,8 @@ static int viommu_get_req_errno(void *buf, size_t len)
> return -ENOENT;
> case VIRTIO_IOMMU_S_FAULT:
> return -EFAULT;
> + case VIRTIO_IOMMU_S_NOMEM:
> + return -ENOMEM;
> case VIRTIO_IOMMU_S_IOERR:
> case VIRTIO_IOMMU_S_DEVERR:
> default:
> @@ -607,15 +613,15 @@ static int viommu_domain_finalise(struct viommu_dev
> *viommu,
> {
> int ret;
> struct viommu_domain *vdomain = to_viommu_domain(domain);
> - unsigned int max_domain = viommu->domain_bits > 31 ? ~0 :
> - (1U << viommu->domain_bits) - 1;
>
> vdomain->viommu = viommu;
> + vdomain->map_flags = viommu->map_flags;
>
> domain->pgsize_bitmap = viommu->pgsize_bitmap;
> domain->geometry= viommu->geometry;
>
> - ret = ida_alloc_max(&viommu->domain_ids, max_domain, GFP_KERNEL);
> + ret = ida_alloc_range(&viommu->domain_ids, viommu->first_domain,
> + viommu->last_domain, GFP_KERNEL);
> if (ret >= 0)
> vdomain->id = (unsigned int)ret;
>
> @@ -710,7 +716,7 @@ static int viommu_map(struct iommu_domain *domain,
> unsigned long iova,
> phys_addr_t paddr, size_t size, int prot)
> {
> int ret;
> - int flags;
> + u32 flags;
> struct virtio_iommu_req_map map;
> struct viommu_domain *vdomain = to_viommu_domain(domain);
>
> @@ -718,6 +724,9 @@ static int viommu_map(struct iommu_domain *domain,
> unsigned long iova,
> (prot & IOMMU_WRITE ? VIRTIO_IOMMU_MAP_F_WRITE : 0) |
> (prot & IOMMU_MMIO ? VIRTIO_IOMMU_MAP_F_MMIO : 0);
>
> + if (flags & ~vdomain->map_flags)
> + return -EINVAL;
> +
> ret = viommu_add_mapping(vdomain, iova, paddr, size, flags);
> if (ret)
> return ret;
> @@ -1027,7 +1036,8 @@ static int viommu_probe(struct virtio_device *vdev)
> goto err_free_vqs;
> }
>
> - viommu->domain_bits = 32;
> + viommu->map_flags = VIRTIO_IOMMU_MAP_F_READ | VIRTIO_IOMMU_MAP_F_WRITE;
> + viommu->last_domain = ~0U;
>
> /* Optional features */
> virtio_cread_feature(vdev, VIRTIO_IOMMU_F_INPUT_RANGE,
> @@ -1038,9 +1048,13 @@ static int viommu_probe(struct virtio_device *vdev)
>struct virtio_iommu_config, input_range.end,
>&input_end);
>
> - virtio_cread_feature(vdev, VIRTIO_IOMMU_F_DOMAIN_BITS,
> - struct virtio_iommu_config, domain_bits,
> - &viommu->domain_bits);
> + virtio_cread_feature(vdev, VIRTIO_IOMMU_F_DOMAIN_RANGE,
> + struct virtio_iommu_config, domain_range.start,
> + &viommu->first_domain);
> +
> + virtio_cread_fe
[PATCH 1/2] dma-mapping: Protect dma_addressing_limited against NULL dma_mask
dma_addressing_limited() should not be called on a device with
a NULL dma_mask. If this occurs let's WARN_ON_ONCE and immediately
return. Existing call sites are updated separately.
Fixes: b866455423e0 ("dma-mapping: add a dma_addressing_limited helper")
Signed-off-by: Eric Auger
---
v1 -> v2:
- add a WARN_ON_ONCE()
- reword the commit message
---
include/linux/dma-mapping.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index e11b115dd0e4..ef0cf9537abc 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -689,8 +689,9 @@ static inline int dma_coerce_mask_and_coherent(struct
device *dev, u64 mask)
*/
static inline bool dma_addressing_limited(struct device *dev)
{
- return min_not_zero(*dev->dma_mask, dev->bus_dma_mask) <
- dma_get_required_mask(dev);
+ return WARN_ON_ONCE(!dev->dma_mask) ? false :
+ min_not_zero(*dev->dma_mask, dev->bus_dma_mask) <
+ dma_get_required_mask(dev);
}
#ifdef CONFIG_ARCH_HAS_SETUP_DMA_OPS
--
2.20.1
[PATCH 2/2] virtio/virtio_ring: Fix the dma_max_mapping_size call
Do not call dma_max_mapping_size for devices that have no DMA
mask set, otherwise we can hit a NULL pointer dereference.
This occurs when a virtio-blk-pci device is protected with
a virtual IOMMU.
Fixes: e6d6dd6c875e ("virtio: Introduce virtio_max_dma_size()")
Signed-off-by: Eric Auger
Suggested-by: Christoph Hellwig
---
drivers/virtio/virtio_ring.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index c8be1c4f5b55..37c143971211 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -262,7 +262,7 @@ size_t virtio_max_dma_size(struct virtio_device *vdev)
{
size_t max_segment_size = SIZE_MAX;
- if (vring_use_dma_api(vdev))
+ if (vring_use_dma_api(vdev) && vdev->dev.dma_mask)
max_segment_size = dma_max_mapping_size(&vdev->dev);
return max_segment_size;
--
2.20.1
[PATCH 0/2] Fix NULL pointer dereference with virtio-blk-pci and virtual IOMMU
When running a guest featuring a virtio-blk-pci protected with a virtual IOMMU we hit a NULL pointer dereference. This series removes the dma_max_mapping_size() call in virtio_max_dma_size when the device does not have any dma_mask set. A check is also added to early return in dma_addressing_limited() if the dma_mask is NULL. Eric Auger (2): dma-mapping: Protect dma_addressing_limited against NULL dma_mask virtio/virtio_ring: Fix the dma_max_mapping_size call drivers/virtio/virtio_ring.c | 2 +- include/linux/dma-mapping.h | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) -- 2.20.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC 0/4] iommu: Add support to change default domain of a group
On Tue, Jul 02, 2019 at 06:53:58PM -0700, Sai Praneeth Prakhya wrote: > Assume a use case where-in the priviliged user would want to use the device in > pass-through mode when the device is used for host but would want to switch to > dma protected mode when switching for VFIO in user space. Presently, this is > not > supported and hence add support to change default domain of a group > dynamically. VFIO does it's own iommu magic with the device and moves the group out of the default domain, so that doesn't sound like a valid use-case. More valid would be something like putting a device into passthrough mode to improve performance, or do you have other valid use-cases in mind? Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/virtio: Update to most recent specification
Following specification review a few things were changed in v8 of the
virtio-iommu series [1], but have been omitted when merging the base
driver. Add them now:
* Remove the EXEC flag.
* Add feature bit for the MMIO flag.
* Change domain_bits to domain_range.
* Add NOMEM status flag.
[1]
https://lore.kernel.org/linux-iommu/20190530170929.19366-1-jean-philippe.bruc...@arm.com/
Fixes: edcd69ab9a32 ("iommu: Add virtio-iommu driver")
Reported-by: Eric Auger
Signed-off-by: Jean-Philippe Brucker
---
drivers/iommu/virtio-iommu.c | 40 ++-
include/uapi/linux/virtio_iommu.h | 32 ++---
2 files changed, 47 insertions(+), 25 deletions(-)
diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 433f4d2ee956..80a740df0737 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -2,7 +2,7 @@
/*
* Virtio driver for the paravirtualized IOMMU
*
- * Copyright (C) 2018 Arm Limited
+ * Copyright (C) 2019 Arm Limited
*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -47,7 +47,10 @@ struct viommu_dev {
/* Device configuration */
struct iommu_domain_geometrygeometry;
u64 pgsize_bitmap;
- u8 domain_bits;
+ u32 first_domain;
+ u32 last_domain;
+ /* Supported MAP flags */
+ u32 map_flags;
u32 probe_size;
};
@@ -62,6 +65,7 @@ struct viommu_domain {
struct viommu_dev *viommu;
struct mutexmutex; /* protects viommu pointer */
unsigned intid;
+ u32 map_flags;
spinlock_t mappings_lock;
struct rb_root_cached mappings;
@@ -113,6 +117,8 @@ static int viommu_get_req_errno(void *buf, size_t len)
return -ENOENT;
case VIRTIO_IOMMU_S_FAULT:
return -EFAULT;
+ case VIRTIO_IOMMU_S_NOMEM:
+ return -ENOMEM;
case VIRTIO_IOMMU_S_IOERR:
case VIRTIO_IOMMU_S_DEVERR:
default:
@@ -607,15 +613,15 @@ static int viommu_domain_finalise(struct viommu_dev
*viommu,
{
int ret;
struct viommu_domain *vdomain = to_viommu_domain(domain);
- unsigned int max_domain = viommu->domain_bits > 31 ? ~0 :
- (1U << viommu->domain_bits) - 1;
vdomain->viommu = viommu;
+ vdomain->map_flags = viommu->map_flags;
domain->pgsize_bitmap = viommu->pgsize_bitmap;
domain->geometry= viommu->geometry;
- ret = ida_alloc_max(&viommu->domain_ids, max_domain, GFP_KERNEL);
+ ret = ida_alloc_range(&viommu->domain_ids, viommu->first_domain,
+ viommu->last_domain, GFP_KERNEL);
if (ret >= 0)
vdomain->id = (unsigned int)ret;
@@ -710,7 +716,7 @@ static int viommu_map(struct iommu_domain *domain, unsigned
long iova,
phys_addr_t paddr, size_t size, int prot)
{
int ret;
- int flags;
+ u32 flags;
struct virtio_iommu_req_map map;
struct viommu_domain *vdomain = to_viommu_domain(domain);
@@ -718,6 +724,9 @@ static int viommu_map(struct iommu_domain *domain, unsigned
long iova,
(prot & IOMMU_WRITE ? VIRTIO_IOMMU_MAP_F_WRITE : 0) |
(prot & IOMMU_MMIO ? VIRTIO_IOMMU_MAP_F_MMIO : 0);
+ if (flags & ~vdomain->map_flags)
+ return -EINVAL;
+
ret = viommu_add_mapping(vdomain, iova, paddr, size, flags);
if (ret)
return ret;
@@ -1027,7 +1036,8 @@ static int viommu_probe(struct virtio_device *vdev)
goto err_free_vqs;
}
- viommu->domain_bits = 32;
+ viommu->map_flags = VIRTIO_IOMMU_MAP_F_READ | VIRTIO_IOMMU_MAP_F_WRITE;
+ viommu->last_domain = ~0U;
/* Optional features */
virtio_cread_feature(vdev, VIRTIO_IOMMU_F_INPUT_RANGE,
@@ -1038,9 +1048,13 @@ static int viommu_probe(struct virtio_device *vdev)
struct virtio_iommu_config, input_range.end,
&input_end);
- virtio_cread_feature(vdev, VIRTIO_IOMMU_F_DOMAIN_BITS,
-struct virtio_iommu_config, domain_bits,
-&viommu->domain_bits);
+ virtio_cread_feature(vdev, VIRTIO_IOMMU_F_DOMAIN_RANGE,
+struct virtio_iommu_config, domain_range.start,
+&viommu->first_domain);
+
+ virtio_cread_feature(vdev, VIRTIO_IOMMU_F_DOMAIN_RANGE,
+struct virtio_iommu_config, domain_range.end,
+&viommu->last_domain);
virtio_cread_feature(vdev, VIRTIO_IOMMU_F_PROBE,
Re: [PATCH v2] iommu: add support for drivers that manage iommu explicitly
On Wed, Jul 10, 2019 at 11:28:30AM -0700, Rob Clark wrote:
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -282,7 +282,8 @@ struct device_driver {
> struct module *owner;
> const char *mod_name; /* used for built-in modules */
>
> - bool suppress_bind_attrs; /* disables bind/unbind via sysfs */
> + bool suppress_bind_attrs:1; /* disables bind/unbind via sysfs */
> + bool driver_manages_iommu:1;/* driver manages IOMMU explicitly */
Who will set this bit?
Regards,
Joerg
Re: [PATCH v2 7/7] iommu/vt-d: Consolidate domain_init() to avoid duplication
On Sat, Jul 20, 2019 at 09:15:58AM +0800, Lu Baolu wrote: > Okay. I agree wit you. Let's revert this commit first. Reverted the patch and queued it to my iommu/fixes branch. Regards, Joerg
[PATCH] MAINTAINERS: Update my email address
Update MAINTAINERS and .mailmap with my @linaro.org address, since I don't have access to my @arm.com address anymore. Signed-off-by: Jean-Philippe Brucker --- .mailmap| 1 + MAINTAINERS | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.mailmap b/.mailmap index 0fef932de3db..8ce554b9c9f1 100644 --- a/.mailmap +++ b/.mailmap @@ -98,6 +98,7 @@ Jason Gunthorpe Javi Merino Jean Tourrilhes + Jeff Garzik Jeff Layton Jeff Layton diff --git a/MAINTAINERS b/MAINTAINERS index 783569e3c4b4..bded78c84701 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17123,7 +17123,7 @@ F: drivers/virtio/virtio_input.c F: include/uapi/linux/virtio_input.h VIRTIO IOMMU DRIVER -M: Jean-Philippe Brucker +M: Jean-Philippe Brucker L: virtualizat...@lists.linux-foundation.org S: Maintained F: drivers/iommu/virtio-iommu.c -- 2.22.0 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] MAINTAINERS: Update my email address
On Mon, Jul 22, 2019 at 02:44:40PM +0100, Jean-Philippe Brucker wrote: > Update MAINTAINERS and .mailmap with my @linaro.org address, since I > don't have access to my @arm.com address anymore. > > Signed-off-by: Jean-Philippe Brucker > --- > .mailmap| 1 + > MAINTAINERS | 2 +- > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/.mailmap b/.mailmap > index 0fef932de3db..8ce554b9c9f1 100644 > --- a/.mailmap > +++ b/.mailmap > @@ -98,6 +98,7 @@ Jason Gunthorpe > > Javi Merino > > Jean Tourrilhes > + > Jeff Garzik > Jeff Layton > Jeff Layton > diff --git a/MAINTAINERS b/MAINTAINERS > index 783569e3c4b4..bded78c84701 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -17123,7 +17123,7 @@ F:drivers/virtio/virtio_input.c > F: include/uapi/linux/virtio_input.h > > VIRTIO IOMMU DRIVER > -M: Jean-Philippe Brucker > +M: Jean-Philippe Brucker > L: virtualizat...@lists.linux-foundation.org > S: Maintained > F: drivers/iommu/virtio-iommu.c Thanks (and your new address is easier to remember ;). I can take this one via arm64, since I already have a bunch of MAINTAINERS updates queued for -rc2. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/amd: fix a crash in iova_magazine_free_pfns
Hi Linus, On Sun, Jul 21, 2019 at 09:58:04AM -0700, Linus Torvalds wrote: > This one seems to have fallen through the cracks. > > Applied directly. Thanks! > Maybe it's hiding in some fixes tree that I haven't gotten a pull > request for yet? No, I havn't had it applied anywhere yet. I usually don't pay close attention to patches sent to me during the merge window, as I won't update my tree anyway before -rc1. I only take care of important fixes, but this one seems to have fallen through my heuristic. Thanks for applying it directly. Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [RFC v1 4/4] vfio/type1: bind guest pasid (guest page tables) to host
> From: Auger Eric [mailto:eric.au...@redhat.com]
> Sent: Thursday, July 18, 2019 6:17 PM
> To: Liu, Yi L ; alex.william...@redhat.com
> Subject: Re: [RFC v1 4/4] vfio/type1: bind guest pasid (guest page tables) to
> host
>
> Yi,
>
> On 7/5/19 1:06 PM, Liu, Yi L wrote:
> > From: Liu Yi L
> >
> > This patch adds vfio support to bind guest translation structure to
> > host iommu. VFIO exposes iommu programming capability to user- space.
> > Guest is a user-space application in host under KVM solution.
> > For SVA usage in Virtual Machine, guest owns GVA->GPA translation
> > structure. And this part should be passdown to host to enable nested
> > translation (or say two stage translation). This patch reuses the
> > VFIO_IOMMU_BIND proposal from Jean-Philippe Brucker, and adds new bind
> > type for binding guest owned translation structure to host.
> >
> > *) Add two new ioctls for VFIO containers.
> >
> > - VFIO_IOMMU_BIND: for bind request from userspace, it could be
> >bind a process to a pasid or bind a guest pasid
> >to a device, this is indicated by type
> > - VFIO_IOMMU_UNBIND: for unbind request from userspace, it could be
> >unbind a process to a pasid or unbind a guest pasid
> >to a device, also indicated by type
> > - Bind type:
> > VFIO_IOMMU_BIND_PROCESS: user-space request to bind a process
> >to a device
> > VFIO_IOMMU_BIND_GUEST_PASID: bind guest owned translation
> >structure to host iommu. e.g. guest page table
> You may add that only VFIO_IOMMU_BIND_GUEST_PASID gets implemented in this
> patch
Good catch:-).
> >
> > *) Code logic in vfio_iommu_type1_ioctl() to handle
> > VFIO_IOMMU_BIND/UNBIND
> >
> > Cc: Kevin Tian
> > Signed-off-by: Jean-Philippe Brucker
> > Signed-off-by: Liu Yi L
> > Signed-off-by: Jacob Pan
> > ---
> > drivers/vfio/vfio_iommu_type1.c | 151
>
> > include/uapi/linux/vfio.h | 56 +++
> > 2 files changed, 207 insertions(+)
> >
> > diff --git a/drivers/vfio/vfio_iommu_type1.c
> > b/drivers/vfio/vfio_iommu_type1.c index d5e0c01..57826ed 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -1920,6 +1920,119 @@ static int vfio_iommu_type1_pasid_free(struct
> vfio_iommu *iommu, int pasid)
> > return ret;
> > }
> >
> > +static int vfio_bind_gpasid_fn(struct device *dev, void *data) {
> > + struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> > + struct vfio_iommu_type1_bind_guest_pasid *guest_bind = data;
> > +
> > + return iommu_sva_bind_gpasid(domain, dev, &guest_bind->bind_data); }
> > +
> > +static int vfio_unbind_gpasid_fn(struct device *dev, void *data) {
> > + struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> > + struct vfio_iommu_type1_bind_guest_pasid *guest_bind = data;
> > +
> > + return iommu_sva_unbind_gpasid(domain, dev,
> > + guest_bind->bind_data.hpasid);
> > +}
> > +
> > +/*
> > + * unbind specific gpasid, caller of this function requires hold
> > + * vfio_iommu->lock
> > + */
> > +static long vfio_iommu_type1_do_guest_unbind(struct vfio_iommu *iommu,
> > + struct vfio_iommu_type1_bind_guest_pasid *guest_bind) {
> > + struct vfio_domain *domain;
> > + struct vfio_group *group;
> > + int ret = 0;
> > +
> > + if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu)) {
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + list_for_each_entry(domain, &iommu->domain_list, next) {
> > + list_for_each_entry(group, &domain->group_list, next) {
> > + ret = iommu_group_for_each_dev(group->iommu_group,
> > + guest_bind, vfio_unbind_gpasid_fn);
> can it fail individually, in which case we end up with something half unset
> or it is safe?
> A comment may be worth.
thanks, good suggestion. Actually, we have an assumption that for devices which
belong to non-singleton group, we should not enable PASID capability. So may not
fail individually. But yes, a comment would be needed here.
> > + if (ret)
> > + goto out;
> > + }
> > + }
> you may use vfio_iommu_lookup_dev() introduced in [RFC v1 2/4] vfio:
> VFIO_IOMMU_CACHE_INVALIDATE
yes, let me do it next version. :-)
> > +
> > + return 0;
> not needed
> > +
> > +out:
> > + return ret;
> > +}
> > +
> > +static long vfio_iommu_type1_bind_gpasid(struct vfio_iommu *iommu,
> > + void __user *arg,
> > + struct vfio_iommu_type1_bind *bind)
> > {
> > + struct vfio_iommu_type1_bind_guest_pasid guest_bind;
> > + struct vfio_domain *domain;
> > + struct vfio_group *group;
> > + unsigned long minsz;
> > + int ret = 0;
> > +
> > + minsz = sizeof(*bind) + sizeof(guest_bind);
> > + if (bind->
Re: [PATCH] powerpc/dma: Fix invalid DMA mmap behavior
Arnd Bergmann writes: > On Thu, Jul 18, 2019 at 11:52 AM Christoph Hellwig wrote: >> On Thu, Jul 18, 2019 at 10:49:34AM +0200, Christoph Hellwig wrote: >> > On Thu, Jul 18, 2019 at 01:45:16PM +1000, Oliver O'Halloran wrote: >> > > > Other than m68k, mips, and arm64, everybody else that doesn't have >> > > > ARCH_NO_COHERENT_DMA_MMAP set uses this default implementation, so >> > > > I assume this behavior is acceptable on those architectures. >> > > >> > > It might be acceptable, but there's no reason to use pgport_noncached >> > > if the platform supports cache-coherent DMA. >> > > >> > > Christoph (+cc) made the change so maybe he saw something we're missing. >> > >> > I always found the forcing of noncached access even for coherent >> > devices a little odd, but this was inherited from the previous >> > implementation, which surprised me a bit as the different attributes >> > are usually problematic even on x86. Let me dig into the history a >> > bit more, but I suspect the righ fix is to default to cached mappings >> > for coherent devices. >> >> Ok, some history: >> >> The generic dma mmap implementation, which we are effectively still >> using today was added by: >> >> commit 64ccc9c033c6089b2d426dad3c56477ab066c999 >> Author: Marek Szyprowski >> Date: Thu Jun 14 13:03:04 2012 +0200 >> >> common: dma-mapping: add support for generic dma_mmap_* calls >> >> and unconditionally uses pgprot_noncached in dma_common_mmap, which is >> then used as the fallback by dma_mmap_attrs if no ->mmap method is >> present. At that point we already had the powerpc implementation >> that only uses pgprot_noncached for non-coherent mappings, and >> the arm one, which uses pgprot_writecombine if DMA_ATTR_WRITE_COMBINE >> is set and otherwise pgprot_dmacoherent, which seems to be uncached. >> Arm did support coherent platforms at that time, but they might have >> been an afterthought and not handled properly. > > Cache-coherent devices are still very rare on 32-bit ARM. > > Among the callers of dma_mmap_coherent(), almost all are in platform > specific device drivers that only ever run on noncoherent ARM SoCs, > which explains why nobody would have noticed problems. > > There is also a difference in behavior between ARM and PowerPC > when dealing with mismatched cacheability attributes: If the same > page is mapped as both cached and uncached to, this may > cause silent undefined behavior on ARM, while PowerPC should > enter a checkstop as soon as it notices. On newer Power CPUs it's actually more like the ARM behaviour. I don't know for sure that it will *never* checkstop but there are at least cases where it won't. There's some (not much) detail in the Power8/9 user manuals. cheers ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] dma-mapping: Protect dma_addressing_limited against NULL dma_mask
I posted a fix that takes care of this in SCSI this morning:
https://marc.info/?l=linux-scsi&m=156378725427719&w=2
I suspect for virtio-blk we should do the same.
> static inline bool dma_addressing_limited(struct device *dev)
> {
> - return min_not_zero(*dev->dma_mask, dev->bus_dma_mask) <
> - dma_get_required_mask(dev);
> + return dev->dma_mask ? min_not_zero(*dev->dma_mask, dev->bus_dma_mask) <
> + dma_get_required_mask(dev) : false;
But to be on the safe side we could still do an early return here,
but it should have a WARN_ON_ONCE.
[PATCH] dma-mapping: Protect dma_addressing_limited against NULL dma_mask
There are cases when the helper gets called when the dma_mask is NULL.
This is observed when running virtio-block-pci devices protected by
a virtual IOMMU. Guest crashes with NULL pointer dereference.
Fixes: b866455423e0 ("dma-mapping: add a dma_addressing_limited helper")
Signed-off-by: Eric Auger
---
include/linux/dma-mapping.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index e11b115dd0e4..5e7f386fe0d2 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -689,8 +689,8 @@ static inline int dma_coerce_mask_and_coherent(struct
device *dev, u64 mask)
*/
static inline bool dma_addressing_limited(struct device *dev)
{
- return min_not_zero(*dev->dma_mask, dev->bus_dma_mask) <
- dma_get_required_mask(dev);
+ return dev->dma_mask ? min_not_zero(*dev->dma_mask, dev->bus_dma_mask) <
+ dma_get_required_mask(dev) : false;
}
#ifdef CONFIG_ARCH_HAS_SETUP_DMA_OPS
--
2.20.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/iova: Remove stale cached32_node
Quoting Robin Murphy (2019-07-22 11:13:49) > Hi Chris, > > On 20/07/2019 19:08, Chris Wilson wrote: > > Since the cached32_node is allowed to be advanced above dma_32bit_pfn > > (to provide a shortcut into the limited range), we need to be careful to > > remove the to be freed node if it is the cached32_node. > > Thanks for digging in - just to confirm my understanding, the > problematic situation is where both 32-bit and 64-bit nodes have been > allocated, the topmost 32-bit node is freed, then the lowest 64-bit node > is freed, which leaves the pointer dangling because the second free > fails the "free->pfn_hi < iovad->dma_32bit_pfn" check. Does that match > your reasoning? Yes. Once cached32_node is set to the right of dma_32bit_pfn it fails to be picked up in the next cycle through __cached_rbnode_delete_update should cached32_node be the next victim. > Assuming I haven't completely misread the problem, I can't think of a > more appropriate way to fix it, so; > > Reviewed-by: Robin Murphy > > I could swear I did consider that corner case at some point, but since > Leizhen and I rewrote this stuff about 5 times between us I'm not > entirely surprised such a subtlety could have got lost again along the way. I admit to being impressed that rb_prev() does not appear high in the profiles -- though I guess that is more an artifact of the scary layers of caching above it. -Chris ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/iova: Remove stale cached32_node
Hi Chris, On 20/07/2019 19:08, Chris Wilson wrote: Since the cached32_node is allowed to be advanced above dma_32bit_pfn (to provide a shortcut into the limited range), we need to be careful to remove the to be freed node if it is the cached32_node. Thanks for digging in - just to confirm my understanding, the problematic situation is where both 32-bit and 64-bit nodes have been allocated, the topmost 32-bit node is freed, then the lowest 64-bit node is freed, which leaves the pointer dangling because the second free fails the "free->pfn_hi < iovad->dma_32bit_pfn" check. Does that match your reasoning? Assuming I haven't completely misread the problem, I can't think of a more appropriate way to fix it, so; Reviewed-by: Robin Murphy I could swear I did consider that corner case at some point, but since Leizhen and I rewrote this stuff about 5 times between us I'm not entirely surprised such a subtlety could have got lost again along the way. Thanks, Robin. [ 48.43] BUG: KASAN: use-after-free in __cached_rbnode_delete_update+0x68/0x110 [ 48.477812] Read of size 8 at addr 88870fc19020 by task kworker/u8:1/37 [ 48.477843] [ 48.477879] CPU: 1 PID: 37 Comm: kworker/u8:1 Tainted: G U 5.2.0+ #735 [ 48.477915] Hardware name: Intel Corporation NUC7i5BNK/NUC7i5BNB, BIOS BNKBL357.86A.0052.2017.0918.1346 09/18/2017 [ 48.478047] Workqueue: i915 __i915_gem_free_work [i915] [ 48.478075] Call Trace: [ 48.478111] dump_stack+0x5b/0x90 [ 48.478137] print_address_description+0x67/0x237 [ 48.478178] ? __cached_rbnode_delete_update+0x68/0x110 [ 48.478212] __kasan_report.cold.3+0x1c/0x38 [ 48.478240] ? __cached_rbnode_delete_update+0x68/0x110 [ 48.478280] ? __cached_rbnode_delete_update+0x68/0x110 [ 48.478308] __cached_rbnode_delete_update+0x68/0x110 [ 48.478344] private_free_iova+0x2b/0x60 [ 48.478378] iova_magazine_free_pfns+0x46/0xa0 [ 48.478403] free_iova_fast+0x277/0x340 [ 48.478443] fq_ring_free+0x15a/0x1a0 [ 48.478473] queue_iova+0x19c/0x1f0 [ 48.478597] cleanup_page_dma.isra.64+0x62/0xb0 [i915] [ 48.478712] __gen8_ppgtt_cleanup+0x63/0x80 [i915] [ 48.478826] __gen8_ppgtt_cleanup+0x42/0x80 [i915] [ 48.478940] __gen8_ppgtt_clear+0x433/0x4b0 [i915] [ 48.479053] __gen8_ppgtt_clear+0x462/0x4b0 [i915] [ 48.479081] ? __sg_free_table+0x9e/0xf0 [ 48.479116] ? kfree+0x7f/0x150 [ 48.479234] i915_vma_unbind+0x1e2/0x240 [i915] [ 48.479352] i915_vma_destroy+0x3a/0x280 [i915] [ 48.479465] __i915_gem_free_objects+0xf0/0x2d0 [i915] [ 48.479579] __i915_gem_free_work+0x41/0xa0 [i915] [ 48.479607] process_one_work+0x495/0x710 [ 48.479642] worker_thread+0x4c7/0x6f0 [ 48.479687] ? process_one_work+0x710/0x710 [ 48.479724] kthread+0x1b2/0x1d0 [ 48.479774] ? kthread_create_worker_on_cpu+0xa0/0xa0 [ 48.479820] ret_from_fork+0x1f/0x30 [ 48.479864] [ 48.479907] Allocated by task 631: [ 48.479944] save_stack+0x19/0x80 [ 48.479994] __kasan_kmalloc.constprop.6+0xc1/0xd0 [ 48.480038] kmem_cache_alloc+0x91/0xf0 [ 48.480082] alloc_iova+0x2b/0x1e0 [ 48.480125] alloc_iova_fast+0x58/0x376 [ 48.480166] intel_alloc_iova+0x90/0xc0 [ 48.480214] intel_map_sg+0xde/0x1f0 [ 48.480343] i915_gem_gtt_prepare_pages+0xb8/0x170 [i915] [ 48.480465] huge_get_pages+0x232/0x2b0 [i915] [ 48.480590] i915_gem_object_get_pages+0x40/0xb0 [i915] [ 48.480712] __i915_gem_object_get_pages+0x90/0xa0 [i915] [ 48.480834] i915_gem_object_prepare_write+0x2d6/0x330 [i915] [ 48.480955] create_test_object.isra.54+0x1a9/0x3e0 [i915] [ 48.481075] igt_shared_ctx_exec+0x365/0x3c0 [i915] [ 48.481210] __i915_subtests.cold.4+0x30/0x92 [i915] [ 48.481341] __run_selftests.cold.3+0xa9/0x119 [i915] [ 48.481466] i915_live_selftests+0x3c/0x70 [i915] [ 48.481583] i915_pci_probe+0xe7/0x220 [i915] [ 48.481620] pci_device_probe+0xe0/0x180 [ 48.481665] really_probe+0x163/0x4e0 [ 48.481710] device_driver_attach+0x85/0x90 [ 48.481750] __driver_attach+0xa5/0x180 [ 48.481796] bus_for_each_dev+0xda/0x130 [ 48.481831] bus_add_driver+0x205/0x2e0 [ 48.481882] driver_register+0xca/0x140 [ 48.481927] do_one_initcall+0x6c/0x1af [ 48.481970] do_init_module+0x106/0x350 [ 48.482010] load_module+0x3d2c/0x3ea0 [ 48.482058] __do_sys_finit_module+0x110/0x180 [ 48.482102] do_syscall_64+0x62/0x1f0 [ 48.482147] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 48.482190] [ 48.482224] Freed by task 37: [ 48.482273] save_stack+0x19/0x80 [ 48.482318] __kasan_slab_free+0x12e/0x180 [ 48.482363] kmem_cache_free+0x70/0x140 [ 48.482406] __free_iova+0x1d/0x30 [ 48.482445] fq_ring_free+0x15a/0x1a0 [ 48.482490] queue_iova+0x19c/0x1f0 [ 48.482624] cleanup_page_dma.isra.64+0x62/0xb0 [i915] [ 48.482749] __gen8_ppgtt_cleanup+0x63/0x80 [i915] [ 48.482873] __gen8_ppgtt_cleanup+0x42/0x80 [i915] [ 48.482999] __gen8_ppgtt_clear+0x433/0x4b0 [i915] [ 48.483123] __gen8_ppgtt_cl
