| Message ID | 1569854031-237636-1-git-send-email-john.garry@huawei.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | SMMUv3 PMCG IMP DEF event support | expand |
Hi John, On 30/09/2019 15:33, John Garry wrote: > This patchset adds IMP DEF event support for the SMMUv3 PMCG. > > It is marked as an RFC as the method to identify the PMCG implementation > may be a quite disliked. And, in general, the series is somewhat > incomplete. > > So the background is that the PMCG supports IMP DEF events, yet we have no > method to identify the PMCG to know the IMP DEF events. > > A method for identifying the PMCG implementation could be using > PMDEVARCH, but we cannot rely on this being set properly, as whether this > is implemented is not defined in SMMUv3 spec. > > Another method would be perf event aliasing, but this method of event > matching is based on CPU id, which would not guarantee same > uniqueness as PMCG implementation. > > Yet another method could be to continue using ACPI OEM ID in the IORT > code, but this does not scale. And it is not suitable if we ever add DT > support to the PMCG driver. > > The method used in this series is based on matching on the parent SMMUv3 > IIDR. We store this IIDR contents in the arm smmu structure as the first > element, which means that we don't have to expose SMMU APIs - this is > the part which may be disliked. > > The final two patches switch the pre-existing PMCG model identification > from ACPI OEM ID to the same parent SMMUv3 IIDR matching. > > For now, we only consider SMMUv3' nodes being the associated node for > PMCG. Two significant concerns right off the bat: - It seems more common than not for silicon designers to fail to implement IIDR correctly, so it's only a matter of time before inevitably needing to bring back some firmware-level identifier abstraction (if not already - does Hi161x have PMCGs?) - This seems like a step in entirely the wrong direction for supporting PMCGs that reference a Named Component or Root Complex. Interpreting the Node Reference is definitely a welcome improvement over matching table headers, but absent a truly compelling argument to the contrary, I'd rather retain the "PMCG model" abstraction in between that and the driver itself (especially since those can trivially be hung off compatibles once it comes to DT support). Thanks, Robin. > > John Garry (6): > ACPI/IORT: Set PMCG device parent > iommu/arm-smmu-v3: Record IIDR in arm_smmu_device structure > perf/smmuv3: Retrieve parent SMMUv3 IIDR > perf/smmuv3: Support HiSilicon hip08 (hi1620) IMP DEF events > perf/smmuv3: Match implementation options based on parent SMMU IIDR > ACPI/IORT: Drop code to set the PMCG software-defined model > > drivers/acpi/arm64/iort.c | 69 ++++++++++++++-------------- > drivers/iommu/arm-smmu-v3.c | 5 +++ > drivers/perf/arm_smmuv3_pmu.c | 84 ++++++++++++++++++++++++++++++----- > include/linux/acpi_iort.h | 8 ---- > 4 files changed, 112 insertions(+), 54 deletions(-) >
On 15/10/2019 19:00, Robin Murphy wrote: > Hi John, > > On 30/09/2019 15:33, John Garry wrote: >> This patchset adds IMP DEF event support for the SMMUv3 PMCG. >> >> It is marked as an RFC as the method to identify the PMCG implementation >> may be a quite disliked. And, in general, the series is somewhat >> incomplete. >> >> So the background is that the PMCG supports IMP DEF events, yet we >> have no >> method to identify the PMCG to know the IMP DEF events. >> >> A method for identifying the PMCG implementation could be using >> PMDEVARCH, but we cannot rely on this being set properly, as whether this >> is implemented is not defined in SMMUv3 spec. >> >> Another method would be perf event aliasing, but this method of event >> matching is based on CPU id, which would not guarantee same >> uniqueness as PMCG implementation. >> >> Yet another method could be to continue using ACPI OEM ID in the IORT >> code, but this does not scale. And it is not suitable if we ever add DT >> support to the PMCG driver. >> >> The method used in this series is based on matching on the parent SMMUv3 >> IIDR. We store this IIDR contents in the arm smmu structure as the first >> element, which means that we don't have to expose SMMU APIs - this is >> the part which may be disliked. >> >> The final two patches switch the pre-existing PMCG model identification >> from ACPI OEM ID to the same parent SMMUv3 IIDR matching. >> >> For now, we only consider SMMUv3' nodes being the associated node for >> PMCG. > Hi Robin, > Two significant concerns right off the bat: > > - It seems more common than not for silicon designers to fail to > implement IIDR correctly, so it's only a matter of time before > inevitably needing to bring back some firmware-level identifier > abstraction (if not already - does Hi161x have PMCGs?) Maybe there's a way that we can switch to this method, and leave the door open for an easy way to support firmware-level identifier again, if ever needed. I'm not too pushed - this was secondary to just allowing the PMCG driver know the associated SMMU model. And, no, hi161x does not have any PMCGs. > > - This seems like a step in entirely the wrong direction for supporting >. So to support PMCGs that reference a Named Component or Root Complex, I thought that the IORT parsing code would have to do some secondary lookup to the associated SMMU, through the Named Component or Root Complex node. What was your idea here? Note: I do acknowledge that an overall issue is that we assume all PMCG IMP DEF events are same for a given SMMU model. > > Interpreting the Node Reference is definitely a welcome improvement over > matching table headers, but absent a truly compelling argument to the > contrary, I'd rather retain the "PMCG model" abstraction in between that > and the driver itself (especially since those can trivially be hung off > compatibles once it comes to DT support). For DT, I would assume that we just use compatible strings would allow us to identify the PMCG model. On a related matter, is there still a need to deal with scenarios of the PMCG being located within the SMMU register map? As you may remember, we did have this issue but relocated the PMCG to outside the SMMU register map in a later chip rev. Cheers, John > > Thanks, > Robin. > >> >> John Garry (6): >> ACPI/IORT: Set PMCG device parent >> iommu/arm-smmu-v3: Record IIDR in arm_smmu_device structure >> perf/smmuv3: Retrieve parent SMMUv3 IIDR >> perf/smmuv3: Support HiSilicon hip08 (hi1620) IMP DEF events >> perf/smmuv3: Match implementation options based on parent SMMU IIDR >> ACPI/IORT: Drop code to set the PMCG software-defined model >> >> drivers/acpi/arm64/iort.c | 69 ++++++++++++++-------------- >> drivers/iommu/arm-smmu-v3.c | 5 +++ >> drivers/perf/arm_smmuv3_pmu.c | 84 ++++++++++++++++++++++++++++++----- >> include/linux/acpi_iort.h | 8 ---- >> 4 files changed, 112 insertions(+), 54 deletions(-) >> > > . >
On 2019-10-16 9:47 am, John Garry wrote: > On 15/10/2019 19:00, Robin Murphy wrote: >> Hi John, >> >> On 30/09/2019 15:33, John Garry wrote: >>> This patchset adds IMP DEF event support for the SMMUv3 PMCG. >>> >>> It is marked as an RFC as the method to identify the PMCG implementation >>> may be a quite disliked. And, in general, the series is somewhat >>> incomplete. >>> >>> So the background is that the PMCG supports IMP DEF events, yet we >>> have no >>> method to identify the PMCG to know the IMP DEF events. >>> >>> A method for identifying the PMCG implementation could be using >>> PMDEVARCH, but we cannot rely on this being set properly, as whether >>> this >>> is implemented is not defined in SMMUv3 spec. >>> >>> Another method would be perf event aliasing, but this method of event >>> matching is based on CPU id, which would not guarantee same >>> uniqueness as PMCG implementation. >>> >>> Yet another method could be to continue using ACPI OEM ID in the IORT >>> code, but this does not scale. And it is not suitable if we ever add DT >>> support to the PMCG driver. >>> >>> The method used in this series is based on matching on the parent SMMUv3 >>> IIDR. We store this IIDR contents in the arm smmu structure as the first >>> element, which means that we don't have to expose SMMU APIs - this is >>> the part which may be disliked. >>> >>> The final two patches switch the pre-existing PMCG model identification >>> from ACPI OEM ID to the same parent SMMUv3 IIDR matching. >>> >>> For now, we only consider SMMUv3' nodes being the associated node for >>> PMCG. >> > > Hi Robin, > >> Two significant concerns right off the bat: >> >> - It seems more common than not for silicon designers to fail to >> implement IIDR correctly, so it's only a matter of time before >> inevitably needing to bring back some firmware-level identifier >> abstraction (if not already - does Hi161x have PMCGs?) > > Maybe there's a way that we can switch to this method, and leave the > door open for an easy way to support firmware-level identifier again, if > ever needed. I'm not too pushed - this was secondary to just allowing > the PMCG driver know the associated SMMU model. But that's the part I'm not buying - there's no clear advantage to pushing that complexity down into the PMCG driver, vs. leaving the IORT code responsible for translating an SMMU model into a PMCG model, yet the aforementioned disadvantages jump out right away. > And, no, hi161x does not have any PMCGs. Hooray, I guess :) >> >> - This seems like a step in entirely the wrong direction for supporting >> . > > So to support PMCGs that reference a Named Component or Root Complex, I > thought that the IORT parsing code would have to do some secondary > lookup to the associated SMMU, through the Named Component or Root > Complex node. > > What was your idea here? The associated SMMU has no relevance in that context - the reason for the Node Reference to point to a non-SMMU node is for devices that implement their own embedded TLB (e.g. AMBA DTI masters) and expose a standard PMCG interface to monitor it. It isn't reasonable to expect any old PCIe controller or on-chip-accelerator driver to expose a fake SMMU IIDR just to keep some other driver happy. > Note: I do acknowledge that an overall issue is that we assume all PMCG > IMP DEF events are same for a given SMMU model. That assumption does technically fail already - I know MMU-600 has different IMP-DEF events for its TCU and TBUs, however as long as we can get as far as "this is some part of an MMU-600" the driver should be able to figure out the rest (annoyingly it looks like both PMCG types expose the same PMCG_ID_REGS information, but they should be distinguishable by PMCG_CEIDn). >> Interpreting the Node Reference is definitely a welcome improvement over >> matching table headers, but absent a truly compelling argument to the >> contrary, I'd rather retain the "PMCG model" abstraction in between that >> and the driver itself (especially since those can trivially be hung off >> compatibles once it comes to DT support). > > For DT, I would assume that we just use compatible strings would allow > us to identify the PMCG model. Right, that was largely my point - DT probing can start with a PMCG model, so it's a lot more logical for ACPI probing to do the same, with the actual PMCG model determination hidden away in the ACPI code. That's the basis of the current design. I have been nagging the architects that PMCGs not having their own IIDR is an unwelcome hole in the spec, so hopefully this might get a bit easier some day. > On a related matter, is there still a need to deal with scenarios of the > PMCG being located within the SMMU register map? As you may remember, we > did have this issue but relocated the PMCG to outside the SMMU register > map in a later chip rev. MMU-600 has its TCU PMCG page 0 in the middle of its SMMU page 0 space, but given that it's an Arm IP, I expect that when the heat gets turned up for making it work, it's most likely to be under me ;) Robin.
Hi Robin, >>> Two significant concerns right off the bat: >>> >>> - It seems more common than not for silicon designers to fail to >>> implement IIDR correctly, so it's only a matter of time before >>> inevitably needing to bring back some firmware-level identifier >>> abstraction (if not already - does Hi161x have PMCGs?) >> >> Maybe there's a way that we can switch to this method, and leave the >> door open for an easy way to support firmware-level identifier again, >> if ever needed. I'm not too pushed - this was secondary to just >> allowing the PMCG driver know the associated SMMU model. > > But that's the part I'm not buying - there's no clear advantage to > pushing that complexity down into the PMCG driver, vs. leaving the IORT > code responsible for translating an SMMU model into a PMCG model, yet > the aforementioned disadvantages jump out right away. > One advantage is that the next piece of quirky hw with a properly implemented IIDR does not require a new IORT model. And today, this handling is only for hi1620, and since we can use hi1620 IIDR to id it, then it seems good to remove code outside the PMCG driver specifically to handle it. But if you think it's going to be needed again, then it makes sense not to remove it. >> And, no, hi161x does not have any PMCGs. > > Hooray, I guess :) > >>> >>> - This seems like a step in entirely the wrong direction for supporting >>> . >> >> So to support PMCGs that reference a Named Component or Root Complex, >> I thought that the IORT parsing code would have to do some secondary >> lookup to the associated SMMU, through the Named Component or Root >> Complex node. >> >> What was your idea here? > > The associated SMMU has no relevance in that context - the reason for > the Node Reference to point to a non-SMMU node is for devices that > implement their own embedded TLB (e.g. AMBA DTI masters) and expose a > standard PMCG interface to monitor it. It isn't reasonable to expect any > old PCIe controller or on-chip-accelerator driver to expose a fake SMMU > IIDR just to keep some other driver happy. But won't there still be an SMMU associated with the AMBA DTI masters, in your example? It's this SMMU which the PMCG driver would reference as the "parent" device, and the IORT parsing would need to do the lookup for this reference. But then, this becomes something that the DT parsing would need to handle also. > >> Note: I do acknowledge that an overall issue is that we assume all >> PMCG IMP DEF events are same for a given SMMU model. > > That assumption does technically fail already - I know MMU-600 has > different IMP-DEF events for its TCU and TBUs, however as long as we can > get as far as "this is some part of an MMU-600" the driver should be > able to figure out the rest (annoyingly it looks like both PMCG types > expose the same PMCG_ID_REGS information, but they should be > distinguishable by PMCG_CEIDn). JFYI, PMCG_CEIDn contents for hi1620 are all zero, apart from PMDEVARCH and PMDEVTYPE, which are same as arm implementation according to the spec - sigh... > >>> Interpreting the Node Reference is definitely a welcome improvement over >>> matching table headers, but absent a truly compelling argument to the >>> contrary, I'd rather retain the "PMCG model" abstraction in between that >>> and the driver itself (especially since those can trivially be hung off >>> compatibles once it comes to DT support). >> >> For DT, I would assume that we just use compatible strings would allow >> us to identify the PMCG model. > > Right, that was largely my point - DT probing can start with a PMCG > model, so it's a lot more logical for ACPI probing to do the same, with > the actual PMCG model determination hidden away in the ACPI code. That's > the basis of the current design. > > I have been nagging the architects that PMCGs not having their own IIDR > is an unwelcome hole in the spec, so hopefully this might get a bit > easier some day. For sure. The spec reads that the PMCGs may be "independently-designed", hence no general id method. I don't get this. > >> On a related matter, is there still a need to deal with scenarios of >> the PMCG being located within the SMMU register map? As you may >> remember, we did have this issue but relocated the PMCG to outside the >> SMMU register map in a later chip rev. > > MMU-600 has its TCU PMCG page 0 in the middle of its SMMU page 0 space, > but given that it's an Arm IP, I expect that when the heat gets turned > up for making it work, it's most likely to be under me ;) OK, so this is another reason why I thought that having a reference to the SMMU device could be useful in terms of solving that problem. > > Robin. > > . Thanks again, John >