| Message ID | 20210430121616.2295-17-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand |
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); 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.
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. >
On Thu, May 27, 2021 at 07:12:00AM -0500, Brijesh Singh wrote:
> Let me know if you still think that snp_prep_memory() helper is required.
Yes, I still do think that because you can put the comment and all
the manupulation of parameters in there and have a single oneliner in
probe_roms.c:
snp_prep_memory(...);
and have the details in sev.c where they belong.
Thx.
On 5/27/21 7:23 AM, Borislav Petkov wrote: > On Thu, May 27, 2021 at 07:12:00AM -0500, Brijesh Singh wrote: >> Let me know if you still think that snp_prep_memory() helper is required. > Yes, I still do think that because you can put the comment and all > the manupulation of parameters in there and have a single oneliner in > probe_roms.c: > > snp_prep_memory(...); > > and have the details in sev.c where they belong. Okay, I will add the helper.
diff --git a/arch/x86/kernel/probe_roms.c b/arch/x86/kernel/probe_roms.c index 9e1def3744f2..7638d9c8e1e8 100644 --- a/arch/x86/kernel/probe_roms.c +++ b/arch/x86/kernel/probe_roms.c @@ -21,6 +21,7 @@ #include <asm/sections.h> #include <asm/io.h> #include <asm/setup_arch.h> +#include <asm/sev.h> static struct resource system_rom_resource = { .name = "System ROM", @@ -197,11 +198,21 @@ static int __init romchecksum(const unsigned char *rom, unsigned long length) void __init probe_roms(void) { + unsigned long start, length, upper, n; const unsigned char *rom; - unsigned long start, length, upper; unsigned char c; int i; + /* + * 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); + /* video rom */ upper = adapter_rom_resources[0].start; for (start = video_rom_resource.start; start < upper; start += 2048) {
The probe_roms() access the memory range (0xc0000 - 0x10000) to probe various ROMs. The memory range is not part of the E820 system RAM range. The memory range is mapped as private (i.e encrypted) in page table. When SEV-SNP is active, all the private memory must be validated before the access. The ROM range was not part of E820 map, so the guest BIOS did not validate it. An access to invalidated memory will cause a VC exception. The guest does not support handling not-validated VC exception yet, so validate the ROM memory regions before it is accessed. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- arch/x86/kernel/probe_roms.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)