* Re: [RFC PATCH] x86/sev: Cleanup vc_handle_msr()
2024-11-06 17:26 [RFC PATCH] x86/sev: Cleanup vc_handle_msr() Borislav Petkov (AMD)
@ 2024-11-06 19:40 ` Tom Lendacky
2024-11-06 19:47 ` Borislav Petkov
2024-11-07 11:02 ` Gupta, Pankaj
2024-11-07 11:26 ` [tip: x86/sev] " tip-bot2 for Borislav Petkov (AMD)
2 siblings, 1 reply; 5+ messages in thread
From: Tom Lendacky @ 2024-11-06 19:40 UTC (permalink / raw)
To: Borislav Petkov (AMD); +Cc: x86-ml, lkml, linux-coco
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;
> }
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [RFC PATCH] x86/sev: Cleanup vc_handle_msr()
2024-11-06 19:40 ` Tom Lendacky
@ 2024-11-06 19:47 ` Borislav Petkov
0 siblings, 0 replies; 5+ messages in thread
From: Borislav Petkov @ 2024-11-06 19:47 UTC (permalink / raw)
To: Tom Lendacky; +Cc: x86-ml, lkml, linux-coco
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] x86/sev: Cleanup vc_handle_msr()
2024-11-06 17:26 [RFC PATCH] x86/sev: Cleanup vc_handle_msr() Borislav Petkov (AMD)
2024-11-06 19:40 ` Tom Lendacky
@ 2024-11-07 11:02 ` Gupta, Pankaj
2024-11-07 11:26 ` [tip: x86/sev] " tip-bot2 for Borislav Petkov (AMD)
2 siblings, 0 replies; 5+ messages in thread
From: Gupta, Pankaj @ 2024-11-07 11:02 UTC (permalink / raw)
To: Borislav Petkov (AMD), Tom Lendacky; +Cc: x86-ml, lkml, linux-coco
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;
> }
^ permalink raw reply [flat|nested] 5+ messages in thread* [tip: x86/sev] x86/sev: Cleanup vc_handle_msr()
2024-11-06 17:26 [RFC PATCH] x86/sev: Cleanup vc_handle_msr() Borislav Petkov (AMD)
2024-11-06 19:40 ` Tom Lendacky
2024-11-07 11:02 ` Gupta, Pankaj
@ 2024-11-07 11:26 ` tip-bot2 for Borislav Petkov (AMD)
2 siblings, 0 replies; 5+ messages in thread
From: tip-bot2 for Borislav Petkov (AMD) @ 2024-11-07 11:26 UTC (permalink / raw)
To: linux-tip-commits
Cc: Borislav Petkov (AMD), Tom Lendacky, Pankaj Gupta, x86, linux-kernel
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;
}
^ permalink raw reply [flat|nested] 5+ messages in thread