Re: [LTP] [PATCH v2 1/1] ptrace07: should not use a hard-coded xstate size and use CPUID specified instead

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Pengfei Xu <pengfei.xu@intel.com> writes:

> Hi Richard,
>
> On 2022-10-17 at 14:55:29 +0100, Richard Palethorpe wrote:
>> Hello,
>> 
>> Pengfei Xu <pengfei.xu@intel.com> writes:
>> 
>> > Hi,
>> >
>> > This patch fixes ptrace07 spurious failures when the platform xstate maxium
>> > size is bigger than 4096bytes(512*8 bytes).
>> >
>> > Thanks for comments!
>> 
>> This patch causes the test to fail on my Xeon workstation. The problem
>> seems to be the cpuid function which just fills the args with zeros.
>   Sorry, I didn't meet this issue,  I think I should use a new cpuid function.
>   Thanks for the report!
>
>> 
>> >
>> > BR.
>> >
>> > On 2022-09-29 at 10:30:20 +0800, Pengfei Xu wrote:
>> >> Should not use a hard-coded xstate size(512 * 8 = 4096 bytes) which is
>> >> wrong, should use maximum XSAVE size specified by CPUID.(EAX=0DH, ECX=0H):EBX.
>> >> If the CPU's maximum XSAVE size exceeds the hard-coded xstate size 4096 bytes,
>> >> it will cause the ptrace07 case to fail as below:
>> >> "
>> >> ./ptrace07
>> >> tst_test.c:1528: TINFO: Timeout per run is 0h 00m 30s
>> >> ptrace07.c:142: TBROK: PTRACE_SETREGSET failed with unexpected error: EFAULT (14)
>> >> tst_test.c:1571: TINFO: Killed the leftover descendant processes
>> >> 
>> >> Summary:
>> >> passed   0
>> >> failed   0
>> >> broken   1
>> >> skipped  0
>> >> warnings 0
>> >> "
>> >> 
>> >> Reported-by: Eric DeVolder <eric.devolder@oracle.com>
>> >> Reviewed-by: Chang S. Bae <chang.seok.bae@intel.com>
>> >> Signed-off-by: Pengfei Xu <pengfei.xu@intel.com>
>> >> ---
>> >>  testcases/kernel/syscalls/ptrace/ptrace07.c | 25 +++++++++++++++++----
>> >>  1 file changed, 21 insertions(+), 4 deletions(-)
>> >> 
>> >> diff --git a/testcases/kernel/syscalls/ptrace/ptrace07.c b/testcases/kernel/syscalls/ptrace/ptrace07.c
>> >> index da62cadb0..0accaceb5 100644
>> >> --- a/testcases/kernel/syscalls/ptrace/ptrace07.c
>> >> +++ b/testcases/kernel/syscalls/ptrace/ptrace07.c
>> >> @@ -35,6 +35,7 @@
>> >>  #include "config.h"
>> >>  #include "ptrace.h"
>> >>  #include "tst_test.h"
>> >> +#include "ltp_cpuid.h"
>> 
>> This is from the old API (starts with ltp_) so we shouldn't use it
>> anymore. If it is being used at all, then it's being used in a way that
>> would allow it to silently fail AFAICT.
>> 
>   Thanks for the comments, I plan to add below __cpuid_count() macro function
> as below in ltp/include/tst_cpu.h first, there seems to be some other place to
> use the cpuid function.
>
> /*
>  * gcc cpuid.h provides __cpuid_count() since v4.4.
>  * Clang/LLVM cpuid.h provides  __cpuid_count() since v3.4.0.
>  *
>  * Provide local define for tests needing __cpuid_count() because
>  * ltp needs to work in older environments that do not yet
>  * have __cpuid_count().
>  */
> #ifndef __cpuid_count
> #define __cpuid_count(level, count, a, b, c, d)				\
> 	({								\
> 	__asm__ __volatile__ ("cpuid\n\t"				\
> 			      : "=a" (a), "=b" (b), "=c" (c), "=d" (d)	\
> 			      : "0" (level), "2" (count));		\
> 	})
> #endif

Looks good. Although this should go in ltp/include/lapi/cpuid.h as this
is where we put system header fallbacks.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp