Re: [PATCH 00/19] coresight: Support for ETMv4.4 system instructions

[Suzuki K Poulose <suzuki.poulose@arm.com>]

Hi Mike,


First of all, thank you so much for your in depth review.
Please find my comments inline.

On 09/18/2020 04:33 PM, Mike Leach wrote:
> Hi Suzuki,
> 
> I've looked at the set and have only one real gripe - the
> implementation and timing of component detection on the sysreg path.
> I've summarised my thoughts here, but as the changes are found across
> multiple patches I may well have repeated myself a little in the
> individual places.
> 
> On Fri, 11 Sep 2020 at 09:41, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>>
>> CoreSight ETMv4.4 introduced system instructions for accessing
>> the ETM. This also implies that they may not be on the amba bus.
> 
> System instructions have always been an option - but we have never
> supported them up to now. In fact both memory access and system
> instructions can live side by side - but the driver really needs to
> choose just one!
> What did happen is that a PE that supports Arm Trace 8.4 mandates ETM
> 4.4, and ETM 4.4 mandates system instruction access for PEs with Arm
> Trace 8.4, and deprecates memory access.
> 
> But there is nothing to stop other variants having the  system
> instruction interface. So there is no need to describe this as a
> purely 4.4. onwards support - it will support any version of the ETM
> that has sysreg access.

Correct, I agree, and will change this.

> 
> The spec permits aarch32 / armv7 register access via CP14 - but I
> assume this is omitted deliberately & not intended to be supported at
> this time.

Yes, because the ETMv4 driver doesn't support arm32 bit at all. We could
definitely add this in the future. I will add it to the commit description.

> 
>> Right now all the CoreSight components are accessed via memory
>> map. Also, we have some common routines in coresight generic
>> code driver (e.g, CS_LOCK, claim/disclaim), which assume the
>> mmio. In order to preserve the generic algorithms at a single
>> place and to allow dynamic switch for ETMs, this series introduces
>> an abstraction layer for accessing a coresight device. It is
>> designed such that the mmio access are fast tracked (i.e, without
>> an indirect function call).
>>
>> This will also help us to get rid of the driver+attribute specific
>> sysfs show/store routines and replace them with a single routine
>> to access a given register offset (which can be embedded in the
>> dev_ext_attribute). This is not currently implemented in the series,
>> but can be achieved.
>>
>> Further we switch the generic routines to work with the abstraction.
>> With this in place, we refactor the etm4x code a bit to allow for
>> supporting the system instructions with very little new code. The
>> changes also switch to using the system instructions by default
>> even when we may have an MMIO.
>>
>> We use TRCDEVARCH for the detection of the ETM component, which
>> is a standard register as per CoreSight architecture, rather than
>> the etm specific id register TRCIDR3. This is for making sure
>> that we are able to detect the ETM via system instructions accurately,
>> when the the trace unit could be anything (etm or a custom trace unit).
>>
> 
> I'm assuming you mean TRCIDR1 here -- which in part, defines the etm
> architecture version. TRCIDR3 does something else entirely.

Yes, definitely, it is a mistake on my side, generated from staring at
both of them TRCIDR1 and TRCIDR3 (for the masks).

> Not sure I agree with this though - the driver is designed to match
> the ETM spec so there is no problem with using TRCIDR1 to spot
> functional variants according to ETM version,

Correct. But for a system instruction based trace unit, we can't trust
just the bindings and must use a CoreSight architected register to
do the basic detection. Also, I am planning to add support the Future
Architectures for the processor trace [0] with the ETM driver, which
mandates the use of TRCDEVARCH for the trace version. So, this is more
of

> 
> The etm4_init_arch_data() function is not about detecting the presence
> of an ETMv4 component, but about exploring the capabilities it has. We
> check 4 bits of the version as a sanity check, but at this point we
> should be pretty sure we are dealing with an ETM of some kind.
> 
> TRCDEVARCH is already used for detection in the AMBA matching code -
> assuming the table includes the optional CoreSIght UCI. I would
> imagine that similar detection needs to go on for instruction access -
> but once we have detected an ETM, then ETM architected registers are
> sufficient. If the device is not an ETM then it should be detected and
> rejected early - and the bindings examined to determine why this
> driver was attached!

I agree with the fact that we should check the device for a supported
type at the earliest and must not trust the bindings. With the AMBA
based devices we have the early check as mentioned above via the PIDs
and the UCI (where available). But where the UCI is not listed, these
will be caught by the additional checks on the TRCIDR1 fields. e.g,
CTI could have the same PID as an ETM4 and without the UCI field,
the driver could assume that an ETM is CTI if the firmware was
incorrect.

> 
> The act of adding in a check against TRCDEVARCH as part of the
> etm4_init_arch_data() function adds new and hidden checks to AMBA
> devices where it was sufficient to have an entry in the probe match
> table before. Most recent additions include the UCI matching, but
> older ones don't. I am concerned that this changes may trip up older
> existing implementations which for some reason may not have
> TRCDEVCARCH, or have set it to not present.

For the records, ETMv4.0 revision A, says, the PRESENT bit is always
Read As One (RAO). So, if they don't implement it or have set to 0,
that means that they are broken. But, we could gracefully handle it
if the PRESENT bit is 0 and fold back to TRCIDR1.

> 
> For this reason, I beleive that the TRCDEVARCH check for the sys reg
> access should occur on the sysreg specific probe - balancing what
> happens on the AMBA side.  That way the common code remains common.

To make the current situation clear, for those who have not looked
at the series, here is the summary :

1) AMBA driver checks the PIDs to match a device to known ETMv4.
    Note that CTIs could share the same PIDs and thus we added additional
    check on the UCI (which is TRCDEVARCH) field for some of the ETMs.

2) The ETM4 driver assumes that the component is ETMv4 and calls
    etmv4_init_arch_data() and probes the ETM4 for features, filling
    in drvdata, including the TRCIDR1. Please note that, at this point
    there is no guarantee that the unit is indeed ETMv4, if the UCI
    check (TRCDEVARCH) has not been performed. So, we are possibly
    treading into wild land here (at least on bring up).

3) The etm4_probe() confirms that the ETM4 architecture is supported
    by checking the TRCIDR1 fields (stored in drvdata->arch from step (2)).
    This check is important (at least for bring up), because if the
    UCI check is not added for the component, a CTI could be mistaken
    for an ETM with AMBA devices.

Fact : TRCDEVARCH must be implemented and represent that the component is
ETMv4 (this is the basis of UCI check) since ETMv4.0 specification.


For the system instructions based devices, we have :
1) Device tree compatible to advertise the presence of a trace unit on
    a CPU. (no PID checks, this is good, because you don't have to
    add an entry for a new CPU to be supported upstream, as long as
    it is compliant with the ETMv4).

2) To confirm that the CPU tracing unit is ETMv4 compatible, we need
    to use CoreSight architected register, TRCDEVARCH (the same as UCI).
    This is because TRCIDR1 may not be what is expected if the Trace unit
    is not ETMv4, since the encoding is ETM specific. And this must be
    performed on the host CPU (just like etm4_init_arch_data).

With this series:

   * AMBA devices pass through the PID check as usual. But the sysreg
     devices jumps straight to etm4_init_arch_data() via common etm4_probe.

   * etm4_init_arch_data() will verify that the component is ETMv4
     by verifying the TRCDEVARCH for all ETMs (both AMBA and system
     instructions), before poking the features. This will avoid having
     to do another round of smp_func_call() in etm4 sysreg probe code.
      - Note: As per Mike's suggestion, this can be further relaxed to check
        TRCIDR1 for AMBA devices, iff TRCDEVARCH is marked absent for
        supporting any wild broken implementations out there (I prefer to add a
        pr_warn_once() for such cases, so that we know such units).


> Further the setup of the CSA for the device can happen immediately in
> the common etm_probe() function, based on *base being NULL or not,

For now, yes. But with the additional changes for supporting [0], this
may not. As we need to really see if we have an ETMv4.x or a future
unit which has slightly different register list.

> rather than as a side effect of the etm4_init_arch_data() call.

[0] https://developer.arm.com/docs/ddi0601/latest , See TRCIDR1

Kind regards
Suzuki




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel