On Thu, Oct 20, 2022, Maxim Levitsky wrote:
Changelog please. This patch in particular is extremely difficult to review
without some explanation of what is being done, and why.
If it's not too much trouble, splitting this over multiple patches would be nice.
> Signed-off-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
> ---
> lib/x86/svm_lib.h | 58 +++++++++++++++++++++++++++++++++++++++
> x86/svm.c | 51 ++++++++++------------------------
> x86/svm.h | 70 ++---------------------------------------------
> x86/svm_tests.c | 24 ++++++++++------
> 4 files changed, 91 insertions(+), 112 deletions(-)
>
> diff --git a/lib/x86/svm_lib.h b/lib/x86/svm_lib.h
> index 27c3b137..59db26de 100644
> --- a/lib/x86/svm_lib.h
> +++ b/lib/x86/svm_lib.h
> @@ -71,4 +71,62 @@ u8* svm_get_io_bitmap(void);
> #define MSR_BITMAP_SIZE 8192
>
>
> +struct svm_extra_regs
Why not just svm_gprs? This could even include RAX by grabbing it from the VMCB
after VMRUN.
> +{
> + u64 rbx;
> + u64 rcx;
> + u64 rdx;
> + u64 rbp;
> + u64 rsi;
> + u64 rdi;
> + u64 r8;
> + u64 r9;
> + u64 r10;
> + u64 r11;
> + u64 r12;
> + u64 r13;
> + u64 r14;
> + u64 r15;
Tab instead of spaces.
> +};
> +
> +#define SWAP_GPRS(reg) \
> + "xchg %%rcx, 0x08(%%" reg ")\n\t" \
No need for 2-tab indentation.
> + "xchg %%rdx, 0x10(%%" reg ")\n\t" \
> + "xchg %%rbp, 0x18(%%" reg ")\n\t" \
> + "xchg %%rsi, 0x20(%%" reg ")\n\t" \
> + "xchg %%rdi, 0x28(%%" reg ")\n\t" \
> + "xchg %%r8, 0x30(%%" reg ")\n\t" \
> + "xchg %%r9, 0x38(%%" reg ")\n\t" \
> + "xchg %%r10, 0x40(%%" reg ")\n\t" \
> + "xchg %%r11, 0x48(%%" reg ")\n\t" \
> + "xchg %%r12, 0x50(%%" reg ")\n\t" \
> + "xchg %%r13, 0x58(%%" reg ")\n\t" \
> + "xchg %%r14, 0x60(%%" reg ")\n\t" \
> + "xchg %%r15, 0x68(%%" reg ")\n\t" \
> + \
Extra line.
> + "xchg %%rbx, 0x00(%%" reg ")\n\t" \
Why is RBX last here, but first in the struct? Ah, because the initial swap uses
RBX as the scratch register. Why use RAX for the post-VMRUN swap? AFAICT, that's
completely arbitrary.
> +
> +
> +#define __SVM_VMRUN(vmcb, regs, label) \
> + asm volatile ( \
Unnecessarily deep indentation.
> + "vmload %%rax\n\t" \
> + "push %%rax\n\t" \
> + "push %%rbx\n\t" \
> + SWAP_GPRS("rbx") \
> + ".global " label "\n\t" \
> + label ": vmrun %%rax\n\t" \
> + "vmsave %%rax\n\t" \
> + "pop %%rax\n\t" \
> + SWAP_GPRS("rax") \
> + "pop %%rax\n\t" \
> + : \
> + : "a" (virt_to_phys(vmcb)), \
> + "b"(regs) \
> + /* clobbers*/ \
> + : "memory" \
> + );
If we're going to rewrite this, why not turn it into a proper assembly routine?
E.g. the whole test_run() noinline thing just so that vmrun_rip isn't redefined
is gross.
> diff --git a/x86/svm.c b/x86/svm.c
> index 37b4cd38..9484a6d1 100644
> --- a/x86/svm.c
> +++ b/x86/svm.c
> @@ -76,11 +76,11 @@ static void test_thunk(struct svm_test *test)
> vmmcall();
> }
>
> -struct regs regs;
> +struct svm_extra_regs regs;
>
> -struct regs get_regs(void)
> +struct svm_extra_regs* get_regs(void)
> {
> - return regs;
> + return ®s;
This isn't strictly necessary, is it? I.e. avoiding the copy can be done in a
separate patch, no?
> @@ -2996,7 +2998,7 @@ static void svm_lbrv_test1(void)
>
> wrmsr(MSR_IA32_DEBUGCTLMSR, DEBUGCTLMSR_LBR);
> DO_BRANCH(host_branch1);
> - SVM_BARE_VMRUN;
> + SVM_VMRUN(vmcb,regs);
Space after the comma. Multiple cases below too.
> dbgctl = rdmsr(MSR_IA32_DEBUGCTLMSR);
>
> if (vmcb->control.exit_code != SVM_EXIT_VMMCALL) {
> @@ -3011,6 +3013,8 @@ static void svm_lbrv_test1(void)
>
> static void svm_lbrv_test2(void)
> {
> + struct svm_extra_regs* regs = get_regs();
> +
> report(true, "Test that without LBRV enabled, guest LBR state does 'leak' to the host(2)");
>
> vmcb->save.rip = (ulong)svm_lbrv_test_guest2;
> @@ -3019,7 +3023,7 @@ static void svm_lbrv_test2(void)
> wrmsr(MSR_IA32_DEBUGCTLMSR, DEBUGCTLMSR_LBR);
> DO_BRANCH(host_branch2);
> wrmsr(MSR_IA32_DEBUGCTLMSR, 0);
> - SVM_BARE_VMRUN;
> + SVM_VMRUN(vmcb,regs);
> dbgctl = rdmsr(MSR_IA32_DEBUGCTLMSR);
> wrmsr(MSR_IA32_DEBUGCTLMSR, 0);
>
> @@ -3035,6 +3039,8 @@ static void svm_lbrv_test2(void)
>
> static void svm_lbrv_nested_test1(void)
> {
> + struct svm_extra_regs* regs = get_regs();
> +
> if (!lbrv_supported()) {
> report_skip("LBRV not supported in the guest");
> return;
> @@ -3047,7 +3053,7 @@ static void svm_lbrv_nested_test1(void)
>
> wrmsr(MSR_IA32_DEBUGCTLMSR, DEBUGCTLMSR_LBR);
> DO_BRANCH(host_branch3);
> - SVM_BARE_VMRUN;
> + SVM_VMRUN(vmcb,regs);
> dbgctl = rdmsr(MSR_IA32_DEBUGCTLMSR);
> wrmsr(MSR_IA32_DEBUGCTLMSR, 0);
>
> @@ -3068,6 +3074,8 @@ static void svm_lbrv_nested_test1(void)
>
> static void svm_lbrv_nested_test2(void)
> {
> + struct svm_extra_regs* regs = get_regs();
> +
> if (!lbrv_supported()) {
> report_skip("LBRV not supported in the guest");
> return;
> @@ -3083,7 +3091,7 @@ static void svm_lbrv_nested_test2(void)
>
> wrmsr(MSR_IA32_DEBUGCTLMSR, DEBUGCTLMSR_LBR);
> DO_BRANCH(host_branch4);
> - SVM_BARE_VMRUN;
> + SVM_VMRUN(vmcb,regs);
> dbgctl = rdmsr(MSR_IA32_DEBUGCTLMSR);
> wrmsr(MSR_IA32_DEBUGCTLMSR, 0);
>
> --
> 2.26.3
>
