[PATCH] KVM: SVM: Update dump_ghcb() to use the GHCB snapshot fields

[PATCH] KVM: SVM: Update dump_ghcb() to use the GHCB snapshot fields

[Tom Lendacky]

Commit 4e15a0ddc3ff ("KVM: SEV: snapshot the GHCB before accessing it")
updated the SEV code to take a snapshot of the GHCB before using it. But
the dump_ghcb() function wasn't updated to use the snapshot locations.
This results in incorrect output from dump_ghcb() for the "is_valid" and
"valid_bitmap" fields.

Update dump_ghcb() to use the proper locations.

Fixes: 4e15a0ddc3ff ("KVM: SEV: snapshot the GHCB before accessing it")
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 arch/x86/kvm/svm/sev.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 0bc708ee2788..91e514d07f8a 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3173,9 +3173,14 @@ void sev_free_vcpu(struct kvm_vcpu *vcpu)
 		kvfree(svm->sev_es.ghcb_sa);
 }
 
+static u64 kvm_ghcb_get_sw_exit_code(struct vmcb_control_area *control)
+{
+	return (((u64)control->exit_code_hi) << 32) | control->exit_code;
+}
+
 static void dump_ghcb(struct vcpu_svm *svm)
 {
-	struct ghcb *ghcb = svm->sev_es.ghcb;
+	struct vmcb_control_area *control = &svm->vmcb->control;
 	unsigned int nbits;
 
 	/* Re-use the dump_invalid_vmcb module parameter */
@@ -3184,18 +3189,18 @@ static void dump_ghcb(struct vcpu_svm *svm)
 		return;
 	}
 
-	nbits = sizeof(ghcb->save.valid_bitmap) * 8;
+	nbits = sizeof(svm->sev_es.valid_bitmap) * 8;
 
 	pr_err("GHCB (GPA=%016llx):\n", svm->vmcb->control.ghcb_gpa);
 	pr_err("%-20s%016llx is_valid: %u\n", "sw_exit_code",
-	       ghcb->save.sw_exit_code, ghcb_sw_exit_code_is_valid(ghcb));
+	       kvm_ghcb_get_sw_exit_code(control), kvm_ghcb_sw_exit_code_is_valid(svm));
 	pr_err("%-20s%016llx is_valid: %u\n", "sw_exit_info_1",
-	       ghcb->save.sw_exit_info_1, ghcb_sw_exit_info_1_is_valid(ghcb));
+	       control->exit_info_1, kvm_ghcb_sw_exit_info_1_is_valid(svm));
 	pr_err("%-20s%016llx is_valid: %u\n", "sw_exit_info_2",
-	       ghcb->save.sw_exit_info_2, ghcb_sw_exit_info_2_is_valid(ghcb));
+	       control->exit_info_2, kvm_ghcb_sw_exit_info_2_is_valid(svm));
 	pr_err("%-20s%016llx is_valid: %u\n", "sw_scratch",
-	       ghcb->save.sw_scratch, ghcb_sw_scratch_is_valid(ghcb));
-	pr_err("%-20s%*pb\n", "valid_bitmap", nbits, ghcb->save.valid_bitmap);
+	       svm->sev_es.sw_scratch, kvm_ghcb_sw_scratch_is_valid(svm));
+	pr_err("%-20s%*pb\n", "valid_bitmap", nbits, svm->sev_es.valid_bitmap);
 }
 
 static void sev_es_sync_to_ghcb(struct vcpu_svm *svm)
@@ -3266,11 +3271,6 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
 	memset(ghcb->save.valid_bitmap, 0, sizeof(ghcb->save.valid_bitmap));
 }
 
-static u64 kvm_ghcb_get_sw_exit_code(struct vmcb_control_area *control)
-{
-	return (((u64)control->exit_code_hi) << 32) | control->exit_code;
-}
-
 static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
 {
 	struct vmcb_control_area *control = &svm->vmcb->control;

base-commit: 2d7124941a273c7233849a7a2bbfbeb7e28f1caa
-- 
2.46.2

Re: [PATCH] KVM: SVM: Update dump_ghcb() to use the GHCB snapshot fields

[Sean Christopherson]

On Mon, Apr 28, 2025, Tom Lendacky wrote:
> @@ -3184,18 +3189,18 @@ static void dump_ghcb(struct vcpu_svm *svm)
>  		return;
>  	}
>  
> -	nbits = sizeof(ghcb->save.valid_bitmap) * 8;
> +	nbits = sizeof(svm->sev_es.valid_bitmap) * 8;

I'm planning on adding this comment to explain the use of KVM's snapshot.  Please
holler if it's wrong/misleading in any way.

	/*
	 * Print KVM's snapshot of the GHCB that was (unsuccessfully) used to
	 * handle the exit.  If the guest has since modified the GHCB itself,
	 * dumping the raw GHCB won't help debug why KVM was unable to handle
	 * the VMGEXIT that KVM observed.
	 */

>  	pr_err("GHCB (GPA=%016llx):\n", svm->vmcb->control.ghcb_gpa);
>  	pr_err("%-20s%016llx is_valid: %u\n", "sw_exit_code",
> -	       ghcb->save.sw_exit_code, ghcb_sw_exit_code_is_valid(ghcb));
> +	       kvm_ghcb_get_sw_exit_code(control), kvm_ghcb_sw_exit_code_is_valid(svm));
>  	pr_err("%-20s%016llx is_valid: %u\n", "sw_exit_info_1",
> -	       ghcb->save.sw_exit_info_1, ghcb_sw_exit_info_1_is_valid(ghcb));
> +	       control->exit_info_1, kvm_ghcb_sw_exit_info_1_is_valid(svm));
>  	pr_err("%-20s%016llx is_valid: %u\n", "sw_exit_info_2",
> -	       ghcb->save.sw_exit_info_2, ghcb_sw_exit_info_2_is_valid(ghcb));
> +	       control->exit_info_2, kvm_ghcb_sw_exit_info_2_is_valid(svm));
>  	pr_err("%-20s%016llx is_valid: %u\n", "sw_scratch",
> -	       ghcb->save.sw_scratch, ghcb_sw_scratch_is_valid(ghcb));
> -	pr_err("%-20s%*pb\n", "valid_bitmap", nbits, ghcb->save.valid_bitmap);
> +	       svm->sev_es.sw_scratch, kvm_ghcb_sw_scratch_is_valid(svm));
> +	pr_err("%-20s%*pb\n", "valid_bitmap", nbits, svm->sev_es.valid_bitmap);
>  }

Re: [PATCH] KVM: SVM: Update dump_ghcb() to use the GHCB snapshot fields

[Liam Merwick]

On 29/04/2025 16:22, Sean Christopherson wrote:
> On Mon, Apr 28, 2025, Tom Lendacky wrote:
>> @@ -3184,18 +3189,18 @@ static void dump_ghcb(struct vcpu_svm *svm)
>>   		return;
>>   	}
>>   
>> -	nbits = sizeof(ghcb->save.valid_bitmap) * 8;
>> +	nbits = sizeof(svm->sev_es.valid_bitmap) * 8;
> 
> I'm planning on adding this comment to explain the use of KVM's snapshot.  Please
> holler if it's wrong/misleading in any way.
> 
> 	/*
> 	 * Print KVM's snapshot of the GHCB that was (unsuccessfully) used to
> 	 * handle the exit.  If the guest has since modified the GHCB itself,
> 	 * dumping the raw GHCB won't help debug why KVM was unable to handle
> 	 * the VMGEXIT that KVM observed.
> 	 */
> 
>>   	pr_err("GHCB (GPA=%016llx):\n", svm->vmcb->control.ghcb_gpa);

Would printing "GHCB snapshot (GPA= ...." here instead of just "GHCB 
(GPA= ..."
help gently remind people just looking at the debug output of this too?

Either way, for patch:
Reviewed-by: Liam Merwick <liam.merwick@oracle.com>


>>   	pr_err("%-20s%016llx is_valid: %u\n", "sw_exit_code",
>> -	       ghcb->save.sw_exit_code, ghcb_sw_exit_code_is_valid(ghcb));
>> +	       kvm_ghcb_get_sw_exit_code(control), kvm_ghcb_sw_exit_code_is_valid(svm));
>>   	pr_err("%-20s%016llx is_valid: %u\n", "sw_exit_info_1",
>> -	       ghcb->save.sw_exit_info_1, ghcb_sw_exit_info_1_is_valid(ghcb));
>> +	       control->exit_info_1, kvm_ghcb_sw_exit_info_1_is_valid(svm));
>>   	pr_err("%-20s%016llx is_valid: %u\n", "sw_exit_info_2",
>> -	       ghcb->save.sw_exit_info_2, ghcb_sw_exit_info_2_is_valid(ghcb));
>> +	       control->exit_info_2, kvm_ghcb_sw_exit_info_2_is_valid(svm));
>>   	pr_err("%-20s%016llx is_valid: %u\n", "sw_scratch",
>> -	       ghcb->save.sw_scratch, ghcb_sw_scratch_is_valid(ghcb));
>> -	pr_err("%-20s%*pb\n", "valid_bitmap", nbits, ghcb->save.valid_bitmap);
>> +	       svm->sev_es.sw_scratch, kvm_ghcb_sw_scratch_is_valid(svm));
>> +	pr_err("%-20s%*pb\n", "valid_bitmap", nbits, svm->sev_es.valid_bitmap);
>>   }
> 

Re: [PATCH] KVM: SVM: Update dump_ghcb() to use the GHCB snapshot fields

[Tom Lendacky]

On 4/29/25 11:31, Liam Merwick wrote:
> On 29/04/2025 16:22, Sean Christopherson wrote:
>> On Mon, Apr 28, 2025, Tom Lendacky wrote:
>>> @@ -3184,18 +3189,18 @@ static void dump_ghcb(struct vcpu_svm *svm)
>>>           return;
>>>       }
>>>   -    nbits = sizeof(ghcb->save.valid_bitmap) * 8;
>>> +    nbits = sizeof(svm->sev_es.valid_bitmap) * 8;
>>
>> I'm planning on adding this comment to explain the use of KVM's
>> snapshot.  Please
>> holler if it's wrong/misleading in any way.
>>
>>     /*
>>      * Print KVM's snapshot of the GHCB that was (unsuccessfully) used to
>>      * handle the exit.  If the guest has since modified the GHCB itself,
>>      * dumping the raw GHCB won't help debug why KVM was unable to handle
>>      * the VMGEXIT that KVM observed.
>>      */
>>
>>>       pr_err("GHCB (GPA=%016llx):\n", svm->vmcb->control.ghcb_gpa);
> 
> Would printing "GHCB snapshot (GPA= ...." here instead of just "GHCB (GPA=
> ..."
> help gently remind people just looking at the debug output of this too?

Except the GPA is that of the actual GHCB. And the values being printed
are the actual values sent by the guest and being used by KVM at the time
the GHCB was read. So I'm not sure if that would clear things up at all.

Thanks,
Tom

> 
> Either way, for patch:
> Reviewed-by: Liam Merwick <liam.merwick@oracle.com>
> 
> 
>>>       pr_err("%-20s%016llx is_valid: %u\n", "sw_exit_code",
>>> -           ghcb->save.sw_exit_code, ghcb_sw_exit_code_is_valid(ghcb));
>>> +           kvm_ghcb_get_sw_exit_code(control),
>>> kvm_ghcb_sw_exit_code_is_valid(svm));
>>>       pr_err("%-20s%016llx is_valid: %u\n", "sw_exit_info_1",
>>> -           ghcb->save.sw_exit_info_1,
>>> ghcb_sw_exit_info_1_is_valid(ghcb));
>>> +           control->exit_info_1, kvm_ghcb_sw_exit_info_1_is_valid(svm));
>>>       pr_err("%-20s%016llx is_valid: %u\n", "sw_exit_info_2",
>>> -           ghcb->save.sw_exit_info_2,
>>> ghcb_sw_exit_info_2_is_valid(ghcb));
>>> +           control->exit_info_2, kvm_ghcb_sw_exit_info_2_is_valid(svm));
>>>       pr_err("%-20s%016llx is_valid: %u\n", "sw_scratch",
>>> -           ghcb->save.sw_scratch, ghcb_sw_scratch_is_valid(ghcb));
>>> -    pr_err("%-20s%*pb\n", "valid_bitmap", nbits,
>>> ghcb->save.valid_bitmap);
>>> +           svm->sev_es.sw_scratch, kvm_ghcb_sw_scratch_is_valid(svm));
>>> +    pr_err("%-20s%*pb\n", "valid_bitmap", nbits,
>>> svm->sev_es.valid_bitmap);
>>>   }
>>
> 

Re: [PATCH] KVM: SVM: Update dump_ghcb() to use the GHCB snapshot fields

[Sean Christopherson]

On Tue, Apr 29, 2025, Tom Lendacky wrote:
> On 4/29/25 11:31, Liam Merwick wrote:
> > On 29/04/2025 16:22, Sean Christopherson wrote:
> >> On Mon, Apr 28, 2025, Tom Lendacky wrote:
> >>> @@ -3184,18 +3189,18 @@ static void dump_ghcb(struct vcpu_svm *svm)
> >>>           return;
> >>>       }
> >>>   -    nbits = sizeof(ghcb->save.valid_bitmap) * 8;
> >>> +    nbits = sizeof(svm->sev_es.valid_bitmap) * 8;
> >>
> >> I'm planning on adding this comment to explain the use of KVM's
> >> snapshot.  Please
> >> holler if it's wrong/misleading in any way.
> >>
> >>     /*
> >>      * Print KVM's snapshot of the GHCB that was (unsuccessfully) used to
> >>      * handle the exit.  If the guest has since modified the GHCB itself,
> >>      * dumping the raw GHCB won't help debug why KVM was unable to handle
> >>      * the VMGEXIT that KVM observed.
> >>      */
> >>
> >>>       pr_err("GHCB (GPA=%016llx):\n", svm->vmcb->control.ghcb_gpa);
> > 
> > Would printing "GHCB snapshot (GPA= ...." here instead of just "GHCB (GPA=
> > ..."
> > help gently remind people just looking at the debug output of this too?
> 
> Except the GPA is that of the actual GHCB. And the values being printed
> are the actual values sent by the guest and being used by KVM at the time
> the GHCB was read. So I'm not sure if that would clear things up at all.

I personally like the "snapshot" addendum.  Yes, the GPA is the GPA of the GHCB,
but it also the GPA from which the snapshot was obtained.  Ditto for the values.

For folks that aren't aware that KVM operates on a snapshot of the GHCB, they
could end up really confused if they somehow have access to the raw GHCB, e.g.
from the guest side or something.

Re: [PATCH] KVM: SVM: Update dump_ghcb() to use the GHCB snapshot fields

[Tom Lendacky]

On 4/29/25 10:22, Sean Christopherson wrote:
> On Mon, Apr 28, 2025, Tom Lendacky wrote:
>> @@ -3184,18 +3189,18 @@ static void dump_ghcb(struct vcpu_svm *svm)
>>  		return;
>>  	}
>>  
>> -	nbits = sizeof(ghcb->save.valid_bitmap) * 8;
>> +	nbits = sizeof(svm->sev_es.valid_bitmap) * 8;
> 
> I'm planning on adding this comment to explain the use of KVM's snapshot.  Please
> holler if it's wrong/misleading in any way.
> 
> 	/*
> 	 * Print KVM's snapshot of the GHCB that was (unsuccessfully) used to

s/snapshot/snapshot values/ ?

> 	 * handle the exit.  If the guest has since modified the GHCB itself,
> 	 * dumping the raw GHCB won't help debug why KVM was unable to handle
> 	 * the VMGEXIT that KVM observed.
> 	 */

Otherwise, looks good to me.

Thanks,
Tom

> 
>>  	pr_err("GHCB (GPA=%016llx):\n", svm->vmcb->control.ghcb_gpa);
>>  	pr_err("%-20s%016llx is_valid: %u\n", "sw_exit_code",
>> -	       ghcb->save.sw_exit_code, ghcb_sw_exit_code_is_valid(ghcb));
>> +	       kvm_ghcb_get_sw_exit_code(control), kvm_ghcb_sw_exit_code_is_valid(svm));
>>  	pr_err("%-20s%016llx is_valid: %u\n", "sw_exit_info_1",
>> -	       ghcb->save.sw_exit_info_1, ghcb_sw_exit_info_1_is_valid(ghcb));
>> +	       control->exit_info_1, kvm_ghcb_sw_exit_info_1_is_valid(svm));
>>  	pr_err("%-20s%016llx is_valid: %u\n", "sw_exit_info_2",
>> -	       ghcb->save.sw_exit_info_2, ghcb_sw_exit_info_2_is_valid(ghcb));
>> +	       control->exit_info_2, kvm_ghcb_sw_exit_info_2_is_valid(svm));
>>  	pr_err("%-20s%016llx is_valid: %u\n", "sw_scratch",
>> -	       ghcb->save.sw_scratch, ghcb_sw_scratch_is_valid(ghcb));
>> -	pr_err("%-20s%*pb\n", "valid_bitmap", nbits, ghcb->save.valid_bitmap);
>> +	       svm->sev_es.sw_scratch, kvm_ghcb_sw_scratch_is_valid(svm));
>> +	pr_err("%-20s%*pb\n", "valid_bitmap", nbits, svm->sev_es.valid_bitmap);
>>  }

Re: [PATCH] KVM: SVM: Update dump_ghcb() to use the GHCB snapshot fields

[Sean Christopherson]

On Mon, 28 Apr 2025 13:55:31 -0500, Tom Lendacky wrote:
> Commit 4e15a0ddc3ff ("KVM: SEV: snapshot the GHCB before accessing it")
> updated the SEV code to take a snapshot of the GHCB before using it. But
> the dump_ghcb() function wasn't updated to use the snapshot locations.
> This results in incorrect output from dump_ghcb() for the "is_valid" and
> "valid_bitmap" fields.
> 
> Update dump_ghcb() to use the proper locations.
> 
> [...]

Applied to kvm-x86 fixes.

Tom, I tried to find a middle ground between capturing the "snapshot" behavior
and not making it seem like the reported GPA is the GPA of the snapshot.  Holler
if you don't like the end result.

[1/1] KVM: SVM: Update dump_ghcb() to use the GHCB snapshot fields
      https://github.com/kvm-x86/linux/commit/5fea0c6c0ebe

--
https://github.com/kvm-x86/linux/tree/next

Re: [PATCH] KVM: SVM: Update dump_ghcb() to use the GHCB snapshot fields

[Tom Lendacky]

On 5/2/25 16:50, Sean Christopherson wrote:
> On Mon, 28 Apr 2025 13:55:31 -0500, Tom Lendacky wrote:
>> Commit 4e15a0ddc3ff ("KVM: SEV: snapshot the GHCB before accessing it")
>> updated the SEV code to take a snapshot of the GHCB before using it. But
>> the dump_ghcb() function wasn't updated to use the snapshot locations.
>> This results in incorrect output from dump_ghcb() for the "is_valid" and
>> "valid_bitmap" fields.
>>
>> Update dump_ghcb() to use the proper locations.
>>
>> [...]
> 
> Applied to kvm-x86 fixes.
> 
> Tom, I tried to find a middle ground between capturing the "snapshot" behavior
> and not making it seem like the reported GPA is the GPA of the snapshot.  Holler
> if you don't like the end result.

No hollerin' here. Looks good, Sean.

Thanks,
Tom

> 
> [1/1] KVM: SVM: Update dump_ghcb() to use the GHCB snapshot fields
>       https://github.com/kvm-x86/linux/commit/5fea0c6c0ebe
> 
> --
> https://github.com/kvm-x86/linux/tree/next