| Message ID | 854e2b3f2d4f1b76f916fcfc288b34526d4d4162.1692693557.git.nicolinc@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu/arm-smmu-v3: Reduce latency in __arm_smmu_tlb_inv_range() | expand |
On 2023-08-22 09:45, Nicolin Chen wrote: > Move the part of per-asid or per-vmid invalidation command issuing into a > new helper function, which will be used in the following change. Why? This achieves nothing except make the code harder to follow and disconnect the rather important comment even further from the code it is significant to. It's not like we need a specific prototype to take a function pointer from, it's just another internal call - see arm_smmu_flush_iotlb_all() for instance. We know the cookie is an arm_smmu_domain pointer because we put it there, and converting it back from a void pointer is exactly the same *at* the function call boundary as immediately afterwards. Thanks, Robin. > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 24 +++++++++++++-------- > 1 file changed, 15 insertions(+), 9 deletions(-) > > 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 9b0dc3505601..d6c647e1eb01 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -1854,12 +1854,24 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid, > return arm_smmu_cmdq_batch_submit(smmu_domain->smmu, &cmds); > } > > +static void arm_smmu_tlb_inv_domain(struct arm_smmu_domain *smmu_domain) > +{ > + struct arm_smmu_device *smmu = smmu_domain->smmu; > + struct arm_smmu_cmdq_ent cmd; > + > + if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { > + arm_smmu_tlb_inv_asid(smmu, smmu_domain->s1_cfg.cd.asid); > + } else { > + cmd.opcode = CMDQ_OP_TLBI_S12_VMALL; > + cmd.tlbi.vmid = smmu_domain->s2_cfg.vmid; > + arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd); > + } > +} > + > /* IO_PGTABLE API */ > static void arm_smmu_tlb_inv_context(void *cookie) > { > struct arm_smmu_domain *smmu_domain = cookie; > - struct arm_smmu_device *smmu = smmu_domain->smmu; > - struct arm_smmu_cmdq_ent cmd; > > /* > * NOTE: when io-pgtable is in non-strict mode, we may get here with > @@ -1868,13 +1880,7 @@ static void arm_smmu_tlb_inv_context(void *cookie) > * insertion to guarantee those are observed before the TLBI. Do be > * careful, 007. > */ > - if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { > - arm_smmu_tlb_inv_asid(smmu, smmu_domain->s1_cfg.cd.asid); > - } else { > - cmd.opcode = CMDQ_OP_TLBI_S12_VMALL; > - cmd.tlbi.vmid = smmu_domain->s2_cfg.vmid; > - arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd); > - } > + arm_smmu_tlb_inv_domain(smmu_domain); > arm_smmu_atc_inv_domain(smmu_domain, 0, 0, 0); > } >
On Tue, Aug 22, 2023 at 10:40:18AM +0100, Robin Murphy wrote: > On 2023-08-22 09:45, Nicolin Chen wrote: > > Move the part of per-asid or per-vmid invalidation command issuing into a > > new helper function, which will be used in the following change. > > Why? This achieves nothing except make the code harder to follow and > disconnect the rather important comment even further from the code it is We need the same if-else routine to issue a per-asid or per-vmid TLBI command. If making a copy of this same routine feels better to you, yea, I can change that. > significant to. It's not like we need a specific prototype to take a > function pointer from, it's just another internal call - see > arm_smmu_flush_iotlb_all() for instance. We know the cookie is an > arm_smmu_domain pointer because we put it there, and converting it back > from a void pointer is exactly the same *at* the function call boundary > as immediately afterwards. Hmm, I am not quite following this. What do you suggest here? Thanks Nic > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > > --- > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 24 +++++++++++++-------- > > 1 file changed, 15 insertions(+), 9 deletions(-) > > > > 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 9b0dc3505601..d6c647e1eb01 100644 > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > @@ -1854,12 +1854,24 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid, > > return arm_smmu_cmdq_batch_submit(smmu_domain->smmu, &cmds); > > } > > > > +static void arm_smmu_tlb_inv_domain(struct arm_smmu_domain *smmu_domain) > > +{ > > + struct arm_smmu_device *smmu = smmu_domain->smmu; > > + struct arm_smmu_cmdq_ent cmd; > > + > > + if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { > > + arm_smmu_tlb_inv_asid(smmu, smmu_domain->s1_cfg.cd.asid); > > + } else { > > + cmd.opcode = CMDQ_OP_TLBI_S12_VMALL; > > + cmd.tlbi.vmid = smmu_domain->s2_cfg.vmid; > > + arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd); > > + } > > +} > > + > > /* IO_PGTABLE API */ > > static void arm_smmu_tlb_inv_context(void *cookie) > > { > > struct arm_smmu_domain *smmu_domain = cookie; > > - struct arm_smmu_device *smmu = smmu_domain->smmu; > > - struct arm_smmu_cmdq_ent cmd; > > > > /* > > * NOTE: when io-pgtable is in non-strict mode, we may get here with > > @@ -1868,13 +1880,7 @@ static void arm_smmu_tlb_inv_context(void *cookie) > > * insertion to guarantee those are observed before the TLBI. Do be > > * careful, 007. > > */ > > - if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { > > - arm_smmu_tlb_inv_asid(smmu, smmu_domain->s1_cfg.cd.asid); > > - } else { > > - cmd.opcode = CMDQ_OP_TLBI_S12_VMALL; > > - cmd.tlbi.vmid = smmu_domain->s2_cfg.vmid; > > - arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd); > > - } > > + arm_smmu_tlb_inv_domain(smmu_domain); > > arm_smmu_atc_inv_domain(smmu_domain, 0, 0, 0); > > } > >
On 2023-08-22 18:03, Nicolin Chen wrote: > On Tue, Aug 22, 2023 at 10:40:18AM +0100, Robin Murphy wrote: > >> On 2023-08-22 09:45, Nicolin Chen wrote: >>> Move the part of per-asid or per-vmid invalidation command issuing into a >>> new helper function, which will be used in the following change. >> >> Why? This achieves nothing except make the code harder to follow and >> disconnect the rather important comment even further from the code it is > > We need the same if-else routine to issue a per-asid or per-vmid > TLBI command. If making a copy of this same routine feels better > to you, yea, I can change that. > >> significant to. It's not like we need a specific prototype to take a >> function pointer from, it's just another internal call - see >> arm_smmu_flush_iotlb_all() for instance. We know the cookie is an >> arm_smmu_domain pointer because we put it there, and converting it back >> from a void pointer is exactly the same *at* the function call boundary >> as immediately afterwards. > > Hmm, I am not quite following this. What do you suggest here? Oh, this is becoming quite the lesson in not reviewing patches in a hurry :( Apparently I managed to misread the diff and the horribly subtle difference between "arm_smmu_tlb_inv_domain" and "arm_smmu_atc_inv_domain", and think that arm_smmu_tlb_inv_context() was already just dealing with the TLBI command and you were moving the entire body into the new helper. Sorry about that. Still, the part about the comment remains true, and I think it goes to show what a thoroughly horrible naming scheme it is to have "tlb_inv" denote a function responsible for TLBI commands and "atc_inv" denote a function responsible for ATC commands and "tlb_inv" denote a function responsible for both TLBI and ATC commands... Thanks, Robin. > > Thanks > Nic > >>> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> >>> --- >>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 24 +++++++++++++-------- >>> 1 file changed, 15 insertions(+), 9 deletions(-) >>> >>> 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 9b0dc3505601..d6c647e1eb01 100644 >>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c >>> @@ -1854,12 +1854,24 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid, >>> return arm_smmu_cmdq_batch_submit(smmu_domain->smmu, &cmds); >>> } >>> >>> +static void arm_smmu_tlb_inv_domain(struct arm_smmu_domain *smmu_domain) >>> +{ >>> + struct arm_smmu_device *smmu = smmu_domain->smmu; >>> + struct arm_smmu_cmdq_ent cmd; >>> + >>> + if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { >>> + arm_smmu_tlb_inv_asid(smmu, smmu_domain->s1_cfg.cd.asid); >>> + } else { >>> + cmd.opcode = CMDQ_OP_TLBI_S12_VMALL; >>> + cmd.tlbi.vmid = smmu_domain->s2_cfg.vmid; >>> + arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd); >>> + } >>> +} >>> + >>> /* IO_PGTABLE API */ >>> static void arm_smmu_tlb_inv_context(void *cookie) >>> { >>> struct arm_smmu_domain *smmu_domain = cookie; >>> - struct arm_smmu_device *smmu = smmu_domain->smmu; >>> - struct arm_smmu_cmdq_ent cmd; >>> >>> /* >>> * NOTE: when io-pgtable is in non-strict mode, we may get here with >>> @@ -1868,13 +1880,7 @@ static void arm_smmu_tlb_inv_context(void *cookie) >>> * insertion to guarantee those are observed before the TLBI. Do be >>> * careful, 007. >>> */ >>> - if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { >>> - arm_smmu_tlb_inv_asid(smmu, smmu_domain->s1_cfg.cd.asid); >>> - } else { >>> - cmd.opcode = CMDQ_OP_TLBI_S12_VMALL; >>> - cmd.tlbi.vmid = smmu_domain->s2_cfg.vmid; >>> - arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd); >>> - } >>> + arm_smmu_tlb_inv_domain(smmu_domain); >>> arm_smmu_atc_inv_domain(smmu_domain, 0, 0, 0); >>> } >>>
On Tue, Aug 29, 2023 at 10:54:00PM +0100, Robin Murphy wrote: > On 2023-08-22 18:03, Nicolin Chen wrote: > > On Tue, Aug 22, 2023 at 10:40:18AM +0100, Robin Murphy wrote: > > > > > On 2023-08-22 09:45, Nicolin Chen wrote: > > > > Move the part of per-asid or per-vmid invalidation command issuing into a > > > > new helper function, which will be used in the following change. > > > > > > Why? This achieves nothing except make the code harder to follow and > > > disconnect the rather important comment even further from the code it is > > > > We need the same if-else routine to issue a per-asid or per-vmid > > TLBI command. If making a copy of this same routine feels better > > to you, yea, I can change that. > > > > > significant to. It's not like we need a specific prototype to take a > > > function pointer from, it's just another internal call - see > > > arm_smmu_flush_iotlb_all() for instance. We know the cookie is an > > > arm_smmu_domain pointer because we put it there, and converting it back > > > from a void pointer is exactly the same *at* the function call boundary > > > as immediately afterwards. > > > > Hmm, I am not quite following this. What do you suggest here? > > Oh, this is becoming quite the lesson in not reviewing patches in a hurry :( > > Apparently I managed to misread the diff and the horribly subtle > difference between "arm_smmu_tlb_inv_domain" and > "arm_smmu_atc_inv_domain", and think that arm_smmu_tlb_inv_context() was > already just dealing with the TLBI command and you were moving the > entire body into the new helper. Sorry about that. > > Still, the part about the comment remains true, and I think it goes to > show what a thoroughly horrible naming scheme it is to have "tlb_inv" > denote a function responsible for TLBI commands and "atc_inv" denote a > function responsible for ATC commands and "tlb_inv" denote a function > responsible for both TLBI and ATC commands... Well, "atc_inv" is quite clear I think. But the"tlb_inv" might not be, as you pointed out. So, we have: tlb_inv_range_asid: tlbi only (NH_VA/EL2_VA) // used by SVA too tlb_inv_range_domain: if (S1) tlb_inv_range_asid(); // NH_VA/EL2_VA else tlbi only (S2_IPA); atc(); tlb_inv_asid: tlbi (NH_ASID) // only used by tlb_inv_context() tlb_inv_context: if (S1) tlb_inv_asid(); // NH_ASID else tlbi only (S2_VMALL); atc(); Then, what this patch wants another non-atc: tlb_inv_asid: tlbi (NH_ASID) // only used by tlb_inv_domain() tlb_inv_domain: // new if (S1) tlb_inv_asid(); // NH_ASID else tlbi only (S2_VMALL); tlb_inv_context: tlb_inv_domain(); atc(); The problem of this is that it conflicts with the naming used in other tlb_inv_range_domain() that does an atc(). Perhaps, we could rename to the following patterns? tlb_inv_range_asid: // used by SVA too tlb_inv_range_domain: if (S1) return tlb_inv_range_asid(); else tlbi only (S2_IPA) tlb_inv_range_domain_with_atc: tlb_inv_range_domain(); atc(); # remove tlb_inv_asid() since it doesn't help much tlb_inv_domain: if (S1) tlbi only (NH_ASID) else tlbi only (S2_VMALL) tlb_inv_domain_with_atc: tlb_inv_domain(); atc(); tlb_inv_context: struct arm_smmu_domain *smmu_domain = (struct arm_smmu_domain *cookie); tlb_inv_domain_with_atc(smmu_domain); 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 9b0dc3505601..d6c647e1eb01 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -1854,12 +1854,24 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid, return arm_smmu_cmdq_batch_submit(smmu_domain->smmu, &cmds); } +static void arm_smmu_tlb_inv_domain(struct arm_smmu_domain *smmu_domain) +{ + struct arm_smmu_device *smmu = smmu_domain->smmu; + struct arm_smmu_cmdq_ent cmd; + + if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { + arm_smmu_tlb_inv_asid(smmu, smmu_domain->s1_cfg.cd.asid); + } else { + cmd.opcode = CMDQ_OP_TLBI_S12_VMALL; + cmd.tlbi.vmid = smmu_domain->s2_cfg.vmid; + arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd); + } +} + /* IO_PGTABLE API */ static void arm_smmu_tlb_inv_context(void *cookie) { struct arm_smmu_domain *smmu_domain = cookie; - struct arm_smmu_device *smmu = smmu_domain->smmu; - struct arm_smmu_cmdq_ent cmd; /* * NOTE: when io-pgtable is in non-strict mode, we may get here with @@ -1868,13 +1880,7 @@ static void arm_smmu_tlb_inv_context(void *cookie) * insertion to guarantee those are observed before the TLBI. Do be * careful, 007. */ - if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { - arm_smmu_tlb_inv_asid(smmu, smmu_domain->s1_cfg.cd.asid); - } else { - cmd.opcode = CMDQ_OP_TLBI_S12_VMALL; - cmd.tlbi.vmid = smmu_domain->s2_cfg.vmid; - arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd); - } + arm_smmu_tlb_inv_domain(smmu_domain); arm_smmu_atc_inv_domain(smmu_domain, 0, 0, 0); }
Move the part of per-asid or per-vmid invalidation command issuing into a new helper function, which will be used in the following change. Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 24 +++++++++++++-------- 1 file changed, 15 insertions(+), 9 deletions(-)