| Message ID | 20240606133302.17540-4-shameerali.kolothum.thodi@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add IOMMUFD dirty tracking support for SMMUv3 | expand |
Hi Shameer, Some nitpicking inline. On Thu, Jun 06, 2024 at 02:32:59PM +0100, Shameer Kolothum wrote: > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > index e30ce55ed1af..c05a74aa52a4 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -3738,6 +3738,29 @@ static void arm_smmu_device_iidr_probe(struct arm_smmu_device *smmu) > } > } > > +static void arm_smmu_get_httu(struct arm_smmu_device *smmu, u32 reg) Following the other helper arm_smmu_device_iidr_probe, how about arm_smmu_device_httu_probe? And we could pass in FIELD_GET(IDR0_HTTU, reg) too? > +{ > + u32 fw_features = smmu->features & (ARM_SMMU_FEAT_HA | ARM_SMMU_FEAT_HD); > + u32 httu = FIELD_GET(IDR0_HTTU, reg); > + u32 features = 0; How about "hw_feats" v.s. "fw_feats"? > + > + switch (httu) { > + case IDR0_HTTU_ACCESS_DIRTY: > + features |= ARM_SMMU_FEAT_HD; > + fallthrough; > + case IDR0_HTTU_ACCESS: > + features |= ARM_SMMU_FEAT_HA; > + } > + > + if (smmu->dev->of_node) > + smmu->features |= features; > + else if (features != fw_features) > + /* ACPI IORT sets the HTTU bits */ > + dev_warn(smmu->dev, > + "IDR0.HTTU(0x%x) overridden by FW configuration (0x%x)\n", > + httu, fw_features); httu and fw_features have different shifts -- could be odd to see: IDR0.HTTU(0x2) overridden by FW configuration (0x600000) FIELD_GET(ACPI_IORT_SMMU_V3_HTTU_OVERRIDE, iort_smmu->flags) seems to be overcomplicated to reference? So, how about just features? + "IDR0.HTTU features (0x%x) overridden by FW configuration (0x%x)\n", Thanks Nicolin
> -----Original Message----- > From: Nicolin Chen <nicolinc@nvidia.com> > Sent: Friday, June 7, 2024 12:11 AM > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > Cc: iommu@lists.linux.dev; linux-arm-kernel@lists.infradead.org; > robin.murphy@arm.com; will@kernel.org; joro@8bytes.org; > jgg@nvidia.com; ryan.roberts@arm.com; kevin.tian@intel.com; > mshavit@google.com; eric.auger@redhat.com; joao.m.martins@oracle.com; > jiangkunkun <jiangkunkun@huawei.com>; zhukeqian > <zhukeqian1@huawei.com>; Linuxarm <linuxarm@huawei.com> > Subject: Re: [PATCH v5 3/6] iommu/arm-smmu-v3: Add feature detection for > HTTU > > Hi Shameer, > > Some nitpicking inline. Thanks for taking a look Nicolin. > > On Thu, Jun 06, 2024 at 02:32:59PM +0100, Shameer Kolothum wrote: > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > index e30ce55ed1af..c05a74aa52a4 100644 > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > @@ -3738,6 +3738,29 @@ static void arm_smmu_device_iidr_probe(struct > arm_smmu_device *smmu) > > } > > } > > > > +static void arm_smmu_get_httu(struct arm_smmu_device *smmu, u32 > reg) > > Following the other helper arm_smmu_device_iidr_probe, how about > arm_smmu_device_httu_probe? Hmm..I don't have any objection per se but "_get_" feels like more simple and straightforward to me. Let me know if you feel strongly about this. > And we could pass in FIELD_GET(IDR0_HTTU, reg) too? Same here. > > > +{ > > + u32 fw_features = smmu->features & (ARM_SMMU_FEAT_HA | > ARM_SMMU_FEAT_HD); > > + u32 httu = FIELD_GET(IDR0_HTTU, reg); > > + u32 features = 0; > > How about "hw_feats" v.s. "fw_feats"? Ok. > > > + > > + switch (httu) { > > + case IDR0_HTTU_ACCESS_DIRTY: > > + features |= ARM_SMMU_FEAT_HD; > > + fallthrough; > > + case IDR0_HTTU_ACCESS: > > + features |= ARM_SMMU_FEAT_HA; > > + } > > + > > + if (smmu->dev->of_node) > > + smmu->features |= features; > > + else if (features != fw_features) > > + /* ACPI IORT sets the HTTU bits */ > > + dev_warn(smmu->dev, > > + "IDR0.HTTU(0x%x) overridden by FW configuration > (0x%x)\n", > > + httu, fw_features); > > httu and fw_features have different shifts -- could be odd to see: > IDR0.HTTU(0x2) overridden by FW configuration (0x600000) > > FIELD_GET(ACPI_IORT_SMMU_V3_HTTU_OVERRIDE, iort_smmu->flags) > seems > to be overcomplicated to reference? So, how about just features? > + "IDR0.HTTU features (0x%x) overridden by FW > configuration (0x%x)\n", Yes. I think that make sense. Will change. Thanks, Shameer
On Fri, Jun 07, 2024 at 08:04:45AM +0000, Shameerali Kolothum Thodi wrote: > > On Thu, Jun 06, 2024 at 02:32:59PM +0100, Shameer Kolothum wrote: > > > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > > index e30ce55ed1af..c05a74aa52a4 100644 > > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > > @@ -3738,6 +3738,29 @@ static void arm_smmu_device_iidr_probe(struct > > arm_smmu_device *smmu) > > > } > > > } > > > > > > +static void arm_smmu_get_httu(struct arm_smmu_device *smmu, u32 > > reg) > > > > Following the other helper arm_smmu_device_iidr_probe, how about > > arm_smmu_device_httu_probe? > > Hmm..I don't have any objection per se but "_get_" feels like more simple > and straightforward to me. Let me know if you feel strongly about this. > > > And we could pass in FIELD_GET(IDR0_HTTU, reg) too? > > Same here. They are not critical. So I'd say keep it as is if you prefer :) Thanks Nicolin
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index e30ce55ed1af..c05a74aa52a4 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -3738,6 +3738,29 @@ static void arm_smmu_device_iidr_probe(struct arm_smmu_device *smmu) } } +static void arm_smmu_get_httu(struct arm_smmu_device *smmu, u32 reg) +{ + u32 fw_features = smmu->features & (ARM_SMMU_FEAT_HA | ARM_SMMU_FEAT_HD); + u32 httu = FIELD_GET(IDR0_HTTU, reg); + u32 features = 0; + + switch (httu) { + case IDR0_HTTU_ACCESS_DIRTY: + features |= ARM_SMMU_FEAT_HD; + fallthrough; + case IDR0_HTTU_ACCESS: + features |= ARM_SMMU_FEAT_HA; + } + + if (smmu->dev->of_node) + smmu->features |= features; + else if (features != fw_features) + /* ACPI IORT sets the HTTU bits */ + dev_warn(smmu->dev, + "IDR0.HTTU(0x%x) overridden by FW configuration (0x%x)\n", + httu, fw_features); +} + static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu) { u32 reg; @@ -3798,6 +3821,8 @@ static int arm_smmu_device_hw_probe(struct arm_smmu_device *smmu) smmu->features |= ARM_SMMU_FEAT_E2H; } + arm_smmu_get_httu(smmu, reg); + /* * The coherency feature as set by FW is used in preference to the ID * register, but warn on mismatch. @@ -3993,6 +4018,14 @@ static int arm_smmu_device_acpi_probe(struct platform_device *pdev, if (iort_smmu->flags & ACPI_IORT_SMMU_V3_COHACC_OVERRIDE) smmu->features |= ARM_SMMU_FEAT_COHERENCY; + switch (FIELD_GET(ACPI_IORT_SMMU_V3_HTTU_OVERRIDE, iort_smmu->flags)) { + case IDR0_HTTU_ACCESS_DIRTY: + smmu->features |= ARM_SMMU_FEAT_HD; + fallthrough; + case IDR0_HTTU_ACCESS: + smmu->features |= ARM_SMMU_FEAT_HA; + } + return 0; } #else diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h index 1242a086c9f9..3f69ffefe4ee 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -33,6 +33,9 @@ #define IDR0_ASID16 (1 << 12) #define IDR0_ATS (1 << 10) #define IDR0_HYP (1 << 9) +#define IDR0_HTTU GENMASK(7, 6) +#define IDR0_HTTU_ACCESS 1 +#define IDR0_HTTU_ACCESS_DIRTY 2 #define IDR0_COHACC (1 << 4) #define IDR0_TTF GENMASK(3, 2) #define IDR0_TTF_AARCH64 2 @@ -648,6 +651,8 @@ struct arm_smmu_device { #define ARM_SMMU_FEAT_E2H (1 << 18) #define ARM_SMMU_FEAT_NESTING (1 << 19) #define ARM_SMMU_FEAT_ATTR_TYPES_OVR (1 << 20) +#define ARM_SMMU_FEAT_HA (1 << 21) +#define ARM_SMMU_FEAT_HD (1 << 22) u32 features; #define ARM_SMMU_OPT_SKIP_PREFETCH (1 << 0)