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

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <SA1PR21MB133502A09535F8A32073AADCBF10A@SA1PR21MB1335.namprd21.prod.outlook.com>
Date:   Fri, 11 Aug 2023 00:39:34 +0000
From:   Dexuan Cui <decui@...rosoft.com>
To:     Tianyu Lan <ltykernel@...il.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "x86@...nel.org" <x86@...nel.org>, "hpa@...or.com" <hpa@...or.com>,
        "daniel.lezcano@...aro.org" <daniel.lezcano@...aro.org>,
        "arnd@...db.de" <arnd@...db.de>,
        "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
CC:     Tianyu Lan <Tianyu.Lan@...rosoft.com>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        vkuznets <vkuznets@...hat.com>,
        "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
Subject: RE: [PATCH V5 7/8] x86/hyperv: Add smp support for SEV-SNP guest

> From: Tianyu Lan <ltykernel@...il.com>
> 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