[RFC PATCH] x86/sev: Cleanup vc_handle_msr()

[RFC PATCH] x86/sev: Cleanup vc_handle_msr()

[Borislav Petkov (AMD)]

Hi,

I think we should clean this one up before in-flight patchsets make it more
unreadable and in need for an even more cleanup.

---
Carve out the MSR_SVSM_CAA into a helper with the suggestion that
upcoming future users should do the same. Rename that silly exit_info_1
into what it actually means in this function - whether the MSR access is
a read or a write.

No functional changes.

Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
---
 arch/x86/coco/sev/core.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 97f445f3366a..1efb4a5c5ab3 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -1406,35 +1406,39 @@ int __init sev_es_efi_map_ghcbs(pgd_t *pgd)
 	return 0;
 }
 
+/* Writes to the SVSM CAA MSR are ignored */
+static enum es_result __vc_handle_msr_caa(struct pt_regs *regs, bool write)
+{
+	if (write)
+		return ES_OK;
+
+	regs->ax = lower_32_bits(this_cpu_read(svsm_caa_pa));
+	regs->dx = upper_32_bits(this_cpu_read(svsm_caa_pa));
+
+	return ES_OK;
+}
+
 static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
 {
 	struct pt_regs *regs = ctxt->regs;
 	enum es_result ret;
-	u64 exit_info_1;
+	bool write;
 
 	/* Is it a WRMSR? */
-	exit_info_1 = (ctxt->insn.opcode.bytes[1] == 0x30) ? 1 : 0;
-
-	if (regs->cx == MSR_SVSM_CAA) {
-		/* Writes to the SVSM CAA msr are ignored */
-		if (exit_info_1)
-			return ES_OK;
-
-		regs->ax = lower_32_bits(this_cpu_read(svsm_caa_pa));
-		regs->dx = upper_32_bits(this_cpu_read(svsm_caa_pa));
+	write = ctxt->insn.opcode.bytes[1] == 0x30;
 
-		return ES_OK;
-	}
+	if (regs->cx == MSR_SVSM_CAA)
+		return __vc_handle_msr_caa(regs, write);
 
 	ghcb_set_rcx(ghcb, regs->cx);
-	if (exit_info_1) {
+	if (write) {
 		ghcb_set_rax(ghcb, regs->ax);
 		ghcb_set_rdx(ghcb, regs->dx);
 	}
 
-	ret = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_MSR, exit_info_1, 0);
+	ret = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_MSR, !!write, 0);
 
-	if ((ret == ES_OK) && (!exit_info_1)) {
+	if ((ret == ES_OK) && (!write)) {
 		regs->ax = ghcb->save.rax;
 		regs->dx = ghcb->save.rdx;
 	}
-- 
2.43.0


-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [RFC PATCH] x86/sev: Cleanup vc_handle_msr()

[Tom Lendacky]

On 11/6/24 11:26, Borislav Petkov (AMD) wrote:
> Hi,
> 
> I think we should clean this one up before in-flight patchsets make it more
> unreadable and in need for an even more cleanup.
> 
> ---
> Carve out the MSR_SVSM_CAA into a helper with the suggestion that
> upcoming future users should do the same. Rename that silly exit_info_1
> into what it actually means in this function - whether the MSR access is
> a read or a write.
> 
> No functional changes.
> 
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> ---
>  arch/x86/coco/sev/core.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index 97f445f3366a..1efb4a5c5ab3 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -1406,35 +1406,39 @@ int __init sev_es_efi_map_ghcbs(pgd_t *pgd)
>  	return 0;
>  }
>  
> +/* Writes to the SVSM CAA MSR are ignored */
> +static enum es_result __vc_handle_msr_caa(struct pt_regs *regs, bool write)
> +{
> +	if (write)
> +		return ES_OK;
> +
> +	regs->ax = lower_32_bits(this_cpu_read(svsm_caa_pa));
> +	regs->dx = upper_32_bits(this_cpu_read(svsm_caa_pa));
> +
> +	return ES_OK;
> +}
> +
>  static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
>  {
>  	struct pt_regs *regs = ctxt->regs;
>  	enum es_result ret;
> -	u64 exit_info_1;
> +	bool write;
>  
>  	/* Is it a WRMSR? */
> -	exit_info_1 = (ctxt->insn.opcode.bytes[1] == 0x30) ? 1 : 0;
> -
> -	if (regs->cx == MSR_SVSM_CAA) {
> -		/* Writes to the SVSM CAA msr are ignored */
> -		if (exit_info_1)
> -			return ES_OK;
> -
> -		regs->ax = lower_32_bits(this_cpu_read(svsm_caa_pa));
> -		regs->dx = upper_32_bits(this_cpu_read(svsm_caa_pa));
> +	write = ctxt->insn.opcode.bytes[1] == 0x30;
>  
> -		return ES_OK;
> -	}
> +	if (regs->cx == MSR_SVSM_CAA)
> +		return __vc_handle_msr_caa(regs, write);
>  
>  	ghcb_set_rcx(ghcb, regs->cx);
> -	if (exit_info_1) {
> +	if (write) {
>  		ghcb_set_rax(ghcb, regs->ax);
>  		ghcb_set_rdx(ghcb, regs->dx);
>  	}
>  
> -	ret = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_MSR, exit_info_1, 0);
> +	ret = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_MSR, !!write, 0);

Is the !! necessary? It should have either 0 or 1 because of the boolean
operation used to set it, right?

>  
> -	if ((ret == ES_OK) && (!exit_info_1)) {
> +	if ((ret == ES_OK) && (!write)) {

I guess the parentheses around "!write" can be removed while your at it.

Other than those two little things...

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

>  		regs->ax = ghcb->save.rax;
>  		regs->dx = ghcb->save.rdx;
>  	}

Re: [RFC PATCH] x86/sev: Cleanup vc_handle_msr()

[Borislav Petkov]

On Wed, Nov 06, 2024 at 01:40:47PM -0600, Tom Lendacky wrote:
> Is the !! necessary? It should have either 0 or 1 because of the boolean
> operation used to set it, right?

I was going to be overly cautious but integer promotion will make sure there
really is a 0 or a 1. So yeah, I can drop the !!.

> 
> >  
> > -	if ((ret == ES_OK) && (!exit_info_1)) {
> > +	if ((ret == ES_OK) && (!write)) {
> 
> I guess the parentheses around "!write" can be removed while your at it.

Ack.

> Other than those two little things...
> 
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

Thanks!

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [RFC PATCH] x86/sev: Cleanup vc_handle_msr()

[Gupta, Pankaj]

On 11/6/2024 6:26 PM, Borislav Petkov (AMD) wrote:
> Hi,
> 
> I think we should clean this one up before in-flight patchsets make it more
> unreadable and in need for an even more cleanup.
> 
> ---
> Carve out the MSR_SVSM_CAA into a helper with the suggestion that
> upcoming future users should do the same. Rename that silly exit_info_1
> into what it actually means in this function - whether the MSR access is
> a read or a write.
> 
> No functional changes.
> 
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>

LGTM

With minor comments from Tom,

Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com>

> ---
>   arch/x86/coco/sev/core.c | 34 +++++++++++++++++++---------------
>   1 file changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index 97f445f3366a..1efb4a5c5ab3 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -1406,35 +1406,39 @@ int __init sev_es_efi_map_ghcbs(pgd_t *pgd)
>   	return 0;
>   }
>   
> +/* Writes to the SVSM CAA MSR are ignored */
> +static enum es_result __vc_handle_msr_caa(struct pt_regs *regs, bool write)
> +{
> +	if (write)
> +		return ES_OK;
> +
> +	regs->ax = lower_32_bits(this_cpu_read(svsm_caa_pa));
> +	regs->dx = upper_32_bits(this_cpu_read(svsm_caa_pa));
> +
> +	return ES_OK;
> +}
> +
>   static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
>   {
>   	struct pt_regs *regs = ctxt->regs;
>   	enum es_result ret;
> -	u64 exit_info_1;
> +	bool write;
>   
>   	/* Is it a WRMSR? */
> -	exit_info_1 = (ctxt->insn.opcode.bytes[1] == 0x30) ? 1 : 0;
> -
> -	if (regs->cx == MSR_SVSM_CAA) {
> -		/* Writes to the SVSM CAA msr are ignored */
> -		if (exit_info_1)
> -			return ES_OK;
> -
> -		regs->ax = lower_32_bits(this_cpu_read(svsm_caa_pa));
> -		regs->dx = upper_32_bits(this_cpu_read(svsm_caa_pa));
> +	write = ctxt->insn.opcode.bytes[1] == 0x30;
>   
> -		return ES_OK;
> -	}
> +	if (regs->cx == MSR_SVSM_CAA)
> +		return __vc_handle_msr_caa(regs, write);
>   
>   	ghcb_set_rcx(ghcb, regs->cx);
> -	if (exit_info_1) {
> +	if (write) {
>   		ghcb_set_rax(ghcb, regs->ax);
>   		ghcb_set_rdx(ghcb, regs->dx);
>   	}
>   
> -	ret = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_MSR, exit_info_1, 0);
> +	ret = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_MSR, !!write, 0);
>   
> -	if ((ret == ES_OK) && (!exit_info_1)) {
> +	if ((ret == ES_OK) && (!write)) {
>   		regs->ax = ghcb->save.rax;
>   		regs->dx = ghcb->save.rdx;
>   	}

[tip: x86/sev] x86/sev: Cleanup vc_handle_msr()

[tip-bot2 for Borislav Petkov (AMD)]

The following commit has been merged into the x86/sev branch of tip:

Commit-ID:     8bca85cc1eb72e21a3544ab32e546a819d8674ca
Gitweb:        https://git.kernel.org/tip/8bca85cc1eb72e21a3544ab32e546a819d8674ca
Author:        Borislav Petkov (AMD) <bp@alien8.de>
AuthorDate:    Wed, 06 Nov 2024 18:21:58 +01:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Thu, 07 Nov 2024 12:10:01 +01:00

x86/sev: Cleanup vc_handle_msr()

Carve out the MSR_SVSM_CAA into a helper with the suggestion that
upcoming future users should do the same. Rename that silly exit_info_1
into what it actually means in this function - whether the MSR access is
a read or a write.

No functional changes.

Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Reviewed-by: Pankaj Gupta <pankaj.gupta@amd.com>
Link: https://lore.kernel.org/r/20241106172647.GAZyum1zngPDwyD2IJ@fat_crate.local
---
 arch/x86/coco/sev/core.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 97f445f..c5b0148 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -1406,35 +1406,39 @@ int __init sev_es_efi_map_ghcbs(pgd_t *pgd)
 	return 0;
 }
 
+/* Writes to the SVSM CAA MSR are ignored */
+static enum es_result __vc_handle_msr_caa(struct pt_regs *regs, bool write)
+{
+	if (write)
+		return ES_OK;
+
+	regs->ax = lower_32_bits(this_cpu_read(svsm_caa_pa));
+	regs->dx = upper_32_bits(this_cpu_read(svsm_caa_pa));
+
+	return ES_OK;
+}
+
 static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
 {
 	struct pt_regs *regs = ctxt->regs;
 	enum es_result ret;
-	u64 exit_info_1;
+	bool write;
 
 	/* Is it a WRMSR? */
-	exit_info_1 = (ctxt->insn.opcode.bytes[1] == 0x30) ? 1 : 0;
-
-	if (regs->cx == MSR_SVSM_CAA) {
-		/* Writes to the SVSM CAA msr are ignored */
-		if (exit_info_1)
-			return ES_OK;
-
-		regs->ax = lower_32_bits(this_cpu_read(svsm_caa_pa));
-		regs->dx = upper_32_bits(this_cpu_read(svsm_caa_pa));
+	write = ctxt->insn.opcode.bytes[1] == 0x30;
 
-		return ES_OK;
-	}
+	if (regs->cx == MSR_SVSM_CAA)
+		return __vc_handle_msr_caa(regs, write);
 
 	ghcb_set_rcx(ghcb, regs->cx);
-	if (exit_info_1) {
+	if (write) {
 		ghcb_set_rax(ghcb, regs->ax);
 		ghcb_set_rdx(ghcb, regs->dx);
 	}
 
-	ret = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_MSR, exit_info_1, 0);
+	ret = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_MSR, write, 0);
 
-	if ((ret == ES_OK) && (!exit_info_1)) {
+	if ((ret == ES_OK) && !write) {
 		regs->ax = ghcb->save.rax;
 		regs->dx = ghcb->save.rdx;
 	}