Good morning Al,
Suzuki sent me this patch [1] that has the effect of printing the following [2] in the kernel boot log at initialisation time. I think it is quite useful since one can correlate CPU information and ETM engine without having to go back to the DT or having to look at all the tracers in sysFS. On the flip side I'm a little weary (since peripheral numbers are implementation specific) that at one point we may end up with say, a Cortex-A53 with different peripheral IDs or another CPU type advertising a peripheral ID that is already taken.
As I said I think this is a good idea but before applying a change that will be very public I'd like to make sure there aren't any ramification we haven't thought about. Please let me know what you think.
Best regards, Mathieu
[1]. https://www.spinics.net/lists/arm-kernel/msg653904.html [2]. https://pastebin.com/gbiLaJHJ
Suzuki sent me this patch [1] that has the effect of printing the following [2] in the kernel boot log at initialisation time. I think it is quite useful since one can correlate CPU information and ETM engine without having to go back to the DT or having to look at all the tracers in sysFS. On the flip side I'm a little weary (since peripheral numbers are implementation specific) that at one point we may end up with say, a Cortex-A53 with different peripheral IDs or another CPU type advertising a peripheral ID that is already taken.
As I said I think this is a good idea but before applying a change that will be very public I'd like to make sure there aren't any ramification we haven't thought about. Please let me know what you think.
From the patch, it looks like it was already using peripheral ids (e.g. for A53) and is just adding the CPU names. This should be safe as the ids should be unique across CPU products and fixed for any given CPU product.
Having said that, it might be wrong to have this table at all. It's a maintenance overhead to have to keep adding new CPU products to this table (it's a long way from being complete even now, and of course more products will appear). The ETM drivers should be working off the ETM feature identification registers and/or device tree as far as possible, so that a given kernel will automatically work for future CPU products. So whatever we're trying to do by looking at the CPU-specific ids here, it's not the right thing to do. If you find CPU #n has an ETM with features XYZ and want to print a message, you should be able to find CPU #n's CPU type some other way.
I'd think the only reason the CoreSight drivers would need to check for specific CPUs is to work around hardware errata specific to those CPUs - and in that case you would likely want to test the CPU release/patch/stepping level in more detail.
Al
Best regards, Mathieu
[1]. https://www.spinics.net/lists/arm-kernel/msg653904.html [2]. https://pastebin.com/gbiLaJHJ
Al,
On 23/05/18 09:54, Al Grant wrote:
Suzuki sent me this patch [1] that has the effect of printing the following [2] in the kernel boot log at initialisation time. I think it is quite useful since one can correlate CPU information and ETM engine without having to go back to the DT or having to look at all the tracers in sysFS. On the flip side I'm a little weary (since peripheral numbers are implementation specific) that at one point we may end up with say, a Cortex-A53 with different peripheral IDs or another CPU type advertising a peripheral ID that is already taken.
As I said I think this is a good idea but before applying a change that will be very public I'd like to make sure there aren't any ramification we haven't thought about. Please let me know what you think.
From the patch, it looks like it was already using peripheral ids (e.g. for A53) and is just adding the CPU names. This should be safe as the ids should be unique across CPU products and fixed for any given CPU product.
Now when I think of it, we may want to avoid printing the CPU names there. There could be cores with different MIDRs (partner codes) but effectively it is same as one of the Arm cores (we have had this situation with erratum work arounds). I could strip off the names from the table. If someone wants to map it, they could always lookup the MIDR from /proc/cpuinfo.
Having said that, it might be wrong to have this table at all. It's a maintenance overhead to have to keep adding new CPU products to this table (it's a long way from being complete even now, and of course more products will appear). The ETM drivers should be working off the ETM feature identification registers and/or device tree as far as possible, so that a given kernel will automatically work for future CPU products. So whatever we're trying to do by looking at the CPU-specific ids here, it's not the right thing to do. If you find CPU #n has an ETM with features XYZ and want to print a message, you should be able to find CPU #n's CPU type some other way.
I agree. The problem here is that the ETM devices are treated as AMBA devices and we rely on the AMBA bus probing (which in turn looks at the PIDs) to match a given device to ETM device. May be we need to get away from the AMBA frame work and come up with something like the PMU probing, where we probe for the device on each CPU using the ETM feature registers and detect the devices (of course, based on the DT)
I'd think the only reason the CoreSight drivers would need to check for specific CPUs is to work around hardware errata specific to those CPUs - and in that case you would likely want to test the CPU release/patch/stepping level in more detail.
We have a generic infrastructure in the arm64 kernel for detecting Errata, which could be leveraged to manage the coresight errata work arounds for ETMs. So we don't duplicate something just for us.
Cheers Suzuki
I agree. The problem here is that the ETM devices are treated as AMBA devices and we rely on the AMBA bus probing (which in turn looks at the PIDs) to match a given device to ETM device.
Even if you're probing the bus you shouldn't need to look at the implementation-specific part of the id. There are generic id registers that let you progressively discover all the ETM features, all the way from generic device class right through to the fact that it's an ETM 4.1 with 6 address comparators etc. You shouldn't ever need to match against a table of implementation-specific ids.
Al
May be we need to get away from the AMBA
frame work and come up with something like the PMU probing, where we probe for the device on each CPU using the ETM feature registers and detect the devices (of course, based on the DT)
I'd think the only reason the CoreSight drivers would need to check for specific CPUs is to work around hardware errata specific to those CPUs - and in that case you would likely want to test the CPU
release/patch/stepping level in more detail.
We have a generic infrastructure in the arm64 kernel for detecting Errata, which could be leveraged to manage the coresight errata work arounds for ETMs. So we don't duplicate something just for us.
Cheers Suzuki
On 23/05/18 10:21, Al Grant wrote:
I agree. The problem here is that the ETM devices are treated as AMBA devices and we rely on the AMBA bus probing (which in turn looks at the PIDs) to match a given device to ETM device.
Even if you're probing the bus you shouldn't need to look at the implementation-specific part of the id. There are generic id registers that let you progressively discover all the ETM features, all the way from generic device class right through to the fact
Al,
I think there is a bit of confusion here. The ETM driver doesn't look up the PID of the ETM for enumerating the features. Those table entries are consumed by the *amba bus* driver, which maps a given AMBA device to a driver (in this case ETM) based on the ID table passed on the amba driver. The ETM driver only uses the feature registers to identify the ETM features and it doesn't even care about the PID, once it is given a device.
The proposed patch only prints the processor name for a given ETM. It doesn't associate magic preconfigured features with an ID.
that it's an ETM 4.1 with 6 address comparators etc. You shouldn't ever need to match against a table of implementation-specific ids.
I hope that is clear. So, coming back to the forward compatibility case, a new CPU must be listed in the amba ID table for the AMBA to invoke the ETM driver for a given ETM. Thats why I mentioned about moving away from the AMBA framework and having the CPU accessible registers of ETM to detect an ETM device and then go on to use it.
Suzuki
Al
May be we need to get away from the AMBA
frame work and come up with something like the PMU probing, where we probe for the device on each CPU using the ETM feature registers and detect the devices (of course, based on the DT)
I'd think the only reason the CoreSight drivers would need to check for specific CPUs is to work around hardware errata specific to those CPUs - and in that case you would likely want to test the CPU
release/patch/stepping level in more detail.
We have a generic infrastructure in the arm64 kernel for detecting Errata, which could be leveraged to manage the coresight errata work arounds for ETMs. So we don't duplicate something just for us.
Cheers Suzuki
On 23 May 2018 at 03:06, Suzuki K Poulose Suzuki.Poulose@arm.com wrote:
Al,
On 23/05/18 09:54, Al Grant wrote:
Suzuki sent me this patch [1] that has the effect of printing the following [2] in the kernel boot log at initialisation time. I think it is quite useful since one can correlate CPU information and ETM engine without having to go back to the DT or having to look at all the tracers in sysFS. On the flip side I'm a little weary (since peripheral numbers are implementation specific) that at one point we may end up with say, a Cortex-A53 with different peripheral IDs or another CPU type advertising a peripheral ID that is already taken.
As I said I think this is a good idea but before applying a change that will be very public I'd like to make sure there aren't any ramification we haven't thought about. Please let me know what you think.
From the patch, it looks like it was already using peripheral ids (e.g. for A53) and is just adding the CPU names. This should be safe as the ids should be unique across CPU products and fixed for any given CPU product.
Now when I think of it, we may want to avoid printing the CPU names there. There could be cores with different MIDRs (partner codes) but effectively it is same as one of the Arm cores (we have had this situation with erratum work arounds). I could strip off the names from the table. If someone wants to map it, they could always lookup the MIDR from /proc/cpuinfo.
That's exactly why I wanted to have a broader conversation on the topic. Once people start relying on output messages it is really hard to change them.
Thanks, Mathieu
Having said that, it might be wrong to have this table at all. It's a maintenance overhead to have to keep adding new CPU products to this table (it's a long way from being complete even now, and of course more products will appear). The ETM drivers should be working off the ETM feature identification registers and/or device tree as far as possible, so that a given kernel will automatically work for future CPU products. So whatever we're trying to do by looking at the CPU-specific ids here, it's not the right thing to do. If you find CPU #n has an ETM with features XYZ and want to print a message, you should be able to find CPU #n's CPU type some other way.
I agree. The problem here is that the ETM devices are treated as AMBA devices and we rely on the AMBA bus probing (which in turn looks at the PIDs) to match a given device to ETM device. May be we need to get away from the AMBA frame work and come up with something like the PMU probing, where we probe for the device on each CPU using the ETM feature registers and detect the devices (of course, based on the DT)
I'd think the only reason the CoreSight drivers would need to check for specific CPUs is to work around hardware errata specific to those CPUs - and in that case you would likely want to test the CPU release/patch/stepping level in more detail.
We have a generic infrastructure in the arm64 kernel for detecting Errata, which could be leveraged to manage the coresight errata work arounds for ETMs. So we don't duplicate something just for us.
Cheers Suzuki
-
Al Grant -
Mathieu Poirier -
Suzuki K Poulose