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

[Tom Lendacky]

On 7/29/26 20:48, Melody Wang wrote:
> The SVSM APIC emulation supports only a subset of the X2APIC MSRs.
> Therefore, route the unsupported ones to the hypervisor's X2APIC
> emulation.

Explain a bit more why this is needed? How will the hypervisor use this,
maybe give an example.

> 
> Signed-off-by: Melody Wang <huibo.wang@xxxxxxx>
> ---
>  arch/x86/coco/sev/core.c            | 12 ++++++------
>  arch/x86/include/asm/sev-common.h   |  1 +
>  arch/x86/include/asm/sev.h          |  8 ++++----
>  arch/x86/kernel/apic/svsm_apic.c    | 11 +++++++----
>  arch/x86/kernel/apic/x2apic_savic.c |  8 ++++----

You could do the function rename as a separate pre-patch and then this
patch would be purley SVSM support. Up to you.

>  5 files changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index 197119807230..b293a36c3594 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -972,7 +972,7 @@ int __init sev_es_efi_map_ghcbs_cas(pgd_t *pgd)
>  	return 0;
>  }
>  
> -u64 savic_ghcb_msr_read(u32 reg)
> +u64 hvs_ghcb_msr_read(u32 reg)

Not sure that hvs is really that descriptive.

sev_apic_ghcb_msr_read ?

>  {
>  	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("Hypervisor MSR (0x%llx) read returned error (%d)\n", msr, res);

Same here with Hypervisor... maybe "APIC MSR via GHCB..." ?

>  		/* 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);

Alteratively you could pass in the code to use so that it is unique for
Secure AVIC and Alternate Injection.

>  	}
>  
>  	__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 hvs_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("Hypervisor MSR (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);
> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
> index 01a6e4dbe423..a41c52a16e31 100644
> --- a/arch/x86/include/asm/sev-common.h
> +++ b/arch/x86/include/asm/sev-common.h
> @@ -209,6 +209,7 @@ struct snp_psc_desc {
>  #define GHCB_TERM_SECURE_TSC		10	/* Secure TSC initialization failed */
>  #define GHCB_TERM_SVSM_CA_REMAP_FAIL	11	/* SVSM is present but CA could not be remapped */
>  #define GHCB_TERM_SAVIC_FAIL		12	/* Secure AVIC-specific failure */
> +#define GHCB_TERM_APIC_MSR_FAIL		13	/* APIC MSR failure */
>  
>  #define GHCB_RESP_CODE(v)		((v) & GHCB_MSR_INFO_MASK)
>  
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index f8a5b5cf939a..491a891a7694 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -549,8 +549,8 @@ void __init snp_secure_tsc_prepare(void);
>  void __init snp_secure_tsc_init(void);
>  enum es_result savic_register_gpa(u64 gpa);
>  enum es_result savic_unregister_gpa(u64 *gpa);
> -u64 savic_ghcb_msr_read(u32 reg);
> -void savic_ghcb_msr_write(u32 reg, u64 value);
> +u64 hvs_ghcb_msr_read(u32 reg);
> +void hvs_ghcb_msr_write(u32 reg, u64 value);
>  
>  static __always_inline void vc_ghcb_invalidate(struct ghcb *ghcb)
>  {
> @@ -659,8 +659,8 @@ static inline void __init snp_secure_tsc_init(void) { }
>  static inline void sev_evict_cache(void *va, int npages) {}
>  static inline enum es_result savic_register_gpa(u64 gpa) { return ES_UNSUPPORTED; }
>  static inline enum es_result savic_unregister_gpa(u64 *gpa) { return ES_UNSUPPORTED; }
> -static inline void savic_ghcb_msr_write(u32 reg, u64 value) { }
> -static inline u64 savic_ghcb_msr_read(u32 reg) { return 0; }
> +static inline void hvs_ghcb_msr_write(u32 reg, u64 value) { }
> +static inline u64 hvs_ghcb_msr_read(u32 reg) { return 0; }
>  
>  #endif	/* CONFIG_AMD_MEM_ENCRYPT */
>  
> diff --git a/arch/x86/kernel/apic/svsm_apic.c b/arch/x86/kernel/apic/svsm_apic.c
> index 7040a1ca8b55..a4e7ba7e9985 100644
> --- a/arch/x86/kernel/apic/svsm_apic.c
> +++ b/arch/x86/kernel/apic/svsm_apic.c
> @@ -68,7 +68,8 @@ static void svsm_apic_msr_write(u32 reg, u32 v)
>  		}
>  		break;
>  	default:
> -		pr_err("SVSM_APIC_WRITE_REGISTER 0x%x not supported\n", reg);
> +		pr_debug("SVSM_APIC_WRITE_REGISTER to HV (0x%x, val:0x%x)\n", reg, v);

Not sure what makes this different from the SVSM calls that don't have a
pr_debug(), so you can probably remove the pr_debug().

> +		hvs_ghcb_msr_write(reg, v);
>  		break;
>  	}
>  }
> @@ -77,6 +78,7 @@ static u32 svsm_apic_msr_read(u32 reg)
>  {
>  	u32 msr = APIC_BASE_MSR + (reg >> 4);
>  	struct svsm_call call = {};
> +	u64 val;
>  	int ret;
>  
>  	switch (reg) {
> @@ -93,17 +95,18 @@ static u32 svsm_apic_msr_read(u32 reg)
>  		call.rcx = msr;
>  
>  		ret = svsm_do_call(&call);
> +		val = call.rdx_out;
>  		if (ret) {
>  			pr_err("SVSM_APIC_READ_REGISTER: 0x%x, error: %d\n", reg, ret);
>  			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
>  		}
>  		break;
>  	default:
> -		pr_err("SVSM_APIC_READ_REGISTER: 0x%x not supported\n", reg);
> -		return 0;
> +		val = hvs_ghcb_msr_read(reg);
> +		pr_debug("SVSM_APIC_READ_REGISTER from HV 0x%x, val: 0x%llx\n", reg, val);

Ditto.

Thanks,
Tom

>  	}
>  
> -	return call.rdx_out;
> +	return val;
>  }
>  
>  static inline void svsm_apic_msr_eoi(void)
> diff --git a/arch/x86/kernel/apic/x2apic_savic.c b/arch/x86/kernel/apic/x2apic_savic.c
> index 4bc6d7e018a5..71040f77dfaf 100644
> --- a/arch/x86/kernel/apic/x2apic_savic.c
> +++ b/arch/x86/kernel/apic/x2apic_savic.c
> @@ -72,7 +72,7 @@ static u32 savic_read(u32 reg)
>  	case APIC_LVT0:
>  	case APIC_LVT1:
>  	case APIC_LVTERR:
> -		return savic_ghcb_msr_read(reg);
> +		return hvs_ghcb_msr_read(reg);
>  	case APIC_ID:
>  	case APIC_LVR:
>  	case APIC_TASKPRI:
> @@ -193,7 +193,7 @@ static void savic_icr_write(u32 icr_low, u32 icr_high)
>  
>  	icr_data = ((u64)icr_high) << 32 | icr_low;
>  	if (dsh != APIC_DEST_SELF)
> -		savic_ghcb_msr_write(APIC_ICR, icr_data);
> +		hvs_ghcb_msr_write(APIC_ICR, icr_data);
>  	apic_set_reg64(this_cpu_ptr(savic_page), APIC_ICR, icr_data);
>  }
>  
> @@ -210,7 +210,7 @@ static void savic_write(u32 reg, u32 data)
>  	case APIC_LVTTHMR:
>  	case APIC_LVTPC:
>  	case APIC_LVTERR:
> -		savic_ghcb_msr_write(reg, data);
> +		hvs_ghcb_msr_write(reg, data);
>  		break;
>  	case APIC_TASKPRI:
>  	case APIC_EOI:
> @@ -316,7 +316,7 @@ static void savic_eoi(void)
>  		 * interrupts. Return to the guest from GHCB protocol event takes
>  		 * care of re-evaluating interrupt state.
>  		 */
> -		savic_ghcb_msr_write(APIC_EOI, 0);
> +		hvs_ghcb_msr_write(APIC_EOI, 0);
>  	} else {
>  		/*
>  		 * Hardware clears APIC_ISR and re-evaluates the interrupt state