Re: [PATCH Part1 RFC v2 16/20] x86/kernel: Validate rom memory before accessing when SEV-SNP is active

[Brijesh Singh]

On 5/27/21 6:49 AM, Borislav Petkov wrote:
> On Fri, Apr 30, 2021 at 07:16:12AM -0500, Brijesh Singh wrote:
>> +	/*
>> +	 * The ROM memory is not part of the E820 system RAM and is not pre-validated
>> +	 * by the BIOS. The kernel page table maps the ROM region as encrypted memory,
>> +	 * the SEV-SNP requires the encrypted memory must be validated before the
>> +	 * access. Validate the ROM before accessing it.
>> +	 */
>> +	n = ((system_rom_resource.end + 1) - video_rom_resource.start) >> PAGE_SHIFT;
>> +	early_snp_set_memory_private((unsigned long)__va(video_rom_resource.start),
>> +			video_rom_resource.start, n);
> From last review:
>
> I don't like this sprinkling of SNP-special stuff that needs to be done,
> around the tree. Instead, pls define a function called
>
>         snp_prep_memory(unsigned long pa, unsigned int num_pages, enum operation);

In the previous patch we were doing:

if (sev_snp_active()) {

   early_set_memory_private(....)

}

Based on your feedback on other patches, I moved the sev_snp_active()
check inside the function. The callsites can now make unconditional call
to change the page state. After implementing that feedback, I don't see
a strong reason for yet another helper unless I am missing something:

snp_prep_memory(pa, n, SNP_PAGE_PRIVATE) == snp_set_memory_private(pa, n)

snp_prep_memory(pa, n, SNP_PAGE_SHARED) == snp_set_memory_shared(pa, n)

Let me know if you still think that snp_prep_memory() helper is required.

-Brijesh

> or so which does all the manipulation needed and the callsites only
> simply unconditionally call that function so that all detail is
> extracted and optimized away when not config-enabled.
>