[LTP] [PATCH] kvm_pagefault01: Do not write into tdp_mmu sysfile

[LTP] [PATCH] kvm_pagefault01: Do not write into tdp_mmu sysfile

[Martin Doucha]

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(-)

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
-- 
2.44.0


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

Re: [LTP] [PATCH] kvm_pagefault01: Do not write into tdp_mmu sysfile

[Petr Vorel]

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

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

Re: [LTP] [PATCH] kvm_pagefault01: Do not write into tdp_mmu sysfile

[Martin Doucha]

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.

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

Re: [LTP] [PATCH] kvm_pagefault01: Do not write into tdp_mmu sysfile

[Petr Vorel]

> 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

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

Re: [LTP] [PATCH] kvm_pagefault01: Do not write into tdp_mmu sysfile

[Martin Doucha]

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.

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


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

Re: [LTP] [PATCH] kvm_pagefault01: Do not write into tdp_mmu sysfile

[Jan Stancek]

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
>


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

Re: [LTP] [PATCH] kvm_pagefault01: Do not write into tdp_mmu sysfile

[Petr Vorel]

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

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