Re: [LTP] [PATCH v2 1/3] lib: adding .arch field in tst_test structure

[Richard Palethorpe <rpalethorpe@suse.de>]

Hell Li,

Li Wang <liwang@redhat.com> writes:

>  
>  
>  > Quite the opposite, it should be an array of strings, so that it's easy
>  > to work with such as:
>  >
>  >       .supported_archs = (const char *const []){"x86_64", "ppc64le", NULL},
>  >
>  > We can put it into a single string delimited by a space, but that would
>  > be more complicated to work with.
>  >
>  >> > However the hard part would be keeping the actual code and metadata in
>  >> > sync, we still have to keep the ifdefs in the code.
>  >> >
>  >> 
>  >> Yes, some inline assemble require ifdefs.
>  >> 
>  >> Btw, I look back at the reviews and find Jan said:
>  >>     "I can see how tst_on_arch() would be useful. Test is valid
>  >>      on all arches, but needs different input/constants/code/etc."
>  >> 
>  >> That may be a slight reason for keeping tst_on_arch.
>  >
>  > I guess that we should reviewe the code we have, I guess that there are
>  > a few tests where we can get rid of a few ifdefs by doing the checks
>  > dynamically.
>  >
>  > Also I guess that it would be slightly easier to work with as an enum,
>  > so that we can do:
>  >
>  >       switch (tst_arch) {
>  >       case TST_X86_64:
>  >               ...
>  >       break;
>  >       case TST_PPC64_LE:
>
>  I prefer enum as well. As an aside, we don't want to include LE in
>
> Sure, but I'm now thinking to extend the tst_arch as a structure
> so that could also be used in a string:

+1

>
>     enum tst_arch_type {
>             TST_I386,
>             TST_X86_64,
>             ...
>             TST_SPARC,
>     };
>
>     /*
>      * This tst_arch is to save the system architecture for
>      * using in the whole test case.
>      */
>     extern struct arch {
>              const char name[16];
>              enum tst_arch_type type;
>     } tst_arch;
>
> then we just can do simply in case:
>
>     switch (tst_arch.type) {
>     case TST_X86_64:
>         ...
>     break;
>
>  
>  ppc64. If someone finds that the byte order is significant for a test
>
> Yes, or we can read info via uname() into 'utsname.machine' for
> ppc64le if really needed.
>  
>  then we can add ppc64le or ppc64be. Also at some point we may need to
>  add a "machine" field for e.g. POWER8, i386 etc.
>
> Adding a new field '.machine' maybe not be necessary if just
> for POWER8/9/10, or can we find a way to combine them together
> with .supported_arch?  Umm, I'm still hesitating.

If it's required then I guess you could add it to the tst_arch_type as
an optional field. Perhaps as cpu_model. Or it could be added to a
separate section for required hardware.

>  
>  
>  Which btw, I have some buildroot and QEMU scripts which can be used to
>  test ppc64 BE and any other machine you have the hardware or QEMU
>  emulator for.
>
>  https://gitlab.com/Palethorpe/cross
>
> Thanks for sharing.


-- 
Thank you,
Richard.

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