Re: [PATCH] iommu/msm: add a check for the return of kzalloc()
On Fri, Mar 25, 2022 at 2:13 PM wrote:
>
> From: Xiaoke Wang
>
> kzalloc() is a memory allocation function which can return NULL when
> some internal memory errors happen. So it is better to check it to
> prevent potential wrong memory access.
>
> Signed-off-by: Xiaoke Wang
> ---
> drivers/iommu/msm_iommu.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
> index 3a38352..697ad63 100644
> --- a/drivers/iommu/msm_iommu.c
> +++ b/drivers/iommu/msm_iommu.c
> @@ -597,6 +597,10 @@ static void insert_iommu_master(struct device *dev,
>
> if (list_empty(&(*iommu)->ctx_list)) {
> master = kzalloc(sizeof(*master), GFP_ATOMIC);
> + if (!master) {
> + dev_err(dev, "Failed to allocate iommu_master\n");
How do you reconcile this with chapter 14 of the coding style document?
"These generic allocation functions all emit a stack dump on failure when used
without __GFP_NOWARN so there is no use in emitting an additional failure
message when NULL is returned."
> + return;
> + }
> master->of_node = dev->of_node;
> list_add(>list, &(*iommu)->ctx_list);
> dev_iommu_priv_set(dev, master);
> --
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC 1/2] iommu: arm-smmu: Handoff SMR registers and context banks
On 6/12/2019 12:42 PM, Bjorn Andersson wrote:
On Wed 12 Jun 11:07 PDT 2019, Jeffrey Hugo wrote:
On Wed, Jun 5, 2019 at 3:09 PM Bjorn Andersson
wrote:
Boot splash screen or EFI framebuffer requires the display hardware to
operate while the Linux iommu driver probes. Therefore, we cannot simply
wipe out the SMR register settings programmed by the bootloader.
Detect which SMR registers are in use during probe, and which context
banks they are associated with. Reserve these context banks for the
first Linux device whose stream-id matches the SMR register.
Any existing page-tables will be discarded.
Heavily based on downstream implementation by Patrick Daly
.
Signed-off-by: Bjorn Andersson
Reviewed-by: Jeffrey Hugo
This is very similar to the hacked up version I did, and I'm glad to see it
cleaned up and posted.
This is important work, and I want to see it move forward, however it doesn't
completely address everything, IMO. Fixing the remaining issues certainly
can be follow on work, but I don't know if they would end up affecting this
implementation.
So, with this series, we've gone from a crash on msm8998/sdm845, to causing
context faults. This is because while we are now not nuking the config, we
are preventing the bootloader installed translations from working. Essentially
the display already has a mapping (technically passthrough) that is likely being
used by EFIFB. The instant the SMMU inits, that mapping becomes invalid,
and the display is going to generate context faults. While not fatal,
this provides
a bad user experience as there are nasty messages, and EFIFB stops working.
The situation does get resolved once the display driver inits and takes over the
HW and SMMU mappings, so we are not stuck with it, but it would be nice to
have that addressed as well, ie have EFIFB working up until the Linux display
driver takes over.
But do you see this even though you don't enable the mdss driver?
The only way I can see that happening is if the SMMU leaves the context bank
alone, with M == 0, and the iommu framework defines a domain attribute or
some other mechanism to allow the driver to flip the M bit in the context bank
after installing the necessary handover translations.
From what I can tell this implementation leaves the framebuffer mapping
in working condition until the attach_dev of the display driver, at
which time we do get context faults until the display driver is done
initializing things.
So we're reducing the problem to a question of how to seamlessly carry
over the mapping during the attach.
Actually, you are correct. Without mdss this won't occur. The window
is from the creation of the default domain for the mdss device, to the
point that the display is fully init'd (and EFIFB is shut down).
Regards,
Bjorn
---
drivers/iommu/arm-smmu-regs.h | 2 +
drivers/iommu/arm-smmu.c | 80 ---
2 files changed, 77 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/arm-smmu-regs.h b/drivers/iommu/arm-smmu-regs.h
index e9132a926761..8c1fd84032a2 100644
--- a/drivers/iommu/arm-smmu-regs.h
+++ b/drivers/iommu/arm-smmu-regs.h
@@ -105,7 +105,9 @@
#define ARM_SMMU_GR0_SMR(n)(0x800 + ((n) << 2))
#define SMR_VALID (1 << 31)
#define SMR_MASK_SHIFT 16
+#define SMR_MASK_MASK 0x7fff
#define SMR_ID_SHIFT 0
+#define SMR_ID_MASK0x
#define ARM_SMMU_GR0_S2CR(n) (0xc00 + ((n) << 2))
#define S2CR_CBNDX_SHIFT 0
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 5e54cc0a28b3..c8629a656b42 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -135,6 +135,7 @@ struct arm_smmu_s2cr {
enum arm_smmu_s2cr_type type;
enum arm_smmu_s2cr_privcfg privcfg;
u8 cbndx;
+ boolhandoff;
};
#define s2cr_init_val (struct arm_smmu_s2cr){ \
@@ -399,9 +400,22 @@ static int arm_smmu_register_legacy_master(struct device
*dev,
return err;
}
-static int __arm_smmu_alloc_bitmap(unsigned long *map, int start, int end)
+static int __arm_smmu_alloc_cb(struct arm_smmu_device *smmu, int start,
+ struct device *dev)
{
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+ unsigned long *map = smmu->context_map;
+ int end = smmu->num_context_banks;
+ int cbndx;
int idx;
+ int i;
+
+ for_each_cfg_sme(fwspec, i, idx) {
+ if (smmu->s2crs[idx].handoff) {
+ cbndx = smmu->s2crs[idx].cbndx;
+ goto found_handoff;
+ }
+ }
do {
idx = find_next_zero_bit(map, end, start);
@@ -410,6 +424,17 @@ static int __arm_smmu_alloc_bitmap(unsigned
Re: [RFC 1/2] iommu: arm-smmu: Handoff SMR registers and context banks
On Wed, Jun 5, 2019 at 3:09 PM Bjorn Andersson
wrote:
>
> Boot splash screen or EFI framebuffer requires the display hardware to
> operate while the Linux iommu driver probes. Therefore, we cannot simply
> wipe out the SMR register settings programmed by the bootloader.
>
> Detect which SMR registers are in use during probe, and which context
> banks they are associated with. Reserve these context banks for the
> first Linux device whose stream-id matches the SMR register.
>
> Any existing page-tables will be discarded.
>
> Heavily based on downstream implementation by Patrick Daly
> .
>
> Signed-off-by: Bjorn Andersson
Reviewed-by: Jeffrey Hugo
This is very similar to the hacked up version I did, and I'm glad to see it
cleaned up and posted.
This is important work, and I want to see it move forward, however it doesn't
completely address everything, IMO. Fixing the remaining issues certainly
can be follow on work, but I don't know if they would end up affecting this
implementation.
So, with this series, we've gone from a crash on msm8998/sdm845, to causing
context faults. This is because while we are now not nuking the config, we
are preventing the bootloader installed translations from working. Essentially
the display already has a mapping (technically passthrough) that is likely being
used by EFIFB. The instant the SMMU inits, that mapping becomes invalid,
and the display is going to generate context faults. While not fatal,
this provides
a bad user experience as there are nasty messages, and EFIFB stops working.
The situation does get resolved once the display driver inits and takes over the
HW and SMMU mappings, so we are not stuck with it, but it would be nice to
have that addressed as well, ie have EFIFB working up until the Linux display
driver takes over.
The only way I can see that happening is if the SMMU leaves the context bank
alone, with M == 0, and the iommu framework defines a domain attribute or
some other mechanism to allow the driver to flip the M bit in the context bank
after installing the necessary handover translations.
> ---
> drivers/iommu/arm-smmu-regs.h | 2 +
> drivers/iommu/arm-smmu.c | 80 ---
> 2 files changed, 77 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-regs.h b/drivers/iommu/arm-smmu-regs.h
> index e9132a926761..8c1fd84032a2 100644
> --- a/drivers/iommu/arm-smmu-regs.h
> +++ b/drivers/iommu/arm-smmu-regs.h
> @@ -105,7 +105,9 @@
> #define ARM_SMMU_GR0_SMR(n)(0x800 + ((n) << 2))
> #define SMR_VALID (1 << 31)
> #define SMR_MASK_SHIFT 16
> +#define SMR_MASK_MASK 0x7fff
> #define SMR_ID_SHIFT 0
> +#define SMR_ID_MASK0x
>
> #define ARM_SMMU_GR0_S2CR(n) (0xc00 + ((n) << 2))
> #define S2CR_CBNDX_SHIFT 0
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 5e54cc0a28b3..c8629a656b42 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -135,6 +135,7 @@ struct arm_smmu_s2cr {
> enum arm_smmu_s2cr_type type;
> enum arm_smmu_s2cr_privcfg privcfg;
> u8 cbndx;
> + boolhandoff;
> };
>
> #define s2cr_init_val (struct arm_smmu_s2cr){ \
> @@ -399,9 +400,22 @@ static int arm_smmu_register_legacy_master(struct device
> *dev,
> return err;
> }
>
> -static int __arm_smmu_alloc_bitmap(unsigned long *map, int start, int end)
> +static int __arm_smmu_alloc_cb(struct arm_smmu_device *smmu, int start,
> + struct device *dev)
> {
> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> + unsigned long *map = smmu->context_map;
> + int end = smmu->num_context_banks;
> + int cbndx;
> int idx;
> + int i;
> +
> + for_each_cfg_sme(fwspec, i, idx) {
> + if (smmu->s2crs[idx].handoff) {
> + cbndx = smmu->s2crs[idx].cbndx;
> + goto found_handoff;
> + }
> + }
>
> do {
> idx = find_next_zero_bit(map, end, start);
> @@ -410,6 +424,17 @@ static int __arm_smmu_alloc_bitmap(unsigned long *map,
> int start, int end)
> } while (test_and_set_bit(idx, map));
>
> return idx;
> +
> +found_handoff:
> + for (i = 0; i < smmu->num_mapping_groups; i++) {
> + if (smmu->s2crs[i].cbndx == cbndx) {
> + smmu->s2crs[i].cbndx = 0;
> + smmu->s2crs[i].handoff = false;
> +
Re: [RFC 2/2] iommu: arm-smmu: Don't blindly use first SMR to calculate mask
On Wed, Jun 5, 2019 at 3:09 PM Bjorn Andersson
wrote:
>
> With the SMRs inherited from the bootloader the first SMR might actually
> be valid and in use. As such probing the SMR mask using the first SMR
> might break a stream in use. Search for an unused stream and use this to
> probe the SMR mask.
>
> Signed-off-by: Bjorn Andersson
Reviewed-by: Jeffrey Hugo
I don't quite like the situation where the is no SMR to compute the mask, but I
think the way you've handled it is the best option/
I'm curious, why is this not included in patch #1? Seems like patch
#1 introduces
the issue, yet doesn't also fix it.
> ---
> drivers/iommu/arm-smmu.c | 20
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index c8629a656b42..0c6f5fe6f382 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1084,23 +1084,35 @@ static void arm_smmu_test_smr_masks(struct
> arm_smmu_device *smmu)
> {
> void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
> u32 smr;
> + int idx;
>
> if (!smmu->smrs)
> return;
>
> + for (idx = 0; idx < smmu->num_mapping_groups; idx++) {
> + smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(idx));
> + if (!(smr & SMR_VALID))
> + break;
> + }
> +
> + if (idx == smmu->num_mapping_groups) {
> + dev_err(smmu->dev, "Unable to compute streamid_mask\n");
> + return;
> + }
> +
> /*
> * SMR.ID bits may not be preserved if the corresponding MASK
> * bits are set, so check each one separately. We can reject
> * masters later if they try to claim IDs outside these masks.
> */
> smr = smmu->streamid_mask << SMR_ID_SHIFT;
> - writel_relaxed(smr, gr0_base + ARM_SMMU_GR0_SMR(0));
> - smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(0));
> + writel_relaxed(smr, gr0_base + ARM_SMMU_GR0_SMR(idx));
> + smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(idx));
> smmu->streamid_mask = smr >> SMR_ID_SHIFT;
>
> smr = smmu->streamid_mask << SMR_MASK_SHIFT;
> - writel_relaxed(smr, gr0_base + ARM_SMMU_GR0_SMR(0));
> - smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(0));
> + writel_relaxed(smr, gr0_base + ARM_SMMU_GR0_SMR(idx));
> + smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(idx));
> smmu->smr_mask_mask = smr >> SMR_MASK_SHIFT;
> }
>
> --
> 2.18.0
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH v1 2/3] arm64: dts: qcom: msm8998: Add PCIe SMMU node
On 3/29/2019 12:29 PM, Robin Murphy wrote: On 29/03/2019 10:51, Marc Gonzalez wrote: On 28/03/2019 18:05, Marc Gonzalez wrote: + + #global-interrupts = <0>; + interrupts = + , + , + , + , + , + ; + }; + The rest of the node looks fairly straight-forward. You should probably have some "bus" and "iface" clocks too, per the requirement of "qcom,smmu-v2". Maybe Vivek might know what's relevant for MSM8998? The clocks that power the SMMU are not under the control of Linux, but rather the RPM subsystem. The interface to them is the interconnect framework, which does not yet support 8998 per my understanding. The clocks will always be on, but perhaps not at the best rate for performance until the interconnect is brought in. -- Jeffrey Hugo Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
