Re: [PATCH v1 4/8] x86/sev: Route unsupported APIC register accesses to the hypervisor APIC emulation

[Borislav Petkov]

On Sat, Aug 29, 2026 at 03:39:42AM +0000, Melody Wang wrote:
> -u64 savic_ghcb_msr_read(u32 reg)
> +u64 sev_apic_ghcb_msr_read(u32 reg)
>  {
>  	u64 msr = APIC_BASE_MSR + (reg >> 4);
>  	struct pt_regs regs = { .cx = msr };
> @@ -988,9 +988,9 @@ u64 savic_ghcb_msr_read(u32 reg)
>  
>  	res = __vc_handle_msr(ghcb, &ctxt, false);
>  	if (res != ES_OK) {
> -		pr_err("Secure AVIC MSR (0x%llx) read returned error (%d)\n", msr, res);
> +		pr_err("APIC MSR via GHCB (0x%llx) read returned error (%d)\n", msr, res);
>  		/* MSR read failures are treated as fatal errors */
> -		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_SAVIC_FAIL);
> +		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_APIC_MSR_FAIL);
>  	}
>  
>  	__sev_put_ghcb(&state);
> @@ -998,7 +998,7 @@ u64 savic_ghcb_msr_read(u32 reg)
>  	return regs.ax | regs.dx << 32;
>  }
>  
> -void savic_ghcb_msr_write(u32 reg, u64 value)
> +void sev_apic_ghcb_msr_write(u32 reg, u64 value)
>  {
>  	u64 msr = APIC_BASE_MSR + (reg >> 4);
>  	struct pt_regs regs = {
> @@ -1018,9 +1018,9 @@ void savic_ghcb_msr_write(u32 reg, u64 value)
>  
>  	res = __vc_handle_msr(ghcb, &ctxt, true);
>  	if (res != ES_OK) {
> -		pr_err("Secure AVIC MSR (0x%llx) write returned error (%d)\n", msr, res);
> +		pr_err("APIC MSR via GHCB (0x%llx) write returned error (%d)\n", msr, res);
>  		/* MSR writes should never fail. Any failure is fatal error for SNP guest */
> -		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_SAVIC_FAIL);
> +		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_APIC_MSR_FAIL);
>  	}
>  
>  	__sev_put_ghcb(&state);

Those two functions look almost identical. I think you should unify them in
a pre-patch exactly like you did __svsm_apic_msr_rw().

...

> diff --git a/arch/x86/kernel/apic/x2apic_svsm.c b/arch/x86/kernel/apic/x2apic_svsm.c
> index b553bfde4b85..1a1aefc26726 100644
> --- a/arch/x86/kernel/apic/x2apic_svsm.c
> +++ b/arch/x86/kernel/apic/x2apic_svsm.c
> @@ -69,14 +69,17 @@ static u32 __svsm_apic_msr_rw(u32 reg, u32 v, bool write)
>  		call.rdx = v;
>  
>  		ret = svsm_perform_call_protocol(&call);
> -		if (ret) {
> +		if (ret) { 

Applying: x86/sev: Route unsupported APIC register accesses to the hypervisor APIC emulation
.git/rebase-apply/patch:142: trailing whitespace.
                if (ret) { 
warning: 1 line adds whitespace errors.

This looks like a change you made by mistake while reworking.

>  			pr_err("%s: 0x%x, error: %d\n", call_reg_str, reg, ret);
>  			sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_ALT_INJ_FAIL);
>  		}
>  		break;
>  	default:
> -		pr_err("%s 0x%x not supported\n", call_reg_str, reg);
> -		return 0;
> +		if (write) {
> +			sev_apic_ghcb_msr_write(reg, v);
> +		} else {
> +			return sev_apic_ghcb_msr_read(reg);
> +		}

You don't need the { } braces for single statements. And you can do:

diff --git a/arch/x86/kernel/apic/x2apic_svsm.c b/arch/x86/kernel/apic/x2apic_svsm.c
index 2de2c243b879..d3867a669e0f 100644
--- a/arch/x86/kernel/apic/x2apic_svsm.c
+++ b/arch/x86/kernel/apic/x2apic_svsm.c
@@ -76,6 +76,10 @@ static u32 __svsm_apic_msr_rw(u32 reg, u32 v, bool write)
 		break;
 	default:
 		if (write)
+			/*
+			 * Even if this will end up returning call.rdx_out,
+			 * that is being ignored by the caller for a write.
+			 */
 			sev_apic_ghcb_msr_write(reg, v);
 		else
 			return sev_apic_ghcb_msr_read(reg);

-- 
Regards/Gruss,
    Boris.

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