Re: [RFC Part1 PATCH v3 06/17] x86/mm: Use encrypted access of boot related data with SEV

Tom Lendacky
2017-08-17T18:05:38Z

On 7/27/2017 8:31 AM, Borislav Petkov wrote:
> On Mon, Jul 24, 2017 at 02:07:46PM -0500, Brijesh Singh wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> When Secure Encrypted Virtualization (SEV) is active, boot data (such as
>> EFI related data, setup data) is encrypted and needs to be accessed as
>> such when mapped. Update the architecture override in early_memremap to
>> keep the encryption attribute when mapping this data.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>> arch/x86/mm/ioremap.c | 44 ++++++++++++++++++++++++++++++++------------
>> 1 file changed, 32 insertions(+), 12 deletions(-)
>
> ...
>
>> @@ -590,10 +598,15 @@ bool arch_memremap_can_ram_remap(resource_size_t phys_addr, unsigned long size,
>> if (flags & MEMREMAP_DEC)
>> return false;
>>
>> - if (memremap_is_setup_data(phys_addr, size) ||
>> - memremap_is_efi_data(phys_addr, size) ||
>> - memremap_should_map_decrypted(phys_addr, size))
>> - return false;
>> + if (sme_active()) {
>> + if (memremap_is_setup_data(phys_addr, size) ||
>> + memremap_is_efi_data(phys_addr, size) ||
>> + memremap_should_map_decrypted(phys_addr, size))
>> + return false;
>> + } else if (sev_active()) {
>> + if (memremap_should_map_decrypted(phys_addr, size))
>> + return false;
>> + }
>>
>> return true;
>> }
>
> I guess this function's hind part can be simplified to:
>
> if (sme_active()) {
> if (memremap_is_setup_data(phys_addr, size) ||
> memremap_is_efi_data(phys_addr, size))
> return false;
> }
>
> return ! memremap_should_map_decrypted(phys_addr, size);
> }
>

Ok, definitely cleaner.

>> @@ -608,15 +621,22 @@ pgprot_t __init early_memremap_pgprot_adjust(resource_size_t phys_addr,
>> unsigned long size,
>> pgprot_t prot)
>
> And this one in a similar manner...
>
>> {
>> - if (!sme_active())
>> + if (!sme_active() && !sev_active())
>> return prot;
>
> ... and you don't need that check...
>
>> - if (early_memremap_is_setup_data(phys_addr, size) ||
>> - memremap_is_efi_data(phys_addr, size) ||
>> - memremap_should_map_decrypted(phys_addr, size))
>> - prot = pgprot_decrypted(prot);
>> - else
>> - prot = pgprot_encrypted(prot);
>> + if (sme_active()) {
>
> ... if you're going to do it here too.
>
>> + if (early_memremap_is_setup_data(phys_addr, size) ||
>> + memremap_is_efi_data(phys_addr, size) ||
>> + memremap_should_map_decrypted(phys_addr, size))
>> + prot = pgprot_decrypted(prot);
>> + else
>> + prot = pgprot_encrypted(prot);
>> + } else if (sev_active()) {
>
> And here.

Will do.

Thanks,
Tom

>
>> + if (memremap_should_map_decrypted(phys_addr, size))
>> + prot = pgprot_decrypted(prot);
>> + else
>> + prot = pgprot_encrypted(prot);
>> + }
>