linux-kernel - Re: [PATCH v3 2/2] x86/sev: Let sev_es_efi_map

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aF0CemSo0go8-Bru@gmail.com>
Date: Thu, 26 Jun 2025 10:19:06 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Gerd Hoffmann <kraxel@...hat.com>
Cc: linux-coco@...ts.linux.dev, kvm@...r.kernel.org,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	"maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
	"H. Peter Anvin" <hpa@...or.com>, Ard Biesheuvel <ardb@...nel.org>,
	"open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <linux-kernel@...r.kernel.org>,
	"open list:EXTENSIBLE FIRMWARE INTERFACE (EFI)" <linux-efi@...r.kernel.org>
Subject: Re: [PATCH v3 2/2] x86/sev: Let sev_es_efi_map_ghcbs() map the caa
 pages too


* Gerd Hoffmann <kraxel@...hat.com> wrote:

> OVMF EFI firmware needs access to the CAA page to do SVSM protocol calls. For
> example, when the SVSM implements an EFI variable store, such calls will be
> necessary.
> 
> So add that to sev_es_efi_map_ghcbs() and also rename the function to reflect
> the additional job it is doing now.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@...hat.com>
> ---
>  arch/x86/include/asm/sev.h     |  4 ++--
>  arch/x86/coco/sev/core.c       | 20 ++++++++++++++++++--
>  arch/x86/platform/efi/efi_64.c |  4 ++--
>  3 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 58e028d42e41..6e0ef192f23b 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -445,7 +445,7 @@ static __always_inline void sev_es_nmi_complete(void)
>  	    cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT))
>  		__sev_es_nmi_complete();
>  }
> -extern int __init sev_es_efi_map_ghcbs(pgd_t *pgd);
> +extern int __init sev_es_efi_map_ghcbs_caas(pgd_t *pgd);
>  extern void sev_enable(struct boot_params *bp);
>  
>  /*
> @@ -556,7 +556,7 @@ static inline void sev_es_ist_enter(struct pt_regs *regs) { }
>  static inline void sev_es_ist_exit(void) { }
>  static inline int sev_es_setup_ap_jump_table(struct real_mode_header *rmh) { return 0; }
>  static inline void sev_es_nmi_complete(void) { }
> -static inline int sev_es_efi_map_ghcbs(pgd_t *pgd) { return 0; }
> +static inline int sev_es_efi_map_ghcbs_caas(pgd_t *pgd) { return 0; }
>  static inline void sev_enable(struct boot_params *bp) { }
>  static inline int pvalidate(unsigned long vaddr, bool rmp_psize, bool validate) { return 0; }
>  static inline int rmpadjust(unsigned long vaddr, bool rmp_psize, unsigned long attrs) { return 0; }
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index b6db4e0b936b..b52318d806b6 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -1045,11 +1045,13 @@ int __init sev_es_setup_ap_jump_table(struct real_mode_header *rmh)
>   * This is needed by the OVMF UEFI firmware which will use whatever it finds in
>   * the GHCB MSR as its GHCB to talk to the hypervisor. So make sure the per-cpu
>   * runtime GHCBs used by the kernel are also mapped in the EFI page-table.
> + *
> + * When running under SVSM the CCA page is needed too, so map it as well.
>   */
> -int __init sev_es_efi_map_ghcbs(pgd_t *pgd)
> +int __init sev_es_efi_map_ghcbs_caas(pgd_t *pgd)
>  {
>  	struct sev_es_runtime_data *data;
> -	unsigned long address, pflags;
> +	unsigned long address, pflags, pflags_enc;
>  	int cpu;
>  	u64 pfn;
>  
> @@ -1057,6 +1059,7 @@ int __init sev_es_efi_map_ghcbs(pgd_t *pgd)
>  		return 0;
>  
>  	pflags = _PAGE_NX | _PAGE_RW;
> +	pflags_enc = cc_mkenc(pflags);
>  
>  	for_each_possible_cpu(cpu) {
>  		data = per_cpu(runtime_data, cpu);
> @@ -1068,6 +1071,19 @@ int __init sev_es_efi_map_ghcbs(pgd_t *pgd)
>  			return 1;
>  	}
>  
> +	if (!snp_vmpl)
> +		return 0;
> +
> +	for_each_possible_cpu(cpu) {


So while it's only run-once __init code, still there's no good reason 
to have *two* all-CPUs loops in the same function.

> +		address = per_cpu(svsm_caa_pa, cpu);
> +		if (!address)
> +			return 1;

Yeah, so could we please use sensible & standard error return values 
such as -EINVAL? This is a pre-existing problem in this function, so it 
should be done in a separate, preparatory patch. (And yeah, the error 
codes of efi_setup_page_tables() are kinda lame too, but there's no 
reason to repeat that mistake in the SEV code.)

> +
> +		pfn = address >> PAGE_SHIFT;
> +		if (kernel_map_pages_in_pgd(pgd, pfn, address, 1, pflags_enc))
> +			return 1;


Ditto - for consistency this should just pass through the error code 
that kernel_map_pages_in_pgd() gives.

No objections to the added functionality/fix aspect.

Thanks,

	Ingo