| Message ID | 20240527135718.29451-1-mdoucha@suse.cz |
|---|---|
| State | Accepted |
| Headers | show |
| Series | kvm_pagefault01: Do not write into tdp_mmu sysfile | expand |
Hi Martin, > The tdp_mmu sysfile was made read-only in kernel v6.3. Do not modify > it but only reload vendor KVM modules if needed. Some kernel versions > before v6.3 have writable tdp_mmu sysfile which appears to be > independent from vendor KVM module settings. Print a warning about > false negatives if tdp_mmu appears enabled after reloading KVM > modules. It is unclear if that state may impact bug reproducibility. Good catch. ... > static void disable_tdp(void) > { > - if (!access(TDP_MMU_SYSFILE, F_OK)) { > - /* FIXME: Is setting tdp_mmu=0 sufficient to disable TDP? */ > - return; > - } > - > if (read_bool_sys_param(TDP_AMD_SYSFILE) > 0) > reload_module("kvm_amd", "npt=0"); > if (read_bool_sys_param(TDP_INTEL_SYSFILE) > 0) > reload_module("kvm_intel", "ept=0"); > + > + if (read_bool_sys_param(TDP_MMU_SYSFILE) > 0) > + tst_res(TINFO, "tdp_mmu is enabled, beware of false negatives"); Wouldn't it be better to add "WARNING:" to make it more visible? tst_res(TINFO, "WARNING: tdp_mmu is enabled, beware of false negatives"); (if you agree, I can change it before merge) Kind regards, Petr
On 28. 05. 24 0:35, Petr Vorel wrote: >> + >> + if (read_bool_sys_param(TDP_MMU_SYSFILE) > 0) >> + tst_res(TINFO, "tdp_mmu is enabled, beware of false negatives"); > > Wouldn't it be better to add "WARNING:" to make it more visible? > tst_res(TINFO, "WARNING: tdp_mmu is enabled, beware of false negatives"); > > (if you agree, I can change it before merge) I thought about it for a while and I see good reasons for both TINFO and TWARN. It shouldn't matter for our tests because tdp_mmu defaults to off on SLE-15SP4 and SLE-15SP5. If other reviewers agree, feel free to change it to TWARN.
> On 28. 05. 24 0:35, Petr Vorel wrote: > > > + > > > + if (read_bool_sys_param(TDP_MMU_SYSFILE) > 0) > > > + tst_res(TINFO, "tdp_mmu is enabled, beware of false negatives"); > > Wouldn't it be better to add "WARNING:" to make it more visible? > > tst_res(TINFO, "WARNING: tdp_mmu is enabled, beware of false negatives"); > > (if you agree, I can change it before merge) > I thought about it for a while and I see good reasons for both TINFO and > TWARN. It shouldn't matter for our tests because tdp_mmu defaults to off on > SLE-15SP4 and SLE-15SP5. If other reviewers agree, feel free to change it to > TWARN. I slightly prefer tst_res(TINFO, "WARNING: ...", than TWARN. Although false negative is serious, still failing the test based on suspicion of false positives is not good. @Li, @Jan, @Metan: any opinion on it? Also, I send a patch to add TINFO_WARN flag (yeah, ugly name) to print TINFO but slightly more visible due the color. https://patchwork.ozlabs.org/project/ltp/list/?series=408394&state=* Kind regards, Petr
On 28. 05. 24 11:37, Petr Vorel wrote: >> I thought about it for a while and I see good reasons for both TINFO and >> TWARN. It shouldn't matter for our tests because tdp_mmu defaults to off on >> SLE-15SP4 and SLE-15SP5. If other reviewers agree, feel free to change it to >> TWARN. > > I slightly prefer tst_res(TINFO, "WARNING: ...", than TWARN. Although false > negative is serious, still failing the test based on suspicion of false > positives is not good. +1 to changing the TINFO message during merge.
On Tue, May 28, 2024 at 11:38 AM Petr Vorel <pvorel@suse.cz> wrote: > > > On 28. 05. 24 0:35, Petr Vorel wrote: > > > > + > > > > + if (read_bool_sys_param(TDP_MMU_SYSFILE) > 0) > > > > + tst_res(TINFO, "tdp_mmu is enabled, beware of false negatives"); > > > > Wouldn't it be better to add "WARNING:" to make it more visible? > > > tst_res(TINFO, "WARNING: tdp_mmu is enabled, beware of false negatives"); > > > > (if you agree, I can change it before merge) > > > I thought about it for a while and I see good reasons for both TINFO and > > TWARN. It shouldn't matter for our tests because tdp_mmu defaults to off on > > SLE-15SP4 and SLE-15SP5. If other reviewers agree, feel free to change it to > > TWARN. > > I slightly prefer tst_res(TINFO, "WARNING: ...", than TWARN. Although false > negative is serious, still failing the test based on suspicion of false > positives is not good. > > @Li, @Jan, @Metan: any opinion on it? I'd go with TINFO here, as you suggested. > > Also, I send a patch to add TINFO_WARN flag (yeah, ugly name) to print TINFO > but slightly more visible due the color. > > https://patchwork.ozlabs.org/project/ltp/list/?series=408394&state=* > > Kind regards, > Petr >
Hi Martin, Jan, > On Tue, May 28, 2024 at 11:38 AM Petr Vorel <pvorel@suse.cz> wrote: > > > On 28. 05. 24 0:35, Petr Vorel wrote: > > > > > + > > > > > + if (read_bool_sys_param(TDP_MMU_SYSFILE) > 0) > > > > > + tst_res(TINFO, "tdp_mmu is enabled, beware of false negatives"); > > > > Wouldn't it be better to add "WARNING:" to make it more visible? > > > > tst_res(TINFO, "WARNING: tdp_mmu is enabled, beware of false negatives"); > > > > (if you agree, I can change it before merge) > > > I thought about it for a while and I see good reasons for both TINFO and > > > TWARN. It shouldn't matter for our tests because tdp_mmu defaults to off on > > > SLE-15SP4 and SLE-15SP5. If other reviewers agree, feel free to change it to > > > TWARN. > > I slightly prefer tst_res(TINFO, "WARNING: ...", than TWARN. Although false > > negative is serious, still failing the test based on suspicion of false > > positives is not good. > > @Li, @Jan, @Metan: any opinion on it? > I'd go with TINFO here, as you suggested. Thanks a lot both! Patchset merged (Jan, I did not dare to add your RBT or ABT as you did not add it), with added WARNING: (as Martin agreed). Kind regards, Petr
diff --git a/testcases/kernel/kvm/kvm_pagefault01.c b/testcases/kernel/kvm/kvm_pagefault01.c index 91891848a..3dd2fa249 100644 --- a/testcases/kernel/kvm/kvm_pagefault01.c +++ b/testcases/kernel/kvm/kvm_pagefault01.c @@ -193,16 +193,14 @@ static void reload_module(const char *module, char *arg) static void disable_tdp(void) { - if (!access(TDP_MMU_SYSFILE, F_OK)) { - /* FIXME: Is setting tdp_mmu=0 sufficient to disable TDP? */ - return; - } - if (read_bool_sys_param(TDP_AMD_SYSFILE) > 0) reload_module("kvm_amd", "npt=0"); if (read_bool_sys_param(TDP_INTEL_SYSFILE) > 0) reload_module("kvm_intel", "ept=0"); + + if (read_bool_sys_param(TDP_MMU_SYSFILE) > 0) + tst_res(TINFO, "tdp_mmu is enabled, beware of false negatives"); } static void setup(void) @@ -216,11 +214,6 @@ static struct tst_test test = { .setup = setup, .cleanup = tst_kvm_cleanup, .needs_root = 1, - .save_restore = (const struct tst_path_val[]) { - {"/sys/module/kvm/parameters/tdp_mmu", "0", - TST_SR_SKIP_MISSING | TST_SR_TCONF_RO}, - {} - }, .supported_archs = (const char *const []) { "x86_64", NULL
The tdp_mmu sysfile was made read-only in kernel v6.3. Do not modify it but only reload vendor KVM modules if needed. Some kernel versions before v6.3 have writable tdp_mmu sysfile which appears to be independent from vendor KVM module settings. Print a warning about false negatives if tdp_mmu appears enabled after reloading KVM modules. It is unclear if that state may impact bug reproducibility. Signed-off-by: Martin Doucha <mdoucha@suse.cz> --- testcases/kernel/kvm/kvm_pagefault01.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)