From: Ard Biesheuvel <ardb@kernel.org>
There are two distinct callers of snp_cpuid(): one where the MSR
protocol is always used, and one where the GHCB page based interface is
always used.
The snp_cpuid() logic does not care about the distinction, which only
matters at a lower level. But the fact that it supports both interfaces
means that the GHCB page based logic is pulled into the early startup
code where PA to VA conversions are problematic, given that it runs from
the 1:1 mapping of memory.
So keep snp_cpuid() itself in the startup code, but factor out the
hypervisor calls via a callback, so that the GHCB page handling can be
moved out.
Code refactoring only - no functional change intended.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
arch/x86/boot/startup/sev-shared.c | 64 ++++----------------
arch/x86/coco/sev/vc-shared.c | 49 ++++++++++++++-
arch/x86/include/asm/sev.h | 3 +-
3 files changed, 63 insertions(+), 53 deletions(-)
diff --git a/arch/x86/boot/startup/sev-shared.c b/arch/x86/boot/startup/sev-shared.c
index 7a706db87b93..992abfa50508 100644
--- a/arch/x86/boot/startup/sev-shared.c
+++ b/arch/x86/boot/startup/sev-shared.c
@@ -319,7 +319,7 @@ static int __sev_cpuid_hv(u32 fn, int reg_idx, u32 *reg)
return 0;
}
-static int __sev_cpuid_hv_msr(struct cpuid_leaf *leaf)
+static int __sev_cpuid_msr_prot(struct cpuid_leaf *leaf)
{
int ret;
@@ -342,44 +342,7 @@ static int __sev_cpuid_hv_msr(struct cpuid_leaf *leaf)
return ret;
}
-static int __sev_cpuid_hv_ghcb(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf)
-{
- u32 cr4 = native_read_cr4();
- int ret;
-
- ghcb_set_rax(ghcb, leaf->fn);
- ghcb_set_rcx(ghcb, leaf->subfn);
-
- if (cr4 & X86_CR4_OSXSAVE)
- /* Safe to read xcr0 */
- ghcb_set_xcr0(ghcb, xgetbv(XCR_XFEATURE_ENABLED_MASK));
- else
- /* xgetbv will cause #UD - use reset value for xcr0 */
- ghcb_set_xcr0(ghcb, 1);
-
- ret = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_CPUID, 0, 0);
- if (ret != ES_OK)
- return ret;
-
- if (!(ghcb_rax_is_valid(ghcb) &&
- ghcb_rbx_is_valid(ghcb) &&
- ghcb_rcx_is_valid(ghcb) &&
- ghcb_rdx_is_valid(ghcb)))
- return ES_VMM_ERROR;
- leaf->eax = ghcb->save.rax;
- leaf->ebx = ghcb->save.rbx;
- leaf->ecx = ghcb->save.rcx;
- leaf->edx = ghcb->save.rdx;
-
- return ES_OK;
-}
-
-static int sev_cpuid_hv(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf)
-{
- return ghcb ? __sev_cpuid_hv_ghcb(ghcb, ctxt, leaf)
- : __sev_cpuid_hv_msr(leaf);
-}
/*
* This may be called early while still running on the initial identity
@@ -484,21 +447,20 @@ snp_cpuid_get_validated_func(struct cpuid_leaf *leaf)
return false;
}
-static void snp_cpuid_hv(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf)
+static void snp_cpuid_msr_prot(void *ctx, struct cpuid_leaf *leaf)
{
- if (sev_cpuid_hv(ghcb, ctxt, leaf))
+ if (__sev_cpuid_msr_prot(leaf))
sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_CPUID_HV);
}
-static int __head
-snp_cpuid_postprocess(struct ghcb *ghcb, struct es_em_ctxt *ctxt,
- struct cpuid_leaf *leaf)
+static int __head snp_cpuid_postprocess(void (*cpuid)(void *ctx, struct cpuid_leaf *),
+ void *ctx, struct cpuid_leaf *leaf)
{
struct cpuid_leaf leaf_hv = *leaf;
switch (leaf->fn) {
case 0x1:
- snp_cpuid_hv(ghcb, ctxt, &leaf_hv);
+ cpuid(ctx, &leaf_hv);
/* initial APIC ID */
leaf->ebx = (leaf_hv.ebx & GENMASK(31, 24)) | (leaf->ebx & GENMASK(23, 0));
@@ -517,7 +479,7 @@ snp_cpuid_postprocess(struct ghcb *ghcb, struct es_em_ctxt *ctxt,
break;
case 0xB:
leaf_hv.subfn = 0;
- snp_cpuid_hv(ghcb, ctxt, &leaf_hv);
+ cpuid(ctx, &leaf_hv);
/* extended APIC ID */
leaf->edx = leaf_hv.edx;
@@ -565,7 +527,7 @@ snp_cpuid_postprocess(struct ghcb *ghcb, struct es_em_ctxt *ctxt,
}
break;
case 0x8000001E:
- snp_cpuid_hv(ghcb, ctxt, &leaf_hv);
+ cpuid(ctx, &leaf_hv);
/* extended APIC ID */
leaf->eax = leaf_hv.eax;
@@ -586,8 +548,8 @@ snp_cpuid_postprocess(struct ghcb *ghcb, struct es_em_ctxt *ctxt,
* Returns -EOPNOTSUPP if feature not enabled. Any other non-zero return value
* should be treated as fatal by caller.
*/
-int __head
-snp_cpuid(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf)
+int __head snp_cpuid(void (*cpuid)(void *ctx, struct cpuid_leaf *), void *ctx,
+ struct cpuid_leaf *leaf)
{
const struct snp_cpuid_table *cpuid_table = snp_cpuid_get_table();
@@ -621,7 +583,7 @@ snp_cpuid(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf)
return 0;
}
- return snp_cpuid_postprocess(ghcb, ctxt, leaf);
+ return snp_cpuid_postprocess(cpuid, ctx, leaf);
}
/*
@@ -648,14 +610,14 @@ void __head do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
leaf.fn = fn;
leaf.subfn = subfn;
- ret = snp_cpuid(NULL, NULL, &leaf);
+ ret = snp_cpuid(snp_cpuid_msr_prot, NULL, &leaf);
if (!ret)
goto cpuid_done;
if (ret != -EOPNOTSUPP)
goto fail;
- if (__sev_cpuid_hv_msr(&leaf))
+ if (__sev_cpuid_msr_prot(&leaf))
goto fail;
cpuid_done:
diff --git a/arch/x86/coco/sev/vc-shared.c b/arch/x86/coco/sev/vc-shared.c
index 2c0ab0fdc060..776cb90be530 100644
--- a/arch/x86/coco/sev/vc-shared.c
+++ b/arch/x86/coco/sev/vc-shared.c
@@ -409,15 +409,62 @@ static enum es_result vc_handle_ioio(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
return ret;
}
+static int __sev_cpuid_ghcb_prot(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf)
+{
+ u32 cr4 = native_read_cr4();
+ int ret;
+
+ ghcb_set_rax(ghcb, leaf->fn);
+ ghcb_set_rcx(ghcb, leaf->subfn);
+
+ if (cr4 & X86_CR4_OSXSAVE)
+ /* Safe to read xcr0 */
+ ghcb_set_xcr0(ghcb, xgetbv(XCR_XFEATURE_ENABLED_MASK));
+ else
+ /* xgetbv will cause #UD - use reset value for xcr0 */
+ ghcb_set_xcr0(ghcb, 1);
+
+ ret = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_CPUID, 0, 0);
+ if (ret != ES_OK)
+ return ret;
+
+ if (!(ghcb_rax_is_valid(ghcb) &&
+ ghcb_rbx_is_valid(ghcb) &&
+ ghcb_rcx_is_valid(ghcb) &&
+ ghcb_rdx_is_valid(ghcb)))
+ return ES_VMM_ERROR;
+
+ leaf->eax = ghcb->save.rax;
+ leaf->ebx = ghcb->save.rbx;
+ leaf->ecx = ghcb->save.rcx;
+ leaf->edx = ghcb->save.rdx;
+
+ return ES_OK;
+}
+
+struct cpuid_ctx {
+ struct ghcb *ghcb;
+ struct es_em_ctxt *ctxt;
+};
+
+static void snp_cpuid_ghcb_prot(void *p, struct cpuid_leaf *leaf)
+{
+ struct cpuid_ctx *ctx = p;
+
+ if (__sev_cpuid_ghcb_prot(ctx->ghcb, ctx->ctxt, leaf))
+ sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_CPUID_HV);
+}
+
static int vc_handle_cpuid_snp(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
{
+ struct cpuid_ctx ctx = { ghcb, ctxt };
struct pt_regs *regs = ctxt->regs;
struct cpuid_leaf leaf;
int ret;
leaf.fn = regs->ax;
leaf.subfn = regs->cx;
- ret = snp_cpuid(ghcb, ctxt, &leaf);
+ ret = snp_cpuid(snp_cpuid_ghcb_prot, &ctx, &leaf);
if (!ret) {
regs->ax = leaf.eax;
regs->bx = leaf.ebx;
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 89075ff19afa..2cabf617de3c 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -552,7 +552,8 @@ struct cpuid_leaf {
u32 edx;
};
-int snp_cpuid(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf);
+int snp_cpuid(void (*cpuid_hv)(void *ctx, struct cpuid_leaf *),
+ void *ctx, struct cpuid_leaf *leaf);
void __noreturn sev_es_terminate(unsigned int set, unsigned int reason);
enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb,
--
2.50.0.727.gbf7dc18ff4-googOn 7/9/25 03:08, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
>
> There are two distinct callers of snp_cpuid(): one where the MSR
> protocol is always used, and one where the GHCB page based interface is
> always used.
>
> The snp_cpuid() logic does not care about the distinction, which only
> matters at a lower level. But the fact that it supports both interfaces
> means that the GHCB page based logic is pulled into the early startup
> code where PA to VA conversions are problematic, given that it runs from
> the 1:1 mapping of memory.
>
> So keep snp_cpuid() itself in the startup code, but factor out the
> hypervisor calls via a callback, so that the GHCB page handling can be
> moved out.
>
> Code refactoring only - no functional change intended.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> arch/x86/boot/startup/sev-shared.c | 64 ++++----------------
> arch/x86/coco/sev/vc-shared.c | 49 ++++++++++++++-
> arch/x86/include/asm/sev.h | 3 +-
> 3 files changed, 63 insertions(+), 53 deletions(-)
>
> diff --git a/arch/x86/boot/startup/sev-shared.c b/arch/x86/boot/startup/sev-shared.c
> index 7a706db87b93..992abfa50508 100644
> --- a/arch/x86/boot/startup/sev-shared.c
> +++ b/arch/x86/boot/startup/sev-shared.c
> @@ -319,7 +319,7 @@ static int __sev_cpuid_hv(u32 fn, int reg_idx, u32 *reg)
> return 0;
> }
>
> -static int __sev_cpuid_hv_msr(struct cpuid_leaf *leaf)
> +static int __sev_cpuid_msr_prot(struct cpuid_leaf *leaf)
Not sure the renaming makes it read any easier or say anything more. It
does add extra changes to the diff that have to be read through, though,
so I don't think it is beneficial.
> {
> int ret;
>
> @@ -342,44 +342,7 @@ static int __sev_cpuid_hv_msr(struct cpuid_leaf *leaf)
> return ret;
> }
>
> -static int __sev_cpuid_hv_ghcb(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf)
> -{
> - u32 cr4 = native_read_cr4();
> - int ret;
> -
> - ghcb_set_rax(ghcb, leaf->fn);
> - ghcb_set_rcx(ghcb, leaf->subfn);
> -
> - if (cr4 & X86_CR4_OSXSAVE)
> - /* Safe to read xcr0 */
> - ghcb_set_xcr0(ghcb, xgetbv(XCR_XFEATURE_ENABLED_MASK));
> - else
> - /* xgetbv will cause #UD - use reset value for xcr0 */
> - ghcb_set_xcr0(ghcb, 1);
> -
> - ret = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_CPUID, 0, 0);
> - if (ret != ES_OK)
> - return ret;
> -
> - if (!(ghcb_rax_is_valid(ghcb) &&
> - ghcb_rbx_is_valid(ghcb) &&
> - ghcb_rcx_is_valid(ghcb) &&
> - ghcb_rdx_is_valid(ghcb)))
> - return ES_VMM_ERROR;
>
> - leaf->eax = ghcb->save.rax;
> - leaf->ebx = ghcb->save.rbx;
> - leaf->ecx = ghcb->save.rcx;
> - leaf->edx = ghcb->save.rdx;
> -
> - return ES_OK;
> -}
> -
> -static int sev_cpuid_hv(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf)
> -{
> - return ghcb ? __sev_cpuid_hv_ghcb(ghcb, ctxt, leaf)
> - : __sev_cpuid_hv_msr(leaf);
> -}
>
> /*
> * This may be called early while still running on the initial identity
> @@ -484,21 +447,20 @@ snp_cpuid_get_validated_func(struct cpuid_leaf *leaf)
> return false;
> }
>
> -static void snp_cpuid_hv(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf)
> +static void snp_cpuid_msr_prot(void *ctx, struct cpuid_leaf *leaf)
> {
> - if (sev_cpuid_hv(ghcb, ctxt, leaf))
> + if (__sev_cpuid_msr_prot(leaf))
> sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_CPUID_HV);
> }
>
> -static int __head
> -snp_cpuid_postprocess(struct ghcb *ghcb, struct es_em_ctxt *ctxt,
> - struct cpuid_leaf *leaf)
> +static int __head snp_cpuid_postprocess(void (*cpuid)(void *ctx, struct cpuid_leaf *),
> + void *ctx, struct cpuid_leaf *leaf)
> {
> struct cpuid_leaf leaf_hv = *leaf;
>
> switch (leaf->fn) {
> case 0x1:
> - snp_cpuid_hv(ghcb, ctxt, &leaf_hv);
> + cpuid(ctx, &leaf_hv);
Maybe rename this parameter to snp_cpuid or snp_cpuid_fn or similar,
because it can be very confusing to see "cpuid" on its own like this.
>
> /* initial APIC ID */
> leaf->ebx = (leaf_hv.ebx & GENMASK(31, 24)) | (leaf->ebx & GENMASK(23, 0));
> @@ -517,7 +479,7 @@ snp_cpuid_postprocess(struct ghcb *ghcb, struct es_em_ctxt *ctxt,
> break;
> case 0xB:
> leaf_hv.subfn = 0;
> - snp_cpuid_hv(ghcb, ctxt, &leaf_hv);
> + cpuid(ctx, &leaf_hv);
>
> /* extended APIC ID */
> leaf->edx = leaf_hv.edx;
> @@ -565,7 +527,7 @@ snp_cpuid_postprocess(struct ghcb *ghcb, struct es_em_ctxt *ctxt,
> }
> break;
> case 0x8000001E:
> - snp_cpuid_hv(ghcb, ctxt, &leaf_hv);
> + cpuid(ctx, &leaf_hv);
>
> /* extended APIC ID */
> leaf->eax = leaf_hv.eax;
> @@ -586,8 +548,8 @@ snp_cpuid_postprocess(struct ghcb *ghcb, struct es_em_ctxt *ctxt,
> * Returns -EOPNOTSUPP if feature not enabled. Any other non-zero return value
> * should be treated as fatal by caller.
> */
> -int __head
> -snp_cpuid(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf)
> +int __head snp_cpuid(void (*cpuid)(void *ctx, struct cpuid_leaf *), void *ctx,
> + struct cpuid_leaf *leaf)
> {
> const struct snp_cpuid_table *cpuid_table = snp_cpuid_get_table();
>
> @@ -621,7 +583,7 @@ snp_cpuid(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf)
> return 0;
> }
>
> - return snp_cpuid_postprocess(ghcb, ctxt, leaf);
> + return snp_cpuid_postprocess(cpuid, ctx, leaf);
> }
>
> /*
> @@ -648,14 +610,14 @@ void __head do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
> leaf.fn = fn;
> leaf.subfn = subfn;
>
> - ret = snp_cpuid(NULL, NULL, &leaf);
> + ret = snp_cpuid(snp_cpuid_msr_prot, NULL, &leaf);
> if (!ret)
> goto cpuid_done;
>
> if (ret != -EOPNOTSUPP)
> goto fail;
>
> - if (__sev_cpuid_hv_msr(&leaf))
> + if (__sev_cpuid_msr_prot(&leaf))
> goto fail;
>
> cpuid_done:
> diff --git a/arch/x86/coco/sev/vc-shared.c b/arch/x86/coco/sev/vc-shared.c
> index 2c0ab0fdc060..776cb90be530 100644
> --- a/arch/x86/coco/sev/vc-shared.c
> +++ b/arch/x86/coco/sev/vc-shared.c
> @@ -409,15 +409,62 @@ static enum es_result vc_handle_ioio(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
> return ret;
> }
>
> +static int __sev_cpuid_ghcb_prot(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf)
Ditto here and below, keeping the __sev_cpuid_hv_ghcb() /
sev_cpuid_hv_ghcb() name would be best.
Thanks,
Tom
> +{
> + u32 cr4 = native_read_cr4();
> + int ret;
> +
> + ghcb_set_rax(ghcb, leaf->fn);
> + ghcb_set_rcx(ghcb, leaf->subfn);
> +
> + if (cr4 & X86_CR4_OSXSAVE)
> + /* Safe to read xcr0 */
> + ghcb_set_xcr0(ghcb, xgetbv(XCR_XFEATURE_ENABLED_MASK));
> + else
> + /* xgetbv will cause #UD - use reset value for xcr0 */
> + ghcb_set_xcr0(ghcb, 1);
> +
> + ret = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_CPUID, 0, 0);
> + if (ret != ES_OK)
> + return ret;
> +
> + if (!(ghcb_rax_is_valid(ghcb) &&
> + ghcb_rbx_is_valid(ghcb) &&
> + ghcb_rcx_is_valid(ghcb) &&
> + ghcb_rdx_is_valid(ghcb)))
> + return ES_VMM_ERROR;
> +
> + leaf->eax = ghcb->save.rax;
> + leaf->ebx = ghcb->save.rbx;
> + leaf->ecx = ghcb->save.rcx;
> + leaf->edx = ghcb->save.rdx;
> +
> + return ES_OK;
> +}
> +
> +struct cpuid_ctx {
> + struct ghcb *ghcb;
> + struct es_em_ctxt *ctxt;
> +};
> +
> +static void snp_cpuid_ghcb_prot(void *p, struct cpuid_leaf *leaf)
> +{
> + struct cpuid_ctx *ctx = p;
> +
> + if (__sev_cpuid_ghcb_prot(ctx->ghcb, ctx->ctxt, leaf))
> + sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_CPUID_HV);
> +}
> +
> static int vc_handle_cpuid_snp(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
> {
> + struct cpuid_ctx ctx = { ghcb, ctxt };
> struct pt_regs *regs = ctxt->regs;
> struct cpuid_leaf leaf;
> int ret;
>
> leaf.fn = regs->ax;
> leaf.subfn = regs->cx;
> - ret = snp_cpuid(ghcb, ctxt, &leaf);
> + ret = snp_cpuid(snp_cpuid_ghcb_prot, &ctx, &leaf);
> if (!ret) {
> regs->ax = leaf.eax;
> regs->bx = leaf.ebx;
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 89075ff19afa..2cabf617de3c 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -552,7 +552,8 @@ struct cpuid_leaf {
> u32 edx;
> };
>
> -int snp_cpuid(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf);
> +int snp_cpuid(void (*cpuid_hv)(void *ctx, struct cpuid_leaf *),
> + void *ctx, struct cpuid_leaf *leaf);
>
> void __noreturn sev_es_terminate(unsigned int set, unsigned int reason);
> enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb,
On Wed, Jul 09, 2025 at 10:12:48AM -0500, Tom Lendacky wrote:
> Not sure the renaming makes it read any easier or say anything more. It
> does add extra changes to the diff that have to be read through, though,
> so I don't think it is beneficial.
So it really comes natural to split them into a msr_prot and a ghcb_prot
variant. If we added a separate patch ontop that does only the renaming, then
that would probably be more churn than necessary.
> Maybe rename this parameter to snp_cpuid or snp_cpuid_fn or similar,
> because it can be very confusing to see "cpuid" on its own like this.a
Yeah, that's a good point - snp_cpuid_fn clearly states that it is a function
pointer and not *the* cpuid() function.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 7/11/25 15:59, Borislav Petkov wrote: > On Wed, Jul 09, 2025 at 10:12:48AM -0500, Tom Lendacky wrote: >> Not sure the renaming makes it read any easier or say anything more. It >> does add extra changes to the diff that have to be read through, though, >> so I don't think it is beneficial. > > So it really comes natural to split them into a msr_prot and a ghcb_prot > variant. If we added a separate patch ontop that does only the renaming, then > that would probably be more churn than necessary. Right, they already are though: __sev_cpuid_hv_msr() and __sev_cpuid_hv_ghcb() the first one meaning that the hypervisor is being called using the msr protocol and the second one meaning that the hypervisor is being called using the ghcb protocol. That's why I made the comment. Just changing __sev_cpuid_hv_msr() to __sev_cpuid_msr_prot() isn't saying anything more in my opinion. Thanks, Tom > >> Maybe rename this parameter to snp_cpuid or snp_cpuid_fn or similar, >> because it can be very confusing to see "cpuid" on its own like this.a > > Yeah, that's a good point - snp_cpuid_fn clearly states that it is a function > pointer and not *the* cpuid() function. > > Thx. >
On Sat, Jul 12, 2025 at 09:54:20AM -0500, Tom Lendacky wrote:
> > So it really comes natural to split them into a msr_prot and a ghcb_prot
> > variant. If we added a separate patch ontop that does only the renaming, then
> > that would probably be more churn than necessary.
>
> Right, they already are though:
>
> __sev_cpuid_hv_msr() and __sev_cpuid_hv_ghcb()
>
> the first one meaning that the hypervisor is being called using the msr
> protocol and the second one meaning that the hypervisor is being called
> using the ghcb protocol.
>
> That's why I made the comment. Just changing
>
> __sev_cpuid_hv_msr() to __sev_cpuid_msr_prot()
>
> isn't saying anything more in my opinion.
Ok, then let's keep 'em that way. I was reacting to snp_cpuid_hv_no_ghcb()
which is snp_cpuid_hv_msr I guess.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, 10 Jul 2025 at 01:12, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 7/9/25 03:08, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <ardb@kernel.org>
> >
> > There are two distinct callers of snp_cpuid(): one where the MSR
> > protocol is always used, and one where the GHCB page based interface is
> > always used.
> >
> > The snp_cpuid() logic does not care about the distinction, which only
> > matters at a lower level. But the fact that it supports both interfaces
> > means that the GHCB page based logic is pulled into the early startup
> > code where PA to VA conversions are problematic, given that it runs from
> > the 1:1 mapping of memory.
> >
> > So keep snp_cpuid() itself in the startup code, but factor out the
> > hypervisor calls via a callback, so that the GHCB page handling can be
> > moved out.
> >
> > Code refactoring only - no functional change intended.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> > arch/x86/boot/startup/sev-shared.c | 64 ++++----------------
> > arch/x86/coco/sev/vc-shared.c | 49 ++++++++++++++-
> > arch/x86/include/asm/sev.h | 3 +-
> > 3 files changed, 63 insertions(+), 53 deletions(-)
> >
> > diff --git a/arch/x86/boot/startup/sev-shared.c b/arch/x86/boot/startup/sev-shared.c
> > index 7a706db87b93..992abfa50508 100644
> > --- a/arch/x86/boot/startup/sev-shared.c
> > +++ b/arch/x86/boot/startup/sev-shared.c
> > @@ -319,7 +319,7 @@ static int __sev_cpuid_hv(u32 fn, int reg_idx, u32 *reg)
> > return 0;
> > }
> >
> > -static int __sev_cpuid_hv_msr(struct cpuid_leaf *leaf)
> > +static int __sev_cpuid_msr_prot(struct cpuid_leaf *leaf)
>
> Not sure the renaming makes it read any easier or say anything more. It
> does add extra changes to the diff that have to be read through, though,
> so I don't think it is beneficial.
>
These additional changes were provided by Boris as a delta patch on
top of my v3, so I will leave to him to respond to this.
https://lore.kernel.org/all/20250512190834.332684-24-ardb+git@google.com/T/#u
> > {
> > int ret;
> >
> > @@ -342,44 +342,7 @@ static int __sev_cpuid_hv_msr(struct cpuid_leaf *leaf)
> > return ret;
> > }
> >
> > -static int __sev_cpuid_hv_ghcb(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf)
> > -{
> > - u32 cr4 = native_read_cr4();
> > - int ret;
> > -
> > - ghcb_set_rax(ghcb, leaf->fn);
> > - ghcb_set_rcx(ghcb, leaf->subfn);
> > -
> > - if (cr4 & X86_CR4_OSXSAVE)
> > - /* Safe to read xcr0 */
> > - ghcb_set_xcr0(ghcb, xgetbv(XCR_XFEATURE_ENABLED_MASK));
> > - else
> > - /* xgetbv will cause #UD - use reset value for xcr0 */
> > - ghcb_set_xcr0(ghcb, 1);
> > -
> > - ret = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_CPUID, 0, 0);
> > - if (ret != ES_OK)
> > - return ret;
> > -
> > - if (!(ghcb_rax_is_valid(ghcb) &&
> > - ghcb_rbx_is_valid(ghcb) &&
> > - ghcb_rcx_is_valid(ghcb) &&
> > - ghcb_rdx_is_valid(ghcb)))
> > - return ES_VMM_ERROR;
> >
> > - leaf->eax = ghcb->save.rax;
> > - leaf->ebx = ghcb->save.rbx;
> > - leaf->ecx = ghcb->save.rcx;
> > - leaf->edx = ghcb->save.rdx;
> > -
> > - return ES_OK;
> > -}
> > -
> > -static int sev_cpuid_hv(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf)
> > -{
> > - return ghcb ? __sev_cpuid_hv_ghcb(ghcb, ctxt, leaf)
> > - : __sev_cpuid_hv_msr(leaf);
> > -}
> >
> > /*
> > * This may be called early while still running on the initial identity
> > @@ -484,21 +447,20 @@ snp_cpuid_get_validated_func(struct cpuid_leaf *leaf)
> > return false;
> > }
> >
> > -static void snp_cpuid_hv(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf)
> > +static void snp_cpuid_msr_prot(void *ctx, struct cpuid_leaf *leaf)
> > {
> > - if (sev_cpuid_hv(ghcb, ctxt, leaf))
> > + if (__sev_cpuid_msr_prot(leaf))
> > sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_CPUID_HV);
> > }
> >
> > -static int __head
> > -snp_cpuid_postprocess(struct ghcb *ghcb, struct es_em_ctxt *ctxt,
> > - struct cpuid_leaf *leaf)
> > +static int __head snp_cpuid_postprocess(void (*cpuid)(void *ctx, struct cpuid_leaf *),
> > + void *ctx, struct cpuid_leaf *leaf)
> > {
> > struct cpuid_leaf leaf_hv = *leaf;
> >
> > switch (leaf->fn) {
> > case 0x1:
> > - snp_cpuid_hv(ghcb, ctxt, &leaf_hv);
> > + cpuid(ctx, &leaf_hv);
>
> Maybe rename this parameter to snp_cpuid or snp_cpuid_fn or similar,
> because it can be very confusing to see "cpuid" on its own like this.
>
> >
> > /* initial APIC ID */
> > leaf->ebx = (leaf_hv.ebx & GENMASK(31, 24)) | (leaf->ebx & GENMASK(23, 0));
> > @@ -517,7 +479,7 @@ snp_cpuid_postprocess(struct ghcb *ghcb, struct es_em_ctxt *ctxt,
> > break;
> > case 0xB:
> > leaf_hv.subfn = 0;
> > - snp_cpuid_hv(ghcb, ctxt, &leaf_hv);
> > + cpuid(ctx, &leaf_hv);
> >
> > /* extended APIC ID */
> > leaf->edx = leaf_hv.edx;
> > @@ -565,7 +527,7 @@ snp_cpuid_postprocess(struct ghcb *ghcb, struct es_em_ctxt *ctxt,
> > }
> > break;
> > case 0x8000001E:
> > - snp_cpuid_hv(ghcb, ctxt, &leaf_hv);
> > + cpuid(ctx, &leaf_hv);
> >
> > /* extended APIC ID */
> > leaf->eax = leaf_hv.eax;
> > @@ -586,8 +548,8 @@ snp_cpuid_postprocess(struct ghcb *ghcb, struct es_em_ctxt *ctxt,
> > * Returns -EOPNOTSUPP if feature not enabled. Any other non-zero return value
> > * should be treated as fatal by caller.
> > */
> > -int __head
> > -snp_cpuid(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf)
> > +int __head snp_cpuid(void (*cpuid)(void *ctx, struct cpuid_leaf *), void *ctx,
> > + struct cpuid_leaf *leaf)
> > {
> > const struct snp_cpuid_table *cpuid_table = snp_cpuid_get_table();
> >
> > @@ -621,7 +583,7 @@ snp_cpuid(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf)
> > return 0;
> > }
> >
> > - return snp_cpuid_postprocess(ghcb, ctxt, leaf);
> > + return snp_cpuid_postprocess(cpuid, ctx, leaf);
> > }
> >
> > /*
> > @@ -648,14 +610,14 @@ void __head do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
> > leaf.fn = fn;
> > leaf.subfn = subfn;
> >
> > - ret = snp_cpuid(NULL, NULL, &leaf);
> > + ret = snp_cpuid(snp_cpuid_msr_prot, NULL, &leaf);
> > if (!ret)
> > goto cpuid_done;
> >
> > if (ret != -EOPNOTSUPP)
> > goto fail;
> >
> > - if (__sev_cpuid_hv_msr(&leaf))
> > + if (__sev_cpuid_msr_prot(&leaf))
> > goto fail;
> >
> > cpuid_done:
> > diff --git a/arch/x86/coco/sev/vc-shared.c b/arch/x86/coco/sev/vc-shared.c
> > index 2c0ab0fdc060..776cb90be530 100644
> > --- a/arch/x86/coco/sev/vc-shared.c
> > +++ b/arch/x86/coco/sev/vc-shared.c
> > @@ -409,15 +409,62 @@ static enum es_result vc_handle_ioio(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
> > return ret;
> > }
> >
> > +static int __sev_cpuid_ghcb_prot(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf)
>
> Ditto here and below, keeping the __sev_cpuid_hv_ghcb() /
> sev_cpuid_hv_ghcb() name would be best.
>
> Thanks,
> Tom
>
> > +{
> > + u32 cr4 = native_read_cr4();
> > + int ret;
> > +
> > + ghcb_set_rax(ghcb, leaf->fn);
> > + ghcb_set_rcx(ghcb, leaf->subfn);
> > +
> > + if (cr4 & X86_CR4_OSXSAVE)
> > + /* Safe to read xcr0 */
> > + ghcb_set_xcr0(ghcb, xgetbv(XCR_XFEATURE_ENABLED_MASK));
> > + else
> > + /* xgetbv will cause #UD - use reset value for xcr0 */
> > + ghcb_set_xcr0(ghcb, 1);
> > +
> > + ret = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_CPUID, 0, 0);
> > + if (ret != ES_OK)
> > + return ret;
> > +
> > + if (!(ghcb_rax_is_valid(ghcb) &&
> > + ghcb_rbx_is_valid(ghcb) &&
> > + ghcb_rcx_is_valid(ghcb) &&
> > + ghcb_rdx_is_valid(ghcb)))
> > + return ES_VMM_ERROR;
> > +
> > + leaf->eax = ghcb->save.rax;
> > + leaf->ebx = ghcb->save.rbx;
> > + leaf->ecx = ghcb->save.rcx;
> > + leaf->edx = ghcb->save.rdx;
> > +
> > + return ES_OK;
> > +}
> > +
> > +struct cpuid_ctx {
> > + struct ghcb *ghcb;
> > + struct es_em_ctxt *ctxt;
> > +};
> > +
> > +static void snp_cpuid_ghcb_prot(void *p, struct cpuid_leaf *leaf)
> > +{
> > + struct cpuid_ctx *ctx = p;
> > +
> > + if (__sev_cpuid_ghcb_prot(ctx->ghcb, ctx->ctxt, leaf))
> > + sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_CPUID_HV);
> > +}
> > +
> > static int vc_handle_cpuid_snp(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
> > {
> > + struct cpuid_ctx ctx = { ghcb, ctxt };
> > struct pt_regs *regs = ctxt->regs;
> > struct cpuid_leaf leaf;
> > int ret;
> >
> > leaf.fn = regs->ax;
> > leaf.subfn = regs->cx;
> > - ret = snp_cpuid(ghcb, ctxt, &leaf);
> > + ret = snp_cpuid(snp_cpuid_ghcb_prot, &ctx, &leaf);
> > if (!ret) {
> > regs->ax = leaf.eax;
> > regs->bx = leaf.ebx;
> > diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> > index 89075ff19afa..2cabf617de3c 100644
> > --- a/arch/x86/include/asm/sev.h
> > +++ b/arch/x86/include/asm/sev.h
> > @@ -552,7 +552,8 @@ struct cpuid_leaf {
> > u32 edx;
> > };
> >
> > -int snp_cpuid(struct ghcb *ghcb, struct es_em_ctxt *ctxt, struct cpuid_leaf *leaf);
> > +int snp_cpuid(void (*cpuid_hv)(void *ctx, struct cpuid_leaf *),
> > + void *ctx, struct cpuid_leaf *leaf);
> >
> > void __noreturn sev_es_terminate(unsigned int set, unsigned int reason);
> > enum es_result sev_es_ghcb_hv_call(struct ghcb *ghcb,
© 2016 - 2025 Red Hat, Inc.