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

Richard Palethorpe
2021-11-05T14:14:15Z

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