Re: [PATCH Part2 RFC v4 04/40] x86/sev: Add the host SEV-SNP initialization support

[Brijesh Singh <brijesh.singh@amd.com>]

On 7/14/21 4:07 PM, Sean Christopherson wrote:
>>   
>> +#define RMPTABLE_ENTRIES_OFFSET        0x4000
> 
> A comment and/or blurb in the changelog describing this magic number would be
> quite helpful.  And maybe call out that this is for the bookkeeping, e.g.
> 
>    #define RMPTABLE_CPU_BOOKKEEPING_SIZE	0x4000

Noted.

> 
> Also, the APM doesn't actually state the exact location of the bookkeeping
> region, it only states that it's somewhere between RMP_BASE and RMP_END.  This
> seems to imply that the bookkeeping region is always at RMP_BASE?
> 
>    The region of memory between RMP_BASE and RMP_END contains a 16KB region used
>    for processor bookkeeping followed by the RMP entries, which are each 16B in
>    size. The size of the RMP determines the range of physical memory that the
>    hypervisor can assign to SNP-active virtual machines at runtime. The RMP covers
>    the system physical address space from address 0h to the address calculated by:
> 
>    ((RMP_END + 1 – RMP_BASE – 16KB) / 16B) x 4KB
> 

The bookkeeping region is at the start of the RMP_BASE. If we look at 
the PPR then it provides a formula which we should use to read the RMP 
entry location. And in that it adds the bookkeeping to the RMP_BASE.

       RMP Entry Address = RMP_BASE + 0x4000 + x>>8


>> +
>> +	val |= MSR_AMD64_SYSCFG_SNP_EN;
>> +	val |= MSR_AMD64_SYSCFG_SNP_VMPL_EN;
> 
> Is VMPL required?  Do we plan on using VMPL out of the gate?
> 

The SEV-SNP firmware requires that VMPL must be enabled otherwise it 
will fail to initialize. However, the current SEV-SNP support is limited 
to the VMPL0.

> 
> Can BIOS put the RMP at PA=0?

No, they should not. As per the PPR, the 0h is a reset value (means the 
MSR is not programmed).

> 
> Also, why is it a BIOS decision?  AFAICT, the MSRs aren't locked until SNP_EN
> is set in SYSCFG, and that appears to be a kernel decision (ignoring kexec),
> i.e. nothing would prevent the kernel from configuring it's own RMP.

In the current patch set, we assume that user is configuring the BIOS to 
reserve memory for the RMP table. From hardware point-of-view, it does 
not matter who reserves the memory (bios or kernel). In future, we could 
look into reserving the memory from the kernel before through the 
memblock etc.

> 
>> +		pr_info("Memory for the RMP table has not been reserved by BIOS\n");
>> +		return false;
>> +	}
>> +
>> +	rmp_sz = rmp_end - rmp_base + 1;
>> +
>> +	/*
>> +	 * Calculate the amount the memory that must be reserved by the BIOS to
>> +	 * address the full system RAM. The reserved memory should also cover the
>> +	 * RMP table itself.
>> +	 *
>> +	 * See PPR section 2.1.5.2 for more information on memory requirement.
>> +	 */
>> +	nr_pages = totalram_pages();
>> +	calc_rmp_sz = (((rmp_sz >> PAGE_SHIFT) + nr_pages) << 4) + RMPTABLE_ENTRIES_OFFSET;
>> +
>> +	if (calc_rmp_sz > rmp_sz) {
>> +		pr_info("Memory reserved for the RMP table does not cover the full system "
>> +			"RAM (expected 0x%llx got 0x%llx)\n", calc_rmp_sz, rmp_sz);
> 
> Is BIOS expected to provide exact coverage, e.g. should this be s/expected/need?
> 

BIOS provides option to reserve the required memory. If they don't cover 
the entire system ram then its a BIOS bug.

Yes, I will fix the wording s/expected/need.

To make things interesting, it also has option where user can specify 
amount of memory to be reserved. If user does not cover the full system 
ram then we need to warn and not enable the SNP. We cannot work with 
partially reserved RMP table memory.


> Should the kernel also sanity check other requirements, e.g. the 8kb alignment,
> or does the CPU enforce those things at WRMSR?
> 

The SNP firmware enforces those requirement. It is documented in the SNP 
firmware specification (SNP_INIT).



>> +
>> +	/*
>> +	 * Check if SEV-SNP is already enabled, this can happen if we are coming from
>> +	 * kexec boot.
>> +	 */
>> +	rdmsrl(MSR_AMD64_SYSCFG, val);
>> +	if (val & MSR_AMD64_SYSCFG_SNP_EN)
> 
> Hmm, it kinda feels like there should be a sanity check for the case where SNP is
> already enabled but get_rmptable_info() fails, e.g. due to insufficient RMP size.
> 

Hmm, I am not sure if we need to do this. We enabled the SNP only after 
all the sanity check is completed, so the get_rmptable_info() will not 
fail after the SNP is enabled. The RMP MSR's are locked after the SNP is 
enabled so we should not see a different size.


>> +		goto skip_enable;
>> +
>> +	/* Initialize the RMP table to zero */
>> +	memset(start, 0, sz);
>> +
>> +	/* Flush the caches to ensure that data is written before SNP is enabled. */
>> +	wbinvd_on_all_cpus();
>> +
>> +	/* Enable SNP on all CPUs. */
>> +	on_each_cpu(snp_enable, NULL, 1);
>> +
>> +skip_enable:
>> +	rmptable_start = (unsigned long)start;
> 
> Mostly out of curiosity, why store start/end as unsigned longs?  This is all 64-bit
> only so it doesn't actually affect the code generation, but it feels odd to store
> things that absolutely have to be 64-bit values as unsigned long.
> 

The AMD memory encryption support is compiled when 64-bit is enabled in 
the Kconfig; Having said that, I am okay to use the u64.


> Similar question for why asm/sev-common.h cases to unsigned long instead of u64.
> E.g. the below in particular looks wrong because we're shifting an unsigned long
> b y32 bits, i.e. the value _must_ be a 64-bit value, why obfuscate that?
> 
> 	#define GHCB_CPUID_REQ(fn, reg)		\
> 		(GHCB_MSR_CPUID_REQ | \
> 		(((unsigned long)reg & GHCB_MSR_CPUID_REG_MASK) << GHCB_MSR_CPUID_REG_POS) | \
> 		(((unsigned long)fn) << GHCB_MSR_CPUID_FUNC_POS))
> 
>> +	rmptable_end = rmptable_start + sz;
>> +
>> +	return 0;
>> +}
>> +
>> +static int __init snp_rmptable_init(void)
>> +{
>> +	if (!boot_cpu_has(X86_FEATURE_SEV_SNP))
>> +		return 0;
>> +
>> +	/*
>> +	 * The SEV-SNP support requires that IOMMU must be enabled, and is not
>> +	 * configured in the passthrough mode.
>> +	 */
>> +	if (no_iommu || iommu_default_passthrough()) {
> 
> Similar comment regarding the sanity check, kexec'ing into a kernel with SNP
> already enabled should probably fail explicitly if the new kernel is booted with
> incompatible params.

Good point on the kexec, I'll look to cover it.

thanks