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