| Message ID | 20230922232606.1928026-5-jithu.joseph@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | IFS support for GNR and SRF | expand |
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@intel.com> > Reviewed-by: Tony Luck <tony.luck@intel.com> > Tested-by: Pengfei Xu <pengfei.xu@intel.com> > --- > 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; > } > } > >
On 9/25/2023 8:39 AM, Ilpo Järvinen wrote: > On Fri, 22 Sep 2023, Jithu Joseph wrote: > ... >> >> - 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. I did test on a gen0 to check if there is a problem due to this (and it seemed fine). I will make a note in changelog as you suggest > >> >> 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. In v1 the second line was kept 1 space to the right of previous line. In v2 I kept them at the same indent, since your original comment was Misaliged. I will change these two lines to if (ifsd->generation) activate.gen2.start = status_chunk; else activate.gen0.start = status_chunk Jithu
On Mon, 25 Sep 2023, Joseph, Jithu wrote: > On 9/25/2023 8:39 AM, Ilpo Järvinen wrote: > > On Fri, 22 Sep 2023, Jithu Joseph wrote: > > > > ... > > >> > >> - 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. > > I did test on a gen0 to check if there is a problem due to this (and it seemed fine). > I will make a note in changelog as you suggest Actually, I realized activate is a variable in stack and those bits are uninitilized without that assignment so don't remove it.
On 9/26/2023 3:20 AM, Ilpo Järvinen wrote: > On Mon, 25 Sep 2023, Joseph, Jithu wrote: >> On 9/25/2023 8:39 AM, Ilpo Järvinen wrote: >>> On Fri, 22 Sep 2023, Jithu Joseph wrote: >>> >> >> ... >> >>>> >>>> - 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. >> >> I did test on a gen0 to check if there is a problem due to this (and it seemed fine). >> I will make a note in changelog as you suggest > > Actually, I realized activate is a variable in stack and those bits are > uninitilized without that assignment so don't remove it. > Ok, I will post a v3 with all the suggested changes in a few days ... Thanks for the review Jithu
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; + } 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); + to_start = status_chunk; } }