* Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging() [not found] <5-v2-c86cc8c2230e+160bb-smmu_newapi_jgg@nvidia.com> @ 2024-02-09 20:05 ` Dmitry Baryshkov 2024-02-09 22:23 ` Jason Gunthorpe 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Baryshkov @ 2024-02-09 20:05 UTC (permalink / raw) To: jgg Cc: iommu, joro, linux-arm-kernel, nicolinc, robin.murphy, will, linux-arm-msm On Tue, 17 Oct 2023 Jason Gunthorpe <jgg@nvidia.com> wrote: > Now that the BLOCKED and IDENTITY behaviors are managed with their own > domains change to the domain_alloc_paging() op. > > The check for using_legacy_binding is now redundant, > arm_smmu_def_domain_type() always returns IOMMU_DOMAIN_IDENTITY for this > mode, so the core code will never attempt to create a DMA domain in the > first place. > > Since commit a4fdd9762272 ("iommu: Use flush queue capability") the core > code only passes in IDENTITY/BLOCKED/UNMANAGED/DMA domain types. It will > not pass in IDENTITY or BLOCKED if the global statics exist, so the test > for DMA is also redundant now too. > > Call arm_smmu_init_domain_context() early if a dev is available. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/arm/arm-smmu/arm-smmu.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) For some reason this patch breaks booting of the APQ8096 Dragonboard820c (qcom/apq8096-db820c.dts). Dispbling display subsystem (mdss) and venus devices makes the board boot in most of the cases. Most frequently the last parts of the log loog in a following way: arm-smmu b40000.iommu: probing hardware configuration... arm-smmu b40000.iommu: SMMUv2 with: arm-smmu b40000.iommu: stage 1 translation arm-smmu b40000.iommu: address translation ops arm-smmu b40000.iommu: non-coherent table walk arm-smmu b40000.iommu: (IDR0.CTTW overridden by FW configuration) arm-smmu b40000.iommu: stream matching with 2 register groups arm-smmu b40000.iommu: 2 context banks (0 stage-2 only) arm-smmu b40000.iommu: Supported page sizes: 0x63315000 arm-smmu b40000.iommu: Stage-1: 48-bit VA -> 36-bit IPA arm-smmu b40000.iommu: preserved 0 boot mappings arm-smmu d00000.iommu: probing hardware configuration... arm-smmu d00000.iommu: SMMUv2 with: arm-smmu d00000.iommu: stage 1 translation arm-smmu d00000.iommu: address translation ops arm-smmu d00000.iommu: non-coherent table walk arm-smmu d00000.iommu: (IDR0.CTTW overridden by FW configuration) arm-smmu d00000.iommu: stream matching with 2 register groups arm-smmu d00000.iommu: 2 context banks (0 stage-2 only) arm-smmu d00000.iommu: Supported page sizes: 0x63315000 arm-smmu d00000.iommu: Stage-1: 32-bit VA -> 36-bit IPA arm-smmu d00000.iommu: preserved 0 boot mappings arm-smmu d40000.iommu: probing hardware configuration... arm-smmu d40000.iommu: SMMUv2 with: arm-smmu d40000.iommu: stage 1 translation arm-smmu d40000.iommu: address translation ops arm-smmu d40000.iommu: non-coherent table walk arm-smmu d40000.iommu: (IDR0.CTTW overridden by FW configuration) arm-smmu d40000.iommu: stream matching with 42 register groups arm-smmu d40000.iommu: 7 context banks (0 stage-2 only) arm-smmu d40000.iommu: Supported page sizes: 0x63315000 arm-smmu d40000.iommu: Stage-1: 32-bit VA -> 36-bit IPA arm-smmu d40000.iommu: preserved 0 boot mappings arm-smmu da0000.iommu: probing hardware configuration... arm-smmu da0000.iommu: SMMUv2 with: arm-smmu da0000.iommu: stage 1 translation arm-smmu da0000.iommu: address translation ops arm-smmu da0000.iommu: non-coherent table walk arm-smmu da0000.iommu: (IDR0.CTTW overridden by FW configuration) arm-smmu da0000.iommu: stream matching with 4 register groups arm-smmu da0000.iommu: 2 context banks (0 stage-2 only) arm-smmu da0000.iommu: Supported page sizes: 0x63315000 arm-smmu da0000.iommu: Stage-1: 32-bit VA -> 36-bit IPA arm-smmu da0000.iommu: preserved 0 boot mappings arm-smmu 1600000.iommu: probing hardware configuration... arm-smmu 1600000.iommu: SMMUv2 with: arm-smmu 1600000.iommu: stage 1 translation arm-smmu 1600000.iommu: address translation ops arm-smmu 1600000.iommu: non-coherent table walk arm-smmu 1600000.iommu: (IDR0.CTTW overridden by FW configuration) arm-smmu 1600000.iommu: stream matching with 15 register groups arm-smmu 1600000.iommu: 12 context banks (0 stage-2 only) arm-smmu 1600000.iommu: Supported page sizes: 0x63315000 arm-smmu 1600000.iommu: Stage-1: 36-bit VA -> 36-bit IPA arm-smmu 1600000.iommu: preserved 0 boot mappings adreno b00000.gpu: Adding to iommu group 0 Bluetooth: hci0: QCA Product ID :0x00000008 Bluetooth: hci0: QCA SOC Version :0x00000044 Bluetooth: hci0: QCA ROM Version :0x00000302 Bluetooth: hci0: QCA Patch Version:0x00000111 Bluetooth: hci0: QCA controller version 0x00440302 platform 9a0000.hdmi-tx: Fixed dependency cycle(s) with /soc@0/display-subsystem@900000/display-controller@901000/ports/port@0/endpoint Bluetooth: hci0: QCA Downloading qca/rampatch_00440302.bin spi_qup 7575000.spi: IN:block:16, fifo:64, OUT:block:16, fifo:64 spi_qup 75ba000.spi: IN:block:16, fifo:64, OUT:block:16, fifo:64 phy phy-7411000.phy.5: QUSB2PHY pll lock failed: status reg = 0 phy phy-7411000.phy.5: phy init failed --> -16 dwc3 6a00000.usb: error -EBUSY: failed to initialize core dwc3: probe of 6a00000.usb failed with error -16 phy phy-7412000.phy.6: QUSB2PHY pll lock failed: status reg = 0 phy phy-7412000.phy.6: phy init failed --> -16 dwc3 7600000.usb: error -EBUSY: failed to initialize core dwc3: probe of 7600000.usb failed with error -16 i2c_qup 7577000.i2c: using default clock-frequency 100000 i2c_qup 75b5000.i2c: using default clock-frequency 100000 sdhci_msm 74a4900.mmc: Got CD GPIO scsi host0: ufshcd ufshcd-qcom 624000.ufshc: ufs_qcom_host_reset: reset control not set remoteproc remoteproc0: 2080000.remoteproc is available remoteproc remoteproc1: 9300000.remoteproc is available mmc0: SDHCI controller on 74a4900.mmc [74a4900.mmc] using ADMA 64-bit qcom-pcie 600000.pcie: host bridge /soc@0/bus@0/pcie@600000 ranges: qcom-pcie 608000.pcie: supply vddpe-3v3 not found, using dummy regulator [ The board resets to the bootloader ] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging() 2024-02-09 20:05 ` [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging() Dmitry Baryshkov @ 2024-02-09 22:23 ` Jason Gunthorpe 2024-02-12 23:18 ` Dmitry Baryshkov 2024-02-13 7:51 ` Dmitry Baryshkov 0 siblings, 2 replies; 9+ messages in thread From: Jason Gunthorpe @ 2024-02-09 22:23 UTC (permalink / raw) To: Dmitry Baryshkov Cc: iommu, joro, linux-arm-kernel, nicolinc, robin.murphy, will, linux-arm-msm On Fri, Feb 09, 2024 at 10:05:38PM +0200, Dmitry Baryshkov wrote: > On Tue, 17 Oct 2023 Jason Gunthorpe <jgg@nvidia.com> wrote: > > Now that the BLOCKED and IDENTITY behaviors are managed with their own > > domains change to the domain_alloc_paging() op. > > > > The check for using_legacy_binding is now redundant, > > arm_smmu_def_domain_type() always returns IOMMU_DOMAIN_IDENTITY for this > > mode, so the core code will never attempt to create a DMA domain in the > > first place. > > > > Since commit a4fdd9762272 ("iommu: Use flush queue capability") the core > > code only passes in IDENTITY/BLOCKED/UNMANAGED/DMA domain types. It will > > not pass in IDENTITY or BLOCKED if the global statics exist, so the test > > for DMA is also redundant now too. > > > > Call arm_smmu_init_domain_context() early if a dev is available. > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > --- > > drivers/iommu/arm/arm-smmu/arm-smmu.c | 21 +++++++++++++++------ > > 1 file changed, 15 insertions(+), 6 deletions(-) > > For some reason this patch breaks booting of the APQ8096 Dragonboard820c > (qcom/apq8096-db820c.dts). Dispbling display subsystem (mdss) and venus > devices makes the board boot in most of the cases. Most frequently the > last parts of the log loog in a following way: It is surprising we tested this patch on some tegra systems with this iommu and didn't hit anything.. The only real functional thing this changes is to move the domain initialization up in time, potentially a lot in time in some cases. That function does alot of things including touching HW so possibly there is some surprising interaction with something else. So, I would expect this to not WARN_ON and to make it work the same as before the patch: --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c @@ -875,7 +875,9 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev) mutex_init(&smmu_domain->init_mutex); spin_lock_init(&smmu_domain->cb_lock); - if (dev) { + WARN_ON(using_legacy_binding); + +/* if (dev) { struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev); if (arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev)) { @@ -883,7 +885,7 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev) return NULL; } } - +*/ return &smmu_domain->domain; } Then I'd ask you to remove the comment and do: @@ -878,7 +878,9 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev) if (dev) { struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev); + WARN_ON(true); if (arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev)) { + printk("Allocation failure in arm_smmu_domain_alloc_paging()\n"); kfree(smmu_domain); return NULL; } And then we may get a clue from the backtraces it generates. I only saw one iommu group reported in your log so I'd expect one trace? Thanks, Jason ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging() 2024-02-09 22:23 ` Jason Gunthorpe @ 2024-02-12 23:18 ` Dmitry Baryshkov 2024-02-13 0:19 ` Jason Gunthorpe 2024-02-13 7:51 ` Dmitry Baryshkov 1 sibling, 1 reply; 9+ messages in thread From: Dmitry Baryshkov @ 2024-02-12 23:18 UTC (permalink / raw) To: Jason Gunthorpe Cc: iommu, joro, linux-arm-kernel, nicolinc, robin.murphy, will, linux-arm-msm On Sat, 10 Feb 2024 at 00:23, Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Fri, Feb 09, 2024 at 10:05:38PM +0200, Dmitry Baryshkov wrote: > > On Tue, 17 Oct 2023 Jason Gunthorpe <jgg@nvidia.com> wrote: > > > Now that the BLOCKED and IDENTITY behaviors are managed with their own > > > domains change to the domain_alloc_paging() op. > > > > > > The check for using_legacy_binding is now redundant, > > > arm_smmu_def_domain_type() always returns IOMMU_DOMAIN_IDENTITY for this > > > mode, so the core code will never attempt to create a DMA domain in the > > > first place. > > > > > > Since commit a4fdd9762272 ("iommu: Use flush queue capability") the core > > > code only passes in IDENTITY/BLOCKED/UNMANAGED/DMA domain types. It will > > > not pass in IDENTITY or BLOCKED if the global statics exist, so the test > > > for DMA is also redundant now too. > > > > > > Call arm_smmu_init_domain_context() early if a dev is available. > > > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > > --- > > > drivers/iommu/arm/arm-smmu/arm-smmu.c | 21 +++++++++++++++------ > > > 1 file changed, 15 insertions(+), 6 deletions(-) > > > > For some reason this patch breaks booting of the APQ8096 Dragonboard820c > > (qcom/apq8096-db820c.dts). Dispbling display subsystem (mdss) and venus > > devices makes the board boot in most of the cases. Most frequently the > > last parts of the log loog in a following way: > > It is surprising we tested this patch on some tegra systems with this > iommu and didn't hit anything.. > > The only real functional thing this changes is to move the domain > initialization up in time, potentially a lot in time in some > cases. That function does alot of things including touching HW so > possibly there is some surprising interaction with something else. > > So, I would expect this to not WARN_ON and to make it work the same as > before the patch: > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c > @@ -875,7 +875,9 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev) > mutex_init(&smmu_domain->init_mutex); > spin_lock_init(&smmu_domain->cb_lock); > > - if (dev) { > + WARN_ON(using_legacy_binding); > + > +/* if (dev) { > struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev); > > if (arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev)) { > @@ -883,7 +885,7 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev) > return NULL; > } > } > - > +*/ > return &smmu_domain->domain; > } With the full device tree, this crashed during the IOMMU probe, no warnings: [ 15.343477] bam-dma-engine 644000.dma-controller: num-channels unspecified in dt [ 15.343675] bam-dma-engine 644000.dma-controller: num-ees unspecified in dt [ 15.394653] msm_serial 7570000.serial: msm_serial: detected port #2 [ 15.395170] msm_serial 7570000.serial: uartclk = 19200000 [ 15.401983] 7570000.serial: ttyMSM2 at MMIO 0x7570000 (irq = 37, base_baud = 1200000) is a MSM [ 15.407549] serial serial0: tty port ttyMSM2 registered [ 15.421567] Bluetooth: hci0: setting up ROME/QCA6390 [ 15.421728] arm-smmu b40000.iommu: probing hardware configuration... [ 15.425866] arm-smmu b40000.iommu: SMMUv2 with: [ 15.432153] arm-smmu b40000.iommu: stage 1 translation [ 15.436407] arm-smmu b40000.iommu: address translation ops [ 15.441580] arm-smmu b40000.iommu: non-coherent table walk [ 15.447136] arm-smmu b40000.iommu: (IDR0.CTTW overridden by FW configuration) [ 15.452750] arm-smmu b40000.iommu: stream matching with 2 register groups [ 15.460038] arm-smmu b40000.iommu: 2 context banks (0 stage-2 only) [ 15.466908] arm-smmu b40000.iommu: Supported page sizes: 0x63315000 [ 15.473367] arm-smmu b40000.iommu: Stage-1: 48-bit VA -> 36-bit IPA [ 15.481638] arm-smmu b40000.iommu: preserved 0 boot mappings [ 15.491783] arm-smmu d00000.iommu: probing hardware configuration... [ 15.491955] arm-smmu d00000.iommu: SMMUv2 with: [ Format: Log Type - Time(microsec) - Message - Optional Info Log Type: B - Since Boot(Power On Reset), D - Delta, S - Statistic S - QC_IMAGE_VERSION_STRING=BOOT.XF.1.0-00301 > > Then I'd ask you to remove the comment and do: > > @@ -878,7 +878,9 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev) > if (dev) { > struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev); > > + WARN_ON(true); > if (arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev)) { > + printk("Allocation failure in arm_smmu_domain_alloc_paging()\n"); > kfree(smmu_domain); > return NULL; > } > > > And then we may get a clue from the backtraces it generates. I only > saw one iommu group reported in your log so I'd expect one trace? With the full device tree, same result: [ 12.319303] bam-dma-engine 644000.dma-controller: num-channels unspecified in dt [ 12.319502] bam-dma-engine 644000.dma-controller: num-ees unspecified in dt [ 12.370379] msm_serial 7570000.serial: msm_serial: detected port #2 [ 12.370895] msm_serial 7570000.serial: uartclk = 19200000 [ 12.377773] 7570000.serial: ttyMSM2 at MMIO 0x7570000 (irq = 37, base_baud = 1200000) is a MSM [ 12.383228] serial serial0: tty port ttyMSM2 registered [ 12.397263] arm-smmu b40000.iommu: probing hardware configuration... [ 12.397441] arm-smmu b40000.iommu: SMMUv2 with: [ 12.398072] Bluetooth: hci0: setting up ROME/QCA6390 [ 12.402779] arm-smmu b40000.iommu: stage 1 translation [ 12.402817] arm-smmu b40000.iommu: address translation ops [ 12.402832] arm-smmu b40000.iommu: non-coherent table walk [ 12.402848] arm-smmu b40000.iommu: (IDR0.CTTW overridden by FW configuration) [ 12.402881] arm-smmu b40000.iommu: stream matching with 2 register groups [ 12.402943] arm-smmu b40000.iommu: 2 context banks (0 stage-2 only) [ 12.402987] arm-smmu b40000.iommu: Supported page sizes: 0x63315000 [ 12.403004] arm-smmu b40000.iommu: Stage-1: 48-bit VA -> 36-bit IPA [ 12.404941] arm-smmu b40000.iommu: preserved 0 boot mappings [ 12.467015] arm-smmu d00000.iommu: probing hardware configuration... [ 12.467318] arm-smmu d0 Format: Log Type - Time(microsec) - Message - Optional Info Log Type: B - Since Boot(Power On Reset), D - Delta, S - Statistic S - QC_IMAGE_VERSION_STRING=BOOT.XF.1.0-00301 Even with disabling display-subsystem and venus, it crashes at the same place, I don't seem to be able to get past it anymore. -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging() 2024-02-12 23:18 ` Dmitry Baryshkov @ 2024-02-13 0:19 ` Jason Gunthorpe 0 siblings, 0 replies; 9+ messages in thread From: Jason Gunthorpe @ 2024-02-13 0:19 UTC (permalink / raw) To: Dmitry Baryshkov Cc: iommu, joro, linux-arm-kernel, nicolinc, robin.murphy, will, linux-arm-msm On Tue, Feb 13, 2024 at 01:18:24AM +0200, Dmitry Baryshkov wrote: > > > For some reason this patch breaks booting of the APQ8096 Dragonboard820c > > > (qcom/apq8096-db820c.dts). Dispbling display subsystem (mdss) and venus > > > devices makes the board boot in most of the cases. Most frequently the > > > last parts of the log loog in a following way: > > > > It is surprising we tested this patch on some tegra systems with this > > iommu and didn't hit anything.. > > > > The only real functional thing this changes is to move the domain > > initialization up in time, potentially a lot in time in some > > cases. That function does alot of things including touching HW so > > possibly there is some surprising interaction with something else. > > > > So, I would expect this to not WARN_ON and to make it work the same as > > before the patch: > > > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c > > @@ -875,7 +875,9 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev) > > mutex_init(&smmu_domain->init_mutex); > > spin_lock_init(&smmu_domain->cb_lock); > > > > - if (dev) { > > + WARN_ON(using_legacy_binding); > > + > > +/* if (dev) { > > struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev); > > > > if (arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev)) { > > @@ -883,7 +885,7 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev) > > return NULL; > > } > > } > > - > > +*/ > > return &smmu_domain->domain; > > } > > With the full device tree, this crashed during the IOMMU probe, no warnings: The above reverts nearly all the functional elements of the patch you said caused the problem, are you certain of your bisection? > > And then we may get a clue from the backtraces it generates. I only > > saw one iommu group reported in your log so I'd expect one trace? > > With the full device tree, same result: This adds basically an unconditional WARN_ON on all the probe paths, and nothing printed? That is even more surprising. Those two together suggest that arm_smmu_domain_alloc_paging() isn't even being called. But the caller is: if (alloc_type == IOMMU_DOMAIN_IDENTITY && ops->identity_domain) return ops->identity_domain; else if (alloc_type == IOMMU_DOMAIN_BLOCKED && ops->blocked_domain) return ops->blocked_domain; else if (type & __IOMMU_DOMAIN_PAGING && ops->domain_alloc_paging) domain = ops->domain_alloc_paging(dev); else if (ops->domain_alloc) domain = ops->domain_alloc(alloc_type); else return ERR_PTR(-EOPNOTSUPP); Which, suggest that alloc_type is somehow garbage for your system? I don't see how that can happen, but this patch will also cause a garbage type to be rejected by the core code. Does this reveal anything for you? @@ -2118,6 +2118,7 @@ static struct iommu_domain *__iommu_domain_alloc(const struct iommu_ops *ops, struct iommu_domain *domain; unsigned int alloc_type = type & IOMMU_DOMAIN_ALLOC_FLAGS; + WARN(true, " __iommu_domain_alloc %u %u", alloc_type, type); if (alloc_type == IOMMU_DOMAIN_IDENTITY && ops->identity_domain) return ops->identity_domain; else if (alloc_type == IOMMU_DOMAIN_BLOCKED && ops->blocked_domain) @@ -2126,8 +2127,10 @@ static struct iommu_domain *__iommu_domain_alloc(const struct iommu_ops *ops, domain = ops->domain_alloc_paging(dev); else if (ops->domain_alloc) domain = ops->domain_alloc(alloc_type); - else + else { + printk("Returning failure from __iommu_domain_alloc()\n"); return ERR_PTR(-EOPNOTSUPP); + } /* * Many domain_alloc ops now return ERR_PTR, make things easier for the It must print, something is wrong with the debugging process if this doesn't generate back traces :\ Jason ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging() 2024-02-09 22:23 ` Jason Gunthorpe 2024-02-12 23:18 ` Dmitry Baryshkov @ 2024-02-13 7:51 ` Dmitry Baryshkov 2024-02-13 10:20 ` Robin Murphy 1 sibling, 1 reply; 9+ messages in thread From: Dmitry Baryshkov @ 2024-02-13 7:51 UTC (permalink / raw) To: Jason Gunthorpe Cc: iommu, joro, linux-arm-kernel, nicolinc, robin.murphy, will, linux-arm-msm On Sat, 10 Feb 2024 at 00:23, Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Fri, Feb 09, 2024 at 10:05:38PM +0200, Dmitry Baryshkov wrote: > > On Tue, 17 Oct 2023 Jason Gunthorpe <jgg@nvidia.com> wrote: > > > Now that the BLOCKED and IDENTITY behaviors are managed with their own > > > domains change to the domain_alloc_paging() op. > > > > > > The check for using_legacy_binding is now redundant, > > > arm_smmu_def_domain_type() always returns IOMMU_DOMAIN_IDENTITY for this > > > mode, so the core code will never attempt to create a DMA domain in the > > > first place. > > > > > > Since commit a4fdd9762272 ("iommu: Use flush queue capability") the core > > > code only passes in IDENTITY/BLOCKED/UNMANAGED/DMA domain types. It will > > > not pass in IDENTITY or BLOCKED if the global statics exist, so the test > > > for DMA is also redundant now too. > > > > > > Call arm_smmu_init_domain_context() early if a dev is available. > > > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > > --- > > > drivers/iommu/arm/arm-smmu/arm-smmu.c | 21 +++++++++++++++------ > > > 1 file changed, 15 insertions(+), 6 deletions(-) > > > > For some reason this patch breaks booting of the APQ8096 Dragonboard820c > > (qcom/apq8096-db820c.dts). Dispbling display subsystem (mdss) and venus > > devices makes the board boot in most of the cases. Most frequently the > > last parts of the log loog in a following way: > > It is surprising we tested this patch on some tegra systems with this > iommu and didn't hit anything.. > > The only real functional thing this changes is to move the domain > initialization up in time, potentially a lot in time in some > cases. That function does alot of things including touching HW so > possibly there is some surprising interaction with something else. I should not be debugging strange platforms at 1 a.m. I forgot that there was another patch to revert. So after reverting the MPM patch, I'm getting the following results: > > So, I would expect this to not WARN_ON and to make it work the same as > before the patch: No warnings, the platform now boots up to the point of actually bringing up the venus device: [ 11.906514] ath10k_pci 0000:01:00.0: qca6174 hw3.2 target 0x05030000 chip_id 0x00340aff sub 0000:0000 [ 11.907119] ath10k_pci 0000:01:00.0: kconfig debug 1 debugfs 0 tracing 0 dfs 0 testmode 0 [ 11.915881] ath10k_pci 0000:01:00.0: firmware ver WLAN.RM.4.4.1-00288- api 6 features wowlan,ignore-otp,mfp crc32 bf907c7c [ 11.979972] Console: switching to colour frame buffer device 320x90 [ 11.990756] ath10k_pci 0000:01:00.0: board_file api 2 bmi_id 0:1 crc32 d2863f91 [ 12.060834] msm_mdp 901000.display-controller: [drm] fb0: msmdrmfb frame buffer device [ 12.096203] qcom-pcie 608000.pcie: Phy link never came up [ 12.103785] qcom-pcie 608000.pcie: PCI host bridge to bus 0001:00 [ 12.103970] qcom-venus c00000.video-codec: Adding to iommu group 3 Format: Log Type - Time(microsec) - Message - Optional Info Log Type: B - Since Boot(Power On Reset), D - Delta, S - Statistic S - QC_IMAGE_VERSION_STRING=BOOT.XF.1.0-00301 S - IMAGE_VARIANT_STRING=M8996LAB S - OEM_IMAGE_VERSION_STRING=crm-ubuntu68 S - Boot Interface: UFS > > Then I'd ask you to remove the comment and do: > > @@ -878,7 +878,9 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev) > if (dev) { > struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev); > > + WARN_ON(true); > if (arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev)) { > + printk("Allocation failure in arm_smmu_domain_alloc_paging()\n"); > kfree(smmu_domain); > return NULL; > } > > > And then we may get a clue from the backtraces it generates. I only > saw one iommu group reported in your log so I'd expect one trace? I added dev_info + mdelays() around the arm_smmu_init_domain_context() and I can see that it crashes within that function. [ 29.819624] qcom-venus c00000.video-codec: Adding to iommu group 1 [ 29.833181] ------------[ cut here ]------------ [ 29.839198] WARNING: CPU: 1 PID: 35 at drivers/iommu/arm/arm-smmu/arm-smmu.c:883 arm_smmu_domain_alloc_paging+0x80/0x174 [ 29.843980] Modules linked in: [ 29.854824] CPU: 1 PID: 35 Comm: kworker/u18:0 Tainted: G U 6.8.0-rc3-next-20240208-05495-g20708c29957d-dirty #1739 [ 29.857694] Hardware name: Qualcomm Technologies, Inc. DB820c (DT) [ 29.869410] Workqueue: events_unbound deferred_probe_work_func [ 29.875658] pstate: 40000005 (nZcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 29.881474] pc : arm_smmu_domain_alloc_paging+0x80/0x174 [ 29.888331] lr : arm_smmu_domain_alloc_paging+0x68/0x174 [ 29.893885] sp : ffff8000830338c0 [ 29.899179] x29: ffff8000830338c0 x28: 0000000000000000 x27: ffff800081e72000 [ 29.902396] x26: ffff00008034ee48 x25: ffff000080b24810 x24: 0000000000000000 [ 29.909513] x23: ffff800081e73000 x22: ffff000080b24810 x21: ffff800082e23258 [ 29.916633] x20: ffff00008389a700 x19: ffff00008034f600 x18: ffffffffffffffff [ 29.918788] usb 1-1: new high-speed USB device number 2 using xhci-hcd [ 29.923746] x17: 0000000c0000000b x16: 0000000900000008 x15: 0000000000000000 [ 29.923765] x14: 000000000000b0af x13: 0000000000000000 x12: 0000000000000166 [ 29.923783] x11: 0000000000000001 x10: 0000000000001410 x9 : 0000000000000000 [ 29.923801] x8 : ffff00008034f800 x7 : 0000000000000000 x6 : 0000000000000000 [ 29.923819] x5 : 0000000000000000 x4 : 0000000000000002 x3 : 0000000000000000 [ 29.923837] x2 : ffff800082e23290 x1 : dead4ead00000000 x0 : 0000000000000000 [ 29.923855] Call trace: [ 29.923861] arm_smmu_domain_alloc_paging+0x80/0x174 [ 29.923872] __iommu_domain_alloc+0xcc/0xf4 [ 29.923884] iommu_setup_default_domain+0x294/0x554 [ 29.938567] Bluetooth: hci0: Frame reassembly failed (-84) [ 29.944494] __iommu_probe_device+0x418/0x43c [ 29.944508] iommu_probe_device+0x3c/0x80 [ 29.944519] of_iommu_configure+0x124/0x1b4 [ 29.944529] of_dma_configure_id+0x170/0x2f4 [ 29.969874] mmc0: new ultra high speed SDR104 SDHC card at address 5048 [ 29.972966] platform_dma_configure+0xa8/0xb4 [ 29.972983] really_probe+0x70/0x2ac [ 29.972992] __driver_probe_device+0x78/0x12c [ 29.973001] driver_probe_device+0xd8/0x160 [ 29.973010] __device_attach_driver+0xb8/0x138 [ 29.973019] bus_for_each_drv+0x80/0xdc [ 29.973027] __device_attach+0x9c/0x188 [ 29.973037] device_initial_probe+0x14/0x20 [ 29.973046] bus_probe_device+0xac/0xb0 [ 29.973055] deferred_probe_work_func+0x8c/0xc8 [ 29.973064] process_one_work+0x210/0x5e4 [ 29.983596] mmcblk0: mmc0:5048 SD32G 28.8 GiB [ 29.987546] worker_thread+0x1bc/0x38c [ 29.987558] kthread+0x120/0x124 [ 29.987568] ret_from_fork+0x10/0x20 [ 29.987579] irq event stamp: 109977 [ 29.987584] hardirqs last enabled at (109977): [<ffff800080fbbc48>] _raw_spin_unlock_irqrestore+0x6c/0x70 [ 29.987600] hardirqs last disabled at (109976): [<ffff800080fbb0a8>] _raw_spin_lock_irqsave+0x84/0x88 [ 29.987610] softirqs last enabled at (109966): [<ffff800080090680>] __do_softirq+0x498/0x4e0 [ 29.987619] softirqs last disabled at (109961): [<ffff800080096184>] ____do_softirq+0x10/0x1c [ 30.006747] mmcblk0: p1 [ 30.010291] ---[ end trace 0000000000000000 ]--- [ 30.018630] remoteproc remoteproc1: remote processor 9300000.remoteproc is now up [ 30.024525] qcom-pcie 600000.pcie: iATU: unroll F, 32 ob, 8 ib, align 4K, limit 4G [ 30.044747] qcom,apr remoteproc1:smd-edge.apr_audio_svc.-1.-1: Adding APR/GPR dev: aprsvc:service:4:3 [ 30.046118] qcom-pcie 600000.pcie: Invalid eDMA IRQs found [ 30.051718] qcom,apr remoteproc1:smd-edge.apr_audio_svc.-1.-1: Adding APR/GPR dev: aprsvc:service:4:4 [ 30.066435] Bluetooth: hci0: QCA Downloading qca/nvm_00440302.bin [ 30.130736] hub 1-1:1.0: USB hub found [ 30.150390] qcom-pcie 600000.pcie: PCIe Gen.1 x1 link up [ 30.156394] hub 1-1:1.0: 4 ports detected [ 30.161837] qcom-pcie 600000.pcie: PCI host bridge to bus 0000:00 [ 30.189583] pci_bus 0000:00: root bus resource [bus 00-ff] [ 30.195652] pci_bus 0000:00: root bus resource [io 0x0000-0xfffff] [ 30.201035] pci_bus 0000:00: root bus resource [mem 0x0c300000-0x0cffffff] [ 30.205424] Bluetooth: hci0: QCA setup on UART is completed [ 30.207262] pci 0000:00:00.0: [17cb:0104] type 01 class 0x060400 PCIe Root Port [ 30.214380] usb 2-1: new SuperSpeed USB device number 2 using xhci-hcd [ 30.219636] qcom-venus c00000.video-codec: Allocating domain [ 30.221503] pci 0000:00:00.0: BAR 0 [mem 0x00000000-0x00000fff] [ 30.221680] pci 0000:00:00.0: PCI bridge to [bus 01-ff] [ 30.221772] pci 0000:00:00.0: bridge window [io 0x0000-0x0fff] [ 30.221832] pci 0000:00:00.0: bridge window [mem 0x00000000-0x000fffff] [ 30.221945] pci 0000:00:00.0: bridge window [mem 0x00000000-0x000fffff 64bit pref] [ 30.222617] pci 0000:00:00.0: PME# supported from D0 D3hot [ 30.273673] hub 2-1:1.0: USB hub found [ 30.276567] hub 2-1:1.0: 4 ports detected Format: Log Type - Time(microsec) - Message - Optional Info Log Type: B - Since Boot(Power On Reset), D - Delta, S - Statistic S - QC_IMAGE_VERSION_STRING=BOOT.XF.1.0-00301 S - IMAGE_VARIANT_STRING=M8996LAB S - OEM_IMAGE_VERSION_STRING=crm-ubuntu68 S - Boot Interface: UFS I traced this further, it crashes during arm_smmu_write_context_bank(). -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging() 2024-02-13 7:51 ` Dmitry Baryshkov @ 2024-02-13 10:20 ` Robin Murphy 2024-02-13 10:55 ` Dmitry Baryshkov 0 siblings, 1 reply; 9+ messages in thread From: Robin Murphy @ 2024-02-13 10:20 UTC (permalink / raw) To: Dmitry Baryshkov, Jason Gunthorpe Cc: iommu, joro, linux-arm-kernel, nicolinc, will, linux-arm-msm On 2024-02-13 7:51 am, Dmitry Baryshkov wrote: > On Sat, 10 Feb 2024 at 00:23, Jason Gunthorpe <jgg@nvidia.com> wrote: >> >> On Fri, Feb 09, 2024 at 10:05:38PM +0200, Dmitry Baryshkov wrote: >>> On Tue, 17 Oct 2023 Jason Gunthorpe <jgg@nvidia.com> wrote: >>>> Now that the BLOCKED and IDENTITY behaviors are managed with their own >>>> domains change to the domain_alloc_paging() op. >>>> >>>> The check for using_legacy_binding is now redundant, >>>> arm_smmu_def_domain_type() always returns IOMMU_DOMAIN_IDENTITY for this >>>> mode, so the core code will never attempt to create a DMA domain in the >>>> first place. >>>> >>>> Since commit a4fdd9762272 ("iommu: Use flush queue capability") the core >>>> code only passes in IDENTITY/BLOCKED/UNMANAGED/DMA domain types. It will >>>> not pass in IDENTITY or BLOCKED if the global statics exist, so the test >>>> for DMA is also redundant now too. >>>> >>>> Call arm_smmu_init_domain_context() early if a dev is available. >>>> >>>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> >>>> --- >>>> drivers/iommu/arm/arm-smmu/arm-smmu.c | 21 +++++++++++++++------ >>>> 1 file changed, 15 insertions(+), 6 deletions(-) >>> >>> For some reason this patch breaks booting of the APQ8096 Dragonboard820c >>> (qcom/apq8096-db820c.dts). Dispbling display subsystem (mdss) and venus >>> devices makes the board boot in most of the cases. Most frequently the >>> last parts of the log loog in a following way: >> >> It is surprising we tested this patch on some tegra systems with this >> iommu and didn't hit anything.. >> >> The only real functional thing this changes is to move the domain >> initialization up in time, potentially a lot in time in some >> cases. That function does alot of things including touching HW so >> possibly there is some surprising interaction with something else. > > I should not be debugging strange platforms at 1 a.m. I forgot that > there was another patch to revert. So after reverting the MPM patch, > I'm getting the following results: > >> >> So, I would expect this to not WARN_ON and to make it work the same as >> before the patch: > > No warnings, the platform now boots up to the point of actually > bringing up the venus device: > > > [ 11.906514] ath10k_pci 0000:01:00.0: qca6174 hw3.2 target > 0x05030000 chip_id 0x00340aff sub 0000:0000 > [ 11.907119] ath10k_pci 0000:01:00.0: kconfig debug 1 debugfs 0 > tracing 0 dfs 0 testmode 0 > [ 11.915881] ath10k_pci 0000:01:00.0: firmware ver > WLAN.RM.4.4.1-00288- api 6 features wowlan,ignore-otp,mfp crc32 > bf907c7c > [ 11.979972] Console: switching to colour frame buffer device 320x90 > [ 11.990756] ath10k_pci 0000:01:00.0: board_file api 2 bmi_id 0:1 > crc32 d2863f91 > [ 12.060834] msm_mdp 901000.display-controller: [drm] fb0: msmdrmfb > frame buffer device > [ 12.096203] qcom-pcie 608000.pcie: Phy link never came up > [ 12.103785] qcom-pcie 608000.pcie: PCI host bridge to bus 0001:00 > [ 12.103970] qcom-venus c00000.video-codec: Adding to iommu group 3 > > Format: Log Type - Time(microsec) - Message - Optional Info > Log Type: B - Since Boot(Power On Reset), D - Delta, S - Statistic > S - QC_IMAGE_VERSION_STRING=BOOT.XF.1.0-00301 > S - IMAGE_VARIANT_STRING=M8996LAB > S - OEM_IMAGE_VERSION_STRING=crm-ubuntu68 > S - Boot Interface: UFS > >> >> Then I'd ask you to remove the comment and do: >> >> @@ -878,7 +878,9 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev) >> if (dev) { >> struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev); >> >> + WARN_ON(true); >> if (arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev)) { >> + printk("Allocation failure in arm_smmu_domain_alloc_paging()\n"); >> kfree(smmu_domain); >> return NULL; >> } >> >> >> And then we may get a clue from the backtraces it generates. I only >> saw one iommu group reported in your log so I'd expect one trace? > > I added dev_info + mdelays() around the arm_smmu_init_domain_context() > and I can see that it crashes within that function. Yeah, this is totally broken. We can't just call the unmodified arm_smmu_init_domain_context() at domain allocation because half of what it's doing belongs to the attach operation. We should not be allocating context banks, IRQs, etc. for a not-yet-attached domain, and we certainly shouldn't be touching hardware there outside of RPM. Thanks, Robin. > > [ 29.819624] qcom-venus c00000.video-codec: Adding to iommu group 1 > [ 29.833181] ------------[ cut here ]------------ > [ 29.839198] WARNING: CPU: 1 PID: 35 at > drivers/iommu/arm/arm-smmu/arm-smmu.c:883 > arm_smmu_domain_alloc_paging+0x80/0x174 > [ 29.843980] Modules linked in: > [ 29.854824] CPU: 1 PID: 35 Comm: kworker/u18:0 Tainted: G U > 6.8.0-rc3-next-20240208-05495-g20708c29957d-dirty #1739 > [ 29.857694] Hardware name: Qualcomm Technologies, Inc. DB820c (DT) > [ 29.869410] Workqueue: events_unbound deferred_probe_work_func > [ 29.875658] pstate: 40000005 (nZcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 29.881474] pc : arm_smmu_domain_alloc_paging+0x80/0x174 > [ 29.888331] lr : arm_smmu_domain_alloc_paging+0x68/0x174 > [ 29.893885] sp : ffff8000830338c0 > [ 29.899179] x29: ffff8000830338c0 x28: 0000000000000000 x27: ffff800081e72000 > [ 29.902396] x26: ffff00008034ee48 x25: ffff000080b24810 x24: 0000000000000000 > [ 29.909513] x23: ffff800081e73000 x22: ffff000080b24810 x21: ffff800082e23258 > [ 29.916633] x20: ffff00008389a700 x19: ffff00008034f600 x18: ffffffffffffffff > [ 29.918788] usb 1-1: new high-speed USB device number 2 using xhci-hcd > [ 29.923746] x17: 0000000c0000000b x16: 0000000900000008 x15: 0000000000000000 > [ 29.923765] x14: 000000000000b0af x13: 0000000000000000 x12: 0000000000000166 > [ 29.923783] x11: 0000000000000001 x10: 0000000000001410 x9 : 0000000000000000 > [ 29.923801] x8 : ffff00008034f800 x7 : 0000000000000000 x6 : 0000000000000000 > [ 29.923819] x5 : 0000000000000000 x4 : 0000000000000002 x3 : 0000000000000000 > [ 29.923837] x2 : ffff800082e23290 x1 : dead4ead00000000 x0 : 0000000000000000 > [ 29.923855] Call trace: > [ 29.923861] arm_smmu_domain_alloc_paging+0x80/0x174 > [ 29.923872] __iommu_domain_alloc+0xcc/0xf4 > [ 29.923884] iommu_setup_default_domain+0x294/0x554 > [ 29.938567] Bluetooth: hci0: Frame reassembly failed (-84) > [ 29.944494] __iommu_probe_device+0x418/0x43c > [ 29.944508] iommu_probe_device+0x3c/0x80 > [ 29.944519] of_iommu_configure+0x124/0x1b4 > [ 29.944529] of_dma_configure_id+0x170/0x2f4 > [ 29.969874] mmc0: new ultra high speed SDR104 SDHC card at address 5048 > [ 29.972966] platform_dma_configure+0xa8/0xb4 > [ 29.972983] really_probe+0x70/0x2ac > [ 29.972992] __driver_probe_device+0x78/0x12c > [ 29.973001] driver_probe_device+0xd8/0x160 > [ 29.973010] __device_attach_driver+0xb8/0x138 > [ 29.973019] bus_for_each_drv+0x80/0xdc > [ 29.973027] __device_attach+0x9c/0x188 > [ 29.973037] device_initial_probe+0x14/0x20 > [ 29.973046] bus_probe_device+0xac/0xb0 > [ 29.973055] deferred_probe_work_func+0x8c/0xc8 > [ 29.973064] process_one_work+0x210/0x5e4 > [ 29.983596] mmcblk0: mmc0:5048 SD32G 28.8 GiB > [ 29.987546] worker_thread+0x1bc/0x38c > [ 29.987558] kthread+0x120/0x124 > [ 29.987568] ret_from_fork+0x10/0x20 > [ 29.987579] irq event stamp: 109977 > [ 29.987584] hardirqs last enabled at (109977): > [<ffff800080fbbc48>] _raw_spin_unlock_irqrestore+0x6c/0x70 > [ 29.987600] hardirqs last disabled at (109976): > [<ffff800080fbb0a8>] _raw_spin_lock_irqsave+0x84/0x88 > [ 29.987610] softirqs last enabled at (109966): > [<ffff800080090680>] __do_softirq+0x498/0x4e0 > [ 29.987619] softirqs last disabled at (109961): > [<ffff800080096184>] ____do_softirq+0x10/0x1c > [ 30.006747] mmcblk0: p1 > [ 30.010291] ---[ end trace 0000000000000000 ]--- > [ 30.018630] remoteproc remoteproc1: remote processor > 9300000.remoteproc is now up > [ 30.024525] qcom-pcie 600000.pcie: iATU: unroll F, 32 ob, 8 ib, > align 4K, limit 4G > [ 30.044747] qcom,apr remoteproc1:smd-edge.apr_audio_svc.-1.-1: > Adding APR/GPR dev: aprsvc:service:4:3 > [ 30.046118] qcom-pcie 600000.pcie: Invalid eDMA IRQs found > [ 30.051718] qcom,apr remoteproc1:smd-edge.apr_audio_svc.-1.-1: > Adding APR/GPR dev: aprsvc:service:4:4 > [ 30.066435] Bluetooth: hci0: QCA Downloading qca/nvm_00440302.bin > [ 30.130736] hub 1-1:1.0: USB hub found > [ 30.150390] qcom-pcie 600000.pcie: PCIe Gen.1 x1 link up > [ 30.156394] hub 1-1:1.0: 4 ports detected > [ 30.161837] qcom-pcie 600000.pcie: PCI host bridge to bus 0000:00 > [ 30.189583] pci_bus 0000:00: root bus resource [bus 00-ff] > [ 30.195652] pci_bus 0000:00: root bus resource [io 0x0000-0xfffff] > [ 30.201035] pci_bus 0000:00: root bus resource [mem 0x0c300000-0x0cffffff] > [ 30.205424] Bluetooth: hci0: QCA setup on UART is completed > [ 30.207262] pci 0000:00:00.0: [17cb:0104] type 01 class 0x060400 > PCIe Root Port > [ 30.214380] usb 2-1: new SuperSpeed USB device number 2 using xhci-hcd > [ 30.219636] qcom-venus c00000.video-codec: Allocating domain > [ 30.221503] pci 0000:00:00.0: BAR 0 [mem 0x00000000-0x00000fff] > [ 30.221680] pci 0000:00:00.0: PCI bridge to [bus 01-ff] > [ 30.221772] pci 0000:00:00.0: bridge window [io 0x0000-0x0fff] > [ 30.221832] pci 0000:00:00.0: bridge window [mem 0x00000000-0x000fffff] > [ 30.221945] pci 0000:00:00.0: bridge window [mem > 0x00000000-0x000fffff 64bit pref] > [ 30.222617] pci 0000:00:00.0: PME# supported from D0 D3hot > [ 30.273673] hub 2-1:1.0: USB hub found > [ 30.276567] hub 2-1:1.0: 4 ports detected > > Format: Log Type - Time(microsec) - Message - Optional Info > Log Type: B - Since Boot(Power On Reset), D - Delta, S - Statistic > S - QC_IMAGE_VERSION_STRING=BOOT.XF.1.0-00301 > S - IMAGE_VARIANT_STRING=M8996LAB > S - OEM_IMAGE_VERSION_STRING=crm-ubuntu68 > S - Boot Interface: UFS > > I traced this further, it crashes during arm_smmu_write_context_bank(). > > > > -- > With best wishes > Dmitry ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging() 2024-02-13 10:20 ` Robin Murphy @ 2024-02-13 10:55 ` Dmitry Baryshkov 2024-02-13 11:16 ` Will Deacon 0 siblings, 1 reply; 9+ messages in thread From: Dmitry Baryshkov @ 2024-02-13 10:55 UTC (permalink / raw) To: Robin Murphy Cc: Jason Gunthorpe, iommu, joro, linux-arm-kernel, nicolinc, will, linux-arm-msm On Tue, 13 Feb 2024 at 12:20, Robin Murphy <robin.murphy@arm.com> wrote: > > On 2024-02-13 7:51 am, Dmitry Baryshkov wrote: > > On Sat, 10 Feb 2024 at 00:23, Jason Gunthorpe <jgg@nvidia.com> wrote: > >> > >> On Fri, Feb 09, 2024 at 10:05:38PM +0200, Dmitry Baryshkov wrote: > >>> On Tue, 17 Oct 2023 Jason Gunthorpe <jgg@nvidia.com> wrote: > >>>> Now that the BLOCKED and IDENTITY behaviors are managed with their own > >>>> domains change to the domain_alloc_paging() op. > >>>> > >>>> The check for using_legacy_binding is now redundant, > >>>> arm_smmu_def_domain_type() always returns IOMMU_DOMAIN_IDENTITY for this > >>>> mode, so the core code will never attempt to create a DMA domain in the > >>>> first place. > >>>> > >>>> Since commit a4fdd9762272 ("iommu: Use flush queue capability") the core > >>>> code only passes in IDENTITY/BLOCKED/UNMANAGED/DMA domain types. It will > >>>> not pass in IDENTITY or BLOCKED if the global statics exist, so the test > >>>> for DMA is also redundant now too. > >>>> > >>>> Call arm_smmu_init_domain_context() early if a dev is available. > >>>> > >>>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > >>>> --- > >>>> drivers/iommu/arm/arm-smmu/arm-smmu.c | 21 +++++++++++++++------ > >>>> 1 file changed, 15 insertions(+), 6 deletions(-) > >>> > >>> For some reason this patch breaks booting of the APQ8096 Dragonboard820c > >>> (qcom/apq8096-db820c.dts). Dispbling display subsystem (mdss) and venus > >>> devices makes the board boot in most of the cases. Most frequently the > >>> last parts of the log loog in a following way: > >> > >> It is surprising we tested this patch on some tegra systems with this > >> iommu and didn't hit anything.. > >> > >> The only real functional thing this changes is to move the domain > >> initialization up in time, potentially a lot in time in some > >> cases. That function does alot of things including touching HW so > >> possibly there is some surprising interaction with something else. > > > > I should not be debugging strange platforms at 1 a.m. I forgot that > > there was another patch to revert. So after reverting the MPM patch, > > I'm getting the following results: > > > >> > >> So, I would expect this to not WARN_ON and to make it work the same as > >> before the patch: > > > > No warnings, the platform now boots up to the point of actually > > bringing up the venus device: > > > > > > [ 11.906514] ath10k_pci 0000:01:00.0: qca6174 hw3.2 target > > 0x05030000 chip_id 0x00340aff sub 0000:0000 > > [ 11.907119] ath10k_pci 0000:01:00.0: kconfig debug 1 debugfs 0 > > tracing 0 dfs 0 testmode 0 > > [ 11.915881] ath10k_pci 0000:01:00.0: firmware ver > > WLAN.RM.4.4.1-00288- api 6 features wowlan,ignore-otp,mfp crc32 > > bf907c7c > > [ 11.979972] Console: switching to colour frame buffer device 320x90 > > [ 11.990756] ath10k_pci 0000:01:00.0: board_file api 2 bmi_id 0:1 > > crc32 d2863f91 > > [ 12.060834] msm_mdp 901000.display-controller: [drm] fb0: msmdrmfb > > frame buffer device > > [ 12.096203] qcom-pcie 608000.pcie: Phy link never came up > > [ 12.103785] qcom-pcie 608000.pcie: PCI host bridge to bus 0001:00 > > [ 12.103970] qcom-venus c00000.video-codec: Adding to iommu group 3 > > > > Format: Log Type - Time(microsec) - Message - Optional Info > > Log Type: B - Since Boot(Power On Reset), D - Delta, S - Statistic > > S - QC_IMAGE_VERSION_STRING=BOOT.XF.1.0-00301 > > S - IMAGE_VARIANT_STRING=M8996LAB > > S - OEM_IMAGE_VERSION_STRING=crm-ubuntu68 > > S - Boot Interface: UFS > > > >> > >> Then I'd ask you to remove the comment and do: > >> > >> @@ -878,7 +878,9 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev) > >> if (dev) { > >> struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev); > >> > >> + WARN_ON(true); > >> if (arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev)) { > >> + printk("Allocation failure in arm_smmu_domain_alloc_paging()\n"); > >> kfree(smmu_domain); > >> return NULL; > >> } > >> > >> > >> And then we may get a clue from the backtraces it generates. I only > >> saw one iommu group reported in your log so I'd expect one trace? > > > > I added dev_info + mdelays() around the arm_smmu_init_domain_context() > > and I can see that it crashes within that function. > > Yeah, this is totally broken. We can't just call the unmodified > arm_smmu_init_domain_context() at domain allocation because half of what > it's doing belongs to the attach operation. We should not be allocating > context banks, IRQs, etc. for a not-yet-attached domain, and we > certainly shouldn't be touching hardware there outside of RPM. Should I send a revert? > > Thanks, > Robin. -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging() 2024-02-13 10:55 ` Dmitry Baryshkov @ 2024-02-13 11:16 ` Will Deacon 2024-02-13 11:54 ` Jason Gunthorpe 0 siblings, 1 reply; 9+ messages in thread From: Will Deacon @ 2024-02-13 11:16 UTC (permalink / raw) To: Dmitry Baryshkov Cc: Robin Murphy, Jason Gunthorpe, iommu, joro, linux-arm-kernel, nicolinc, linux-arm-msm On Tue, Feb 13, 2024 at 12:55:38PM +0200, Dmitry Baryshkov wrote: > On Tue, 13 Feb 2024 at 12:20, Robin Murphy <robin.murphy@arm.com> wrote: > > On 2024-02-13 7:51 am, Dmitry Baryshkov wrote: > > > On Sat, 10 Feb 2024 at 00:23, Jason Gunthorpe <jgg@nvidia.com> wrote: > > >> And then we may get a clue from the backtraces it generates. I only > > >> saw one iommu group reported in your log so I'd expect one trace? > > > > > > I added dev_info + mdelays() around the arm_smmu_init_domain_context() > > > and I can see that it crashes within that function. > > > > Yeah, this is totally broken. We can't just call the unmodified > > arm_smmu_init_domain_context() at domain allocation because half of what > > it's doing belongs to the attach operation. We should not be allocating > > context banks, IRQs, etc. for a not-yet-attached domain, and we > > certainly shouldn't be touching hardware there outside of RPM. > > Should I send a revert? If reverting the patch fixes the issue for you, then yes please! Hopefully you can help Jason test a reworked verson for the future, as it's evident that Tegra doesn't tickle the power management side of things in the same way. Will ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging() 2024-02-13 11:16 ` Will Deacon @ 2024-02-13 11:54 ` Jason Gunthorpe 0 siblings, 0 replies; 9+ messages in thread From: Jason Gunthorpe @ 2024-02-13 11:54 UTC (permalink / raw) To: Will Deacon Cc: Dmitry Baryshkov, Robin Murphy, iommu, joro, linux-arm-kernel, nicolinc, linux-arm-msm On Tue, Feb 13, 2024 at 11:16:23AM +0000, Will Deacon wrote: > On Tue, Feb 13, 2024 at 12:55:38PM +0200, Dmitry Baryshkov wrote: > > On Tue, 13 Feb 2024 at 12:20, Robin Murphy <robin.murphy@arm.com> wrote: > > > On 2024-02-13 7:51 am, Dmitry Baryshkov wrote: > > > > On Sat, 10 Feb 2024 at 00:23, Jason Gunthorpe <jgg@nvidia.com> wrote: > > > >> And then we may get a clue from the backtraces it generates. I only > > > >> saw one iommu group reported in your log so I'd expect one trace? > > > > > > > > I added dev_info + mdelays() around the arm_smmu_init_domain_context() > > > > and I can see that it crashes within that function. > > > > > > Yeah, this is totally broken. We can't just call the unmodified > > > arm_smmu_init_domain_context() at domain allocation because half of what > > > it's doing belongs to the attach operation. We should not be allocating > > > context banks, IRQs, etc. for a not-yet-attached domain, and we > > > certainly shouldn't be touching hardware there outside of RPM. > > > > Should I send a revert? > > If reverting the patch fixes the issue for you, then yes please! Not the whole thing though, just remove the 'if (dev)' like you tested, thanks. If you want I will send it > Hopefully you can help Jason test a reworked verson for the future, as > it's evident that Tegra doesn't tickle the power management side of things > in the same way. It can stay, as long as it uses the alloc_domain_paging() that is enough for the core code to move forward. I included it only because we were able to test it, most of the other drivers I did not try to move their "finalize". Jason ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-02-13 11:54 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <5-v2-c86cc8c2230e+160bb-smmu_newapi_jgg@nvidia.com>
2024-02-09 20:05 ` [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging() Dmitry Baryshkov
2024-02-09 22:23 ` Jason Gunthorpe
2024-02-12 23:18 ` Dmitry Baryshkov
2024-02-13 0:19 ` Jason Gunthorpe
2024-02-13 7:51 ` Dmitry Baryshkov
2024-02-13 10:20 ` Robin Murphy
2024-02-13 10:55 ` Dmitry Baryshkov
2024-02-13 11:16 ` Will Deacon
2024-02-13 11:54 ` Jason Gunthorpe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox