RE: [PATCH V5 7/8] x86/hyperv: Add smp support for SEV-SNP guest

[Dexuan Cui]

> From: Tianyu Lan <ltykernel@xxxxxxxxx>
> Sent: Thursday, August 10, 2023 9:04 AM
> [...]
> --- a/arch/x86/hyperv/ivm.c
> +++ b/arch/x86/hyperv/ivm.c
> [...]
> +static u8 ap_start_input_arg[PAGE_SIZE] __bss_decrypted
> __aligned(PAGE_SIZE);
> +static u8 ap_start_stack[PAGE_SIZE] __aligned(PAGE_SIZE);
> +
>  union hv_ghcb {
>  	struct ghcb ghcb;
>  	struct {
> @@ -357,6 +366,97 @@ static bool hv_is_private_mmio(u64 addr)
>  	return false;
>  }
> 
> +#define hv_populate_vmcb_seg(seg, gdtr_base)			\
> +do {								\
> +	if (seg.selector) {					\
> +		seg.base = 0;					\
> +		seg.limit = HV_AP_SEGMENT_LIMIT;		\
> +		seg.attrib = *(u16 *)(gdtr_base + seg.selector + 5);	\
> +		seg.attrib = (seg.attrib & 0xFF) | ((seg.attrib >> 4) & 0xF00); \
> +	}							\
> +} while (0)							\
> +
> +int hv_snp_boot_ap(int cpu, unsigned long start_ip)
> +{
> +	struct sev_es_save_area *vmsa = (struct sev_es_save_area *)
> +		__get_free_page(GFP_KERNEL | __GFP_ZERO);

Should we check against -ENOMEM?

> +	rmp_adjust = RMPADJUST_VMSA_PAGE_BIT | 1;
> +	ret = rmpadjust((unsigned long)vmsa, RMP_PG_SIZE_4K,
> +			rmp_adjust);
> +	if (ret != 0) {
> +		pr_err("RMPADJUST(%llx) failed: %llx\n", (u64)vmsa, ret);

Need to free the 'vmsa' page before returning?

> +		return ret;
> +	}
> +
> +	local_irq_save(flags);
> +	start_vp_input =
> +		(struct hv_enable_vp_vtl *)ap_start_input_arg;

The above 2 lines can be merged into one line.

> +	memset(start_vp_input, 0, sizeof(*start_vp_input));
> +	start_vp_input->partition_id = -1;
> +	start_vp_input->vp_index = cpu;
> +	start_vp_input->target_vtl.target_vtl = ms_hyperv.vtl;
> +	*(u64 *)&start_vp_input->vp_context = __pa(vmsa) | 1;

Is CPU online/offline supported for a fully enlightened SNP VM?
If yes, then it looks like we need to reuse the same "vmsa" page?
Currently hv_snp_boot_ap() always creates a new page and it looks
like the page is never released

> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
>  [...]
> +<<<<<<< ours
> +static int hv_snp_boot_ap(int cpu, unsigned long start_ip) {}
> +=======

Remove the 3 lines.

> +static int hv_snp_boot_ap(int cpu, unsigned long start_ip) { return 0; }
> +static inline void hv_sev_init_mem_and_cpu(void) {}
> +>>>>>>> theirs

Remove the line

>  #endif
> 
>  extern bool hv_isolation_type_snp(void);
> diff --git a/arch/x86/kernel/cpu/mshyperv.c
> b/arch/x86/kernel/cpu/mshyperv.c
> index 5398fb2f4d39..c2ccb49b49c2 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -295,6 +295,16 @@ static void __init hv_smp_prepare_cpus(unsigned
> int max_cpus)
> 
>  	native_smp_prepare_cpus(max_cpus);
> 
> +	/*
> +	 *  Override wakeup_secondary_cpu_64 callback for SEV-SNP
> +	 *  enlightened guest.
> +	 */
> +	if (hv_isolation_type_en_snp())
> +		apic->wakeup_secondary_cpu_64 = hv_snp_boot_ap;

I suspect the global page "ap_start_stack" in arch/x86/hyperv/ivm.c
can be used simultaneously by different APs? If so, IMO we should
add x86_cpuinit.parallel_bringup = false; 

Thanks,
Dexuan