[PATCH v3 2/2] x86/sev: Let sev_es_efi_map_ghcbs() map the caa pages too

[PATCH v3 2/2] x86/sev: Let sev_es_efi_map_ghcbs() map the caa pages too

[Gerd Hoffmann]

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@redhat.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) {
+		address = per_cpu(svsm_caa_pa, cpu);
+		if (!address)
+			return 1;
+
+		pfn = address >> PAGE_SHIFT;
+		if (kernel_map_pages_in_pgd(pgd, pfn, address, 1, pflags_enc))
+			return 1;
+	}
+
 	return 0;
 }
 
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index e7e8f77f77f8..97e8032db45d 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -216,8 +216,8 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 	 * When SEV-ES is active, the GHCB as set by the kernel will be used
 	 * by firmware. Create a 1:1 unencrypted mapping for each GHCB.
 	 */
-	if (sev_es_efi_map_ghcbs(pgd)) {
-		pr_err("Failed to create 1:1 mapping for the GHCBs!\n");
+	if (sev_es_efi_map_ghcbs_caas(pgd)) {
+		pr_err("Failed to create 1:1 mapping for the GHCBs and CAAs!\n");
 		return 1;
 	}
 
-- 
2.50.0

Re: [PATCH v3 2/2] x86/sev: Let sev_es_efi_map_ghcbs() map the caa pages too

[Ingo Molnar]

* Gerd Hoffmann <kraxel@redhat.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@redhat.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