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

[Tom Lendacky]

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@xxxxxxx>
> >
> > 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@xxxxxxx>
> > Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> > ---
> >   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);
> > +	}