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@xxxxxxxxx>
> ---
...
> --- 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;
> + }
