| Message ID | 20191220092529.3239-4-pengfei.xu@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5,1/4] lib/tst_kconfig.c: add any kconfig with or without expected value function | expand |
Hi Xu, > From v5.5 main line, umip kconfig changed from "CONFIG_X86_INTEL_UMIP=y" > to "CONFIG_X86_UMIP=y". > We could use "CONFIG_X86_INTEL_UMIP|CONFIG_X86_UMIP=y" to check kconfig > CONFIG_X86_INTEL_UMIP=y(old kernel) or CONFIG_X86_UMIP=y(new kernel) for umip. > Signed-off-by: Pengfei Xu <pengfei.xu@intel.com> > --- > testcases/kernel/security/umip/umip_basic_test.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > diff --git a/testcases/kernel/security/umip/umip_basic_test.c b/testcases/kernel/security/umip/umip_basic_test.c > index 1baa26c52..24eca8890 100644 > --- a/testcases/kernel/security/umip/umip_basic_test.c > +++ b/testcases/kernel/security/umip/umip_basic_test.c > @@ -171,7 +171,7 @@ static struct tst_test test = { > .forks_child = 1, > .test = verify_umip_instruction, > .needs_kconfigs = (const char *[]){ > - "CONFIG_X86_INTEL_UMIP=y", > + "CONFIG_X86_INTEL_UMIP|CONFIG_X86_UMIP=y", We're sorry to get to your patch now, after 5 months. Thanks for a report and your effort to fix the problem. But this does not work, because current implementation does not support '|' as bitwise or, with this patch will result on tests being skipped for both cases. While it'd be easy to implement support for bitwise or in tst_kconfig_read(), it might be enough just to check for kernel version: .needs_kconfigs = (const char *[]){ #if LINUX_VERSION_CODE < KERNEL_VERSION(5, 5, 0) "CONFIG_X86_INTEL_UMIP=y", #else "CONFIG_X86_UMIP=y", #endif But that will work unless this feature is not backported (IMHO commit b971880fe79f ("x86/Kconfig: Rename UMIP config parameter") is kind of cleanup, therefore unlikely to be backported, but it can happen). Kind regards, Petr
Hi Petr, Thanks and my feedback is as below. Thanks. BR. On 2020-05-25 at 23:24:01 +0200, Petr Vorel wrote: > Hi Xu, > > > From v5.5 main line, umip kconfig changed from "CONFIG_X86_INTEL_UMIP=y" > > to "CONFIG_X86_UMIP=y". > > We could use "CONFIG_X86_INTEL_UMIP|CONFIG_X86_UMIP=y" to check kconfig > > CONFIG_X86_INTEL_UMIP=y(old kernel) or CONFIG_X86_UMIP=y(new kernel) for umip. > > > Signed-off-by: Pengfei Xu <pengfei.xu@intel.com> > > --- > > testcases/kernel/security/umip/umip_basic_test.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > diff --git a/testcases/kernel/security/umip/umip_basic_test.c b/testcases/kernel/security/umip/umip_basic_test.c > > index 1baa26c52..24eca8890 100644 > > --- a/testcases/kernel/security/umip/umip_basic_test.c > > +++ b/testcases/kernel/security/umip/umip_basic_test.c > > @@ -171,7 +171,7 @@ static struct tst_test test = { > > .forks_child = 1, > > .test = verify_umip_instruction, > > .needs_kconfigs = (const char *[]){ > > - "CONFIG_X86_INTEL_UMIP=y", > > + "CONFIG_X86_INTEL_UMIP|CONFIG_X86_UMIP=y", > > We're sorry to get to your patch now, after 5 months. > > Thanks for a report and your effort to fix the problem. But this does not work, > because current implementation does not support '|' as bitwise or, with this > patch will result on tests being skipped for both cases. CONFIG_A|CONFIG_B=y means CONGIG_A or CONGIG_B equal 'y', it will meet the test condition. So it's as expected; only could not find CONFIG_A and CONFIG_B equal to 'y', then it will not meet the test condition and exit. It should be as expected. Thank you for considering this patch again. > > While it'd be easy to implement support for bitwise or in tst_kconfig_read(), > it might be enough just to check for kernel version: > > .needs_kconfigs = (const char *[]){ > #if LINUX_VERSION_CODE < KERNEL_VERSION(5, 5, 0) > "CONFIG_X86_INTEL_UMIP=y", > #else > "CONFIG_X86_UMIP=y", > #endif > > But that will work unless this feature is not backported (IMHO commit > b971880fe79f ("x86/Kconfig: Rename UMIP config parameter") is kind of cleanup, > therefore unlikely to be backported, but it can happen). > > Kind regards, > Petr
Hi Xu, ... > > Thanks for a report and your effort to fix the problem. But this does not work, > > because current implementation does not support '|' as bitwise or, with this > > patch will result on tests being skipped for both cases. > CONFIG_A|CONFIG_B=y means CONGIG_A or CONGIG_B equal 'y', it will meet the > test condition. So it's as expected; only could not find CONFIG_A and > CONFIG_B equal to 'y', then it will not meet the test condition and exit. > It should be as expected. > Thank you for considering this patch again. Well, I understand your syntax, that you mean | as bitwise or :). But where did you find that this syntax is supported? Have a look in tst_kconfig_read() implementation (lib/tst_kconfig.c), there is nothing like this. And, indeed, if you test your patch on both CONFIG_X86_INTEL_UMIP=y and CONFIG_X86_UMIP=y, it end up with: tst_kconfig.c:252: INFO: Missing kernel CONFIG_X86_INTEL_UMIP|CONFIG_X86_UMIP=y tst_kconfig.c:284: CONF: Aborting due to unsuitable kernel config, see above! which confirm my statement there is no bitwise or support implemented :). Or am I missing something? And it might be questionable if CONFIG_A|CONFIG_B=y would mean (CONFIG_A|CONFIG_B)=y or (CONFIG_A|CONFIG_B=y), thus it should be CONFIG_A=y|CONFIG_B=y But given the fact this functionality is needed just for a single test and can be workarounded I suggested to use LINUX_VERSION_CODE instead. The only problem can happen when this is backported to the old code. But we could also try to detect that with custom call twice tst_kconfig_read() in the test setup. Kind regards, Petr > > While it'd be easy to implement support for bitwise or in tst_kconfig_read(), > > it might be enough just to check for kernel version: > > .needs_kconfigs = (const char *[]) { > > #if LINUX_VERSION_CODE < KERNEL_VERSION(5, 5, 0) > > "CONFIG_X86_INTEL_UMIP=y", > > #else > > "CONFIG_X86_UMIP=y", > > #endif > > But that will work unless this feature is not backported (IMHO commit > > b971880fe79f ("x86/Kconfig: Rename UMIP config parameter") is kind of cleanup, > > therefore unlikely to be backported, but it can happen). > > Kind regards, > > Petr
Hi Xu, > ... > > > Thanks for a report and your effort to fix the problem. But this does not work, > > > because current implementation does not support '|' as bitwise or, with this > > > patch will result on tests being skipped for both cases. > > CONFIG_A|CONFIG_B=y means CONGIG_A or CONGIG_B equal 'y', it will meet the > > test condition. So it's as expected; only could not find CONFIG_A and > > CONFIG_B equal to 'y', then it will not meet the test condition and exit. > > It should be as expected. > > Thank you for considering this patch again. > Well, I understand your syntax, that you mean | as bitwise or :). > But where did you find that this syntax is supported? Have a look in > tst_kconfig_read() implementation (lib/tst_kconfig.c), there is nothing like > this. And, indeed, if you test your patch on both CONFIG_X86_INTEL_UMIP=y and > CONFIG_X86_UMIP=y, it end up with: > tst_kconfig.c:252: INFO: Missing kernel CONFIG_X86_INTEL_UMIP|CONFIG_X86_UMIP=y > tst_kconfig.c:284: CONF: Aborting due to unsuitable kernel config, see above! > which confirm my statement there is no bitwise or support implemented :). > Or am I missing something? OK I now realized, that it's a 4th patch of patchset. I thought it's just single patch, because the rest was rejected by Cyril: https://patchwork.ozlabs.org/project/ltp/list/?series=149804&state=* But it looks like Cyril is not against the implementation, it just needs to be fixed: https://patchwork.ozlabs.org/comment/2352151/ Kind regards, Petr
Hi Petr, On 2020-05-26 at 11:27:03 +0200, Petr Vorel wrote: > Hi Xu, > > > ... > > > > Thanks for a report and your effort to fix the problem. But this does not work, > > > > because current implementation does not support '|' as bitwise or, with this > > > > patch will result on tests being skipped for both cases. > > > CONFIG_A|CONFIG_B=y means CONGIG_A or CONGIG_B equal 'y', it will meet the > > > test condition. So it's as expected; only could not find CONFIG_A and > > > CONFIG_B equal to 'y', then it will not meet the test condition and exit. > > > It should be as expected. > > > Thank you for considering this patch again. > > > Well, I understand your syntax, that you mean | as bitwise or :). > > But where did you find that this syntax is supported? Have a look in > > tst_kconfig_read() implementation (lib/tst_kconfig.c), there is nothing like > > this. And, indeed, if you test your patch on both CONFIG_X86_INTEL_UMIP=y and > > CONFIG_X86_UMIP=y, it end up with: > > > tst_kconfig.c:252: INFO: Missing kernel CONFIG_X86_INTEL_UMIP|CONFIG_X86_UMIP=y > > tst_kconfig.c:284: CONF: Aborting due to unsuitable kernel config, see above! > > > which confirm my statement there is no bitwise or support implemented :). > > Or am I missing something? > > OK I now realized, that it's a 4th patch of patchset. I thought it's just single > patch, because the rest was rejected by Cyril: > https://patchwork.ozlabs.org/project/ltp/list/?series=149804&state=* > > But it looks like Cyril is not against the implementation, it just needs to be > fixed: > https://patchwork.ozlabs.org/comment/2352151/ You are right, actually it could be worked as my suggest way: "CONFIG_A|CONFIG_B=Y". I tried to use Cyril's advice "CONFIG_A=X|CONFIG_B=Y" way, which will add more code complexity, so I just want to solve the problem I am currently facing. If we really need the "CONFIG_A=X|CONFIG_B=Y" function, which cannot be satisfied by "CONFIG_A|CONFIG_B=Y" function in the future, then we could add this function I think. Thanks for your considering. BR Thanks! Pengfei > > Kind regards, > Petr
Hi Pengfei, > > But it looks like Cyril is not against the implementation, it just needs to be > > fixed: > > https://patchwork.ozlabs.org/comment/2352151/ > You are right, actually it could be worked as my suggest way: > "CONFIG_A|CONFIG_B=Y". > I tried to use Cyril's advice "CONFIG_A=X|CONFIG_B=Y" way, which will > add more code complexity, so I just want to solve the problem I am currently > facing. > If we really need the "CONFIG_A=X|CONFIG_B=Y" function, which cannot be > satisfied by "CONFIG_A|CONFIG_B=Y" function in the future, then we could add > this function I think. > Thanks for your considering. I'd also think that we need "CONFIG_A=X|CONFIG_B=Y", because "CONFIG_A|CONFIG_B=Y" is ambiguous (we support both CONFIG_FOO and CONFIG_FOO=bar and this must stay even with |). Will you send a patch for that or shell I fix it with LINUX_VERSION_CODE < KERNEL_VERSION for now? Kind regards, Petr
Hi Petr, On 2020-05-26 at 12:11:33 +0200, Petr Vorel wrote: > Hi Pengfei, > > > > But it looks like Cyril is not against the implementation, it just needs to be > > > fixed: > > > https://patchwork.ozlabs.org/comment/2352151/ > > > You are right, actually it could be worked as my suggest way: > > "CONFIG_A|CONFIG_B=Y". > > I tried to use Cyril's advice "CONFIG_A=X|CONFIG_B=Y" way, which will > > add more code complexity, so I just want to solve the problem I am currently > > facing. > > If we really need the "CONFIG_A=X|CONFIG_B=Y" function, which cannot be > > satisfied by "CONFIG_A|CONFIG_B=Y" function in the future, then we could add > > this function I think. > > Thanks for your considering. > > I'd also think that we need "CONFIG_A=X|CONFIG_B=Y", because > "CONFIG_A|CONFIG_B=Y" is ambiguous (we support both CONFIG_FOO and > CONFIG_FOO=bar and this must stay even with |). > > Will you send a patch for that or shell I fix it with LINUX_VERSION_CODE < > KERNEL_VERSION for now? Ok, thanks for your LINUX_VERSION way to fix this issue. Thanks! BR. > > Kind regards, > Petr
Hi Petr, Seems LINUX_VERSION_CODE way it not suitable when test platform is not compiled platform. Need to use " if ((tst_kvercmp(5, 5, 0)) >= 0)" way. Thanks! BR Pengfei +86 021 61164364 -----Original Message----- From: Pengfei Xu <pengfei.xu@intel.com> Sent: Tuesday, May 26, 2020 18:37 To: Petr Vorel <pvorel@suse.cz> Cc: ltp <ltp@lists.linux.it>; Neri, Ricardo <ricardo.neri@intel.com>; Su, Heng <heng.su@intel.com>; Kasten, Robert A <robert.a.kasten@intel.com>; Cyril Hrubis <chrubis@suse.cz>; Jan Stancek <jstancek@redhat.com>; Li Wang <liwang@redhat.com> Subject: Re: [LTP] [PATCH v5 4/4] umip_basic_test.c: improve kconfig verification to avoid umip wrong abort case Hi Petr, On 2020-05-26 at 12:11:33 +0200, Petr Vorel wrote: > Hi Pengfei, > > > > But it looks like Cyril is not against the implementation, it just > > > needs to be > > > fixed: > > > https://patchwork.ozlabs.org/comment/2352151/ > > > You are right, actually it could be worked as my suggest way: > > "CONFIG_A|CONFIG_B=Y". > > I tried to use Cyril's advice "CONFIG_A=X|CONFIG_B=Y" way, which > > will add more code complexity, so I just want to solve the problem I > > am currently facing. > > If we really need the "CONFIG_A=X|CONFIG_B=Y" function, which cannot > > be satisfied by "CONFIG_A|CONFIG_B=Y" function in the future, then > > we could add this function I think. > > Thanks for your considering. > > I'd also think that we need "CONFIG_A=X|CONFIG_B=Y", because > "CONFIG_A|CONFIG_B=Y" is ambiguous (we support both CONFIG_FOO and > CONFIG_FOO=bar and this must stay even with |). > > Will you send a patch for that or shell I fix it with > LINUX_VERSION_CODE < KERNEL_VERSION for now? Ok, thanks for your LINUX_VERSION way to fix this issue. Thanks! BR. > > Kind regards, > Petr
Hi Pengfei, > Seems LINUX_VERSION_CODE way it not suitable when test platform is not compiled platform. Well, you're expected to have installed kernel headers for target platform, when you cross compile. That's what embedded distros like buildroot or yocto do. We already use constructs like this in the code. > Need to use " if ((tst_kvercmp(5, 5, 0)) >= 0)" way. As you noticed in previous mail, this will not work, as it's code outside of the function, so you cannot call any function. Kind regards, Petr
Hi Petr, Ok, thanks! Thanks! BR Pengfei +86 021 61164364 -----Original Message----- From: Petr Vorel <pvorel@suse.cz> Sent: Wednesday, May 27, 2020 14:27 To: Xu, Pengfei <pengfei.xu@intel.com> Cc: ltp <ltp@lists.linux.it>; Neri, Ricardo <ricardo.neri@intel.com>; Su, Heng <heng.su@intel.com>; Kasten, Robert A <robert.a.kasten@intel.com>; Cyril Hrubis <chrubis@suse.cz>; Jan Stancek <jstancek@redhat.com>; Li Wang <liwang@redhat.com> Subject: Re: [LTP] [PATCH v5 4/4] umip_basic_test.c: improve kconfig verification to avoid umip wrong abort case Hi Pengfei, > Seems LINUX_VERSION_CODE way it not suitable when test platform is not compiled platform. Well, you're expected to have installed kernel headers for target platform, when you cross compile. That's what embedded distros like buildroot or yocto do. We already use constructs like this in the code. > Need to use " if ((tst_kvercmp(5, 5, 0)) >= 0)" way. As you noticed in previous mail, this will not work, as it's code outside of the function, so you cannot call any function. Kind regards, Petr
diff --git a/testcases/kernel/security/umip/umip_basic_test.c b/testcases/kernel/security/umip/umip_basic_test.c index 1baa26c52..24eca8890 100644 --- a/testcases/kernel/security/umip/umip_basic_test.c +++ b/testcases/kernel/security/umip/umip_basic_test.c @@ -171,7 +171,7 @@ static struct tst_test test = { .forks_child = 1, .test = verify_umip_instruction, .needs_kconfigs = (const char *[]){ - "CONFIG_X86_INTEL_UMIP=y", + "CONFIG_X86_INTEL_UMIP|CONFIG_X86_UMIP=y", NULL }, .needs_root = 1,
From v5.5 main line, umip kconfig changed from "CONFIG_X86_INTEL_UMIP=y" to "CONFIG_X86_UMIP=y". We could use "CONFIG_X86_INTEL_UMIP|CONFIG_X86_UMIP=y" to check kconfig CONFIG_X86_INTEL_UMIP=y(old kernel) or CONFIG_X86_UMIP=y(new kernel) for umip. Signed-off-by: Pengfei Xu <pengfei.xu@intel.com> --- testcases/kernel/security/umip/umip_basic_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)