On Fri, 22 Sep 2023, Jithu Joseph wrote:
> Width of chunk related bitfields is ACTIVATE_SCAN and SCAN_STATUS MSRs
> are different in newer IFS generation compared to gen0.
>
> Make changes to scan test flow such that MSRs are populated
> appropriately based on the generation supported by hardware.
>
> Account for the 8/16 bit MSR bitfield width differences between gen0 and
> newer generations for the scan test trace event too.
>
> Signed-off-by: Jithu Joseph <jithu.joseph@xxxxxxxxx>
> Reviewed-by: Tony Luck <tony.luck@xxxxxxxxx>
> Tested-by: Pengfei Xu <pengfei.xu@xxxxxxxxx>
> ---
> drivers/platform/x86/intel/ifs/ifs.h | 28 +++++++++++++++++++-----
> include/trace/events/intel_ifs.h | 16 +++++++-------
> drivers/platform/x86/intel/ifs/runtest.c | 26 ++++++++++++++++------
> 3 files changed, 49 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index 43281d456a09..cd213b89d278 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -199,9 +199,17 @@ union ifs_chunks_auth_status_gen2 {
> union ifs_scan {
> u64 data;
> struct {
> - u32 start :8;
> - u32 stop :8;
> - u32 rsvd :16;
> + union {
> + struct {
> + u8 start;
> + u8 stop;
> + u16 rsvd;
> + } gen0;
> + struct {
> + u16 start;
> + u16 stop;
> + } gen2;
> + };
> u32 delay :31;
> u32 sigmce :1;
> };
> @@ -211,9 +219,17 @@ union ifs_scan {
> union ifs_status {
> u64 data;
> struct {
> - u32 chunk_num :8;
> - u32 chunk_stop_index :8;
> - u32 rsvd1 :16;
> + union {
> + struct {
> + u8 chunk_num;
> + u8 chunk_stop_index;
> + u16 rsvd1;
> + } gen0;
> + struct {
> + u16 chunk_num;
> + u16 chunk_stop_index;
> + } gen2;
> + };
> u32 error_code :8;
> u32 rsvd2 :22;
> u32 control_error :1;
> diff --git a/include/trace/events/intel_ifs.h b/include/trace/events/intel_ifs.h
> index d7353024016c..af0af3f1d9b7 100644
> --- a/include/trace/events/intel_ifs.h
> +++ b/include/trace/events/intel_ifs.h
> @@ -10,25 +10,25 @@
>
> TRACE_EVENT(ifs_status,
>
> - TP_PROTO(int cpu, union ifs_scan activate, union ifs_status status),
> + TP_PROTO(int cpu, int start, int stop, u64 status),
>
> - TP_ARGS(cpu, activate, status),
> + TP_ARGS(cpu, start, stop, status),
>
> TP_STRUCT__entry(
> __field( u64, status )
> __field( int, cpu )
> - __field( u8, start )
> - __field( u8, stop )
> + __field( u16, start )
> + __field( u16, stop )
> ),
>
> TP_fast_assign(
> __entry->cpu = cpu;
> - __entry->start = activate.start;
> - __entry->stop = activate.stop;
> - __entry->status = status.data;
> + __entry->start = start;
> + __entry->stop = stop;
> + __entry->status = status;
> ),
>
> - TP_printk("cpu: %d, start: %.2x, stop: %.2x, status: %llx",
> + TP_printk("cpu: %d, start: %.4x, stop: %.4x, status: %.16llx",
> __entry->cpu,
> __entry->start,
> __entry->stop,
> diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c
> index 1061eb7ec399..94d486e5d263 100644
> --- a/drivers/platform/x86/intel/ifs/runtest.c
> +++ b/drivers/platform/x86/intel/ifs/runtest.c
> @@ -171,21 +171,30 @@ static void ifs_test_core(int cpu, struct device *dev)
> union ifs_status status;
> unsigned long timeout;
> struct ifs_data *ifsd;
> + int to_start, to_stop;
> + int status_chunk;
> u64 msrvals[2];
> int retries;
>
> ifsd = ifs_get_data(dev);
>
> - activate.rsvd = 0;
> activate.delay = IFS_THREAD_WAIT;
> activate.sigmce = 0;
> - activate.start = 0;
> - activate.stop = ifsd->valid_chunks - 1;
> + to_start = 0;
> + to_stop = ifsd->valid_chunks - 1;
> +
> + if (ifsd->generation) {
> + activate.gen2.start = to_start;
> + activate.gen2.stop = to_stop;
> + } else {
> + activate.gen0.start = to_start;
> + activate.gen0.stop = to_stop;
> + }
Is it okay to not do activate.gen0.rsvd = 0 anymore? If you know it is, it
would be nice to record that fact into the changelog so that it can be
found in the history.
>
> timeout = jiffies + HZ / 2;
> retries = MAX_IFS_RETRIES;
>
> - while (activate.start <= activate.stop) {
> + while (to_start <= to_stop) {
> if (time_after(jiffies, timeout)) {
> status.error_code = IFS_SW_TIMEOUT;
> break;
> @@ -196,13 +205,14 @@ static void ifs_test_core(int cpu, struct device *dev)
>
> status.data = msrvals[1];
>
> - trace_ifs_status(cpu, activate, status);
> + trace_ifs_status(cpu, to_start, to_stop, status.data);
>
> /* Some cases can be retried, give up for others */
> if (!can_restart(status))
> break;
>
> - if (status.chunk_num == activate.start) {
> + status_chunk = ifsd->generation ? status.gen2.chunk_num : status.gen0.chunk_num;
> + if (status_chunk == to_start) {
> /* Check for forward progress */
> if (--retries == 0) {
> if (status.error_code == IFS_NO_ERROR)
> @@ -211,7 +221,9 @@ static void ifs_test_core(int cpu, struct device *dev)
> }
> } else {
> retries = MAX_IFS_RETRIES;
> - activate.start = status.chunk_num;
> + ifsd->generation ? (activate.gen2.start = status_chunk) :
> + (activate.gen0.start = status_chunk);
The alignment of the second line is still not correct but now I notice how
the left-hand side is hidden within those expressions. Just do a normal if
instead so that it is simpler to understand, please.
> + to_start = status_chunk;
> }
> }
>
>
--
i.
