Re: [kvm-unit-tests PATCH] nSVM: Added test for VGIF feature

[Sean Christopherson <seanjc@google.com>]

Nit, s/Added/Add in the shortlog

On Thu, Jul 22, 2021, Lara Lazier wrote:
> When VGIF is enabled STGI executed in guest mode
> sets bit 9, while CLGI clears bit 9 in the int_ctl (offset 60h)
> of the VMCB.
> 
> Signed-off-by: Lara Lazier <laramglazier@gmail.com>
> ---

...

> --- a/x86/svm_tests.c
> +++ b/x86/svm_tests.c
> @@ -2772,6 +2772,73 @@ static void svm_vmload_vmsave(void)
>  	vmcb->control.intercept = intercept_saved;
>  }
>  
> +static void prepare_vgif_enabled(struct svm_test *test)
> +{
> +    default_prepare(test);
> +}
> +
> +static void test_vgif(struct svm_test *test)
> +{
> +    asm volatile ("vmmcall\n\tstgi\n\tvmmcall\n\tclgi\n\tvmmcall\n\t");

While amusing, this isn't very readable :-)  The SVM tests that use this for
back-to-back VMMCALL are setting a bad example.  The space between "volatile" and
the opening "(" can go too.

	asm volatile("vmmcall\n\t"
		     "stgi\n\t"
		     "vmmcall\n\t"
		     "clgi\n\t"
		     "vmmcall\n\t");

> +

Unnecessary newline.

> +}
> +
> +static bool vgif_finished(struct svm_test *test)
> +{
> +    switch (get_test_stage(test))
> +    {
> +    case 0:
> +        if (vmcb->control.exit_code != SVM_EXIT_VMMCALL) {
> +            report(false, "VMEXIT not due to vmmcall.");
> +            return true;
> +        }
> +        vmcb->control.int_ctl |= V_GIF_ENABLED_MASK;

Setting and restoring a control flag should be done in setup/teardown, e.g. this
approach will leave V_GIF_ENABLED_MASK set if a VMMCALL check fails.

> +        vmcb->save.rip += 3;
> +        inc_test_stage(test);
> +        break;
> +    case 1:
> +        if (vmcb->control.exit_code != SVM_EXIT_VMMCALL) {
> +            report(false, "VMEXIT not due to vmmcall.");
> +            return true;
> +        }
> +        if (!(vmcb->control.int_ctl & V_GIF_MASK)) {
> +            report(false, "Failed to set VGIF when executing STGI.");
> +            vmcb->control.int_ctl &= ~V_GIF_ENABLED_MASK;
> +            return true;

Are the VGIF checks really fatal?   I.e. can we keep running of a STGI/CLGI test
fails?  That would allow for slightly cleaner code.

> +        }
> +        report(true, "STGI set VGIF bit.");
> +        vmcb->save.rip += 3;
> +        inc_test_stage(test);
> +        break;
> +    case 2:
> +        if (vmcb->control.exit_code != SVM_EXIT_VMMCALL) {
> +            report(false, "VMEXIT not due to vmmcall.");
> +            return true;
> +        }
> +        if (vmcb->control.int_ctl & V_GIF_MASK) {
> +            report(false, "Failed to clear VGIF when executing CLGI.");
> +            vmcb->control.int_ctl &= ~V_GIF_ENABLED_MASK;
> +            return true;
> +        }
> +        report(true, "CLGI cleared VGIF bit.");
> +        vmcb->save.rip += 3;
> +        inc_test_stage(test);
> +        vmcb->control.int_ctl &= ~V_GIF_ENABLED_MASK;
> +        break;


Something like this is more reader friendly as the boilerplate is consoliated,
abd the interesting test code is isolated in the switch statement.

	bool is_vmmcall_exit = (vmcb->control.exit_code == SVM_EXIT_VMMCALL);

	report(is_vmmcall_exit, ...);
	if (!is_vmmcall_exit)
		return true;

	switch (get_test_stage())
	{
	case 0:
		vmcb->control.int_ctl |= V_GIF_ENABLED_MASK;
		break;
	case 1:
		report(vmcb->control.int_ctl & V_GIF_MASK, ...);
		break;
	case 2:
		report(!(vmcb->control.int_ctl & V_GIF_MASK), ...);
		break;
	default:
		break;
	}

	vmcb->save.rip += 3;
	inc_test_stage(test);

	return get_test_stage() >= 3;

> +    default:
> +        return true;
> +        break;
> +    }