Re: [PATCH 1/3] kexec: Do not map the kexec area as decrypted when SEV is active

[Lendacky, Thomas]

On 3/25/19 1:17 PM, Singh, Brijesh wrote:
> 
> 
> On 3/25/19 12:32 PM, Borislav Petkov wrote:
>> On Mon, Mar 25, 2019 at 05:17:55PM +0000, Singh, Brijesh wrote:
>>> By default all the memory regions are mapped encrypted. The
>>> set_memory_{encrypt,decrypt}() is a generic function which can be
>>> called explicitly to clear/set the encryption mask from the existing
>>> memory mapping. The mem_encrypt_active() returns true if either SEV or
>>> SME is active. So the __set_memory_enc_dec() uses the
>>> memory_encrypt_active() check to ensure that the function is no-op when
>>> SME/SEV are not active.
>>>
>>> Currently, the arch_kexec_post_alloc_pages() unconditionally clear the
>>> encryption mask from the kexec area. In case of SEV, we should not clear
>>> the encryption mask.
>>
>> Brijesh, I know all that.
>>
>> Please read what I said here at the end:
>>
>> https://lkml.kernel.org/r/20190324150034.GH23289@xxxxxxx
>>
>> With this change, the code looks like this:
>>
>> +       if (sme_active())
>> +               return set_memory_decrypted((unsigned long)vaddr, pages);
>>
>> now in __set_memory_enc_dec via set_memory_decrypted():
>>
>>          /* Nothing to do if memory encryption is not active */
>>          if (!mem_encrypt_active())
>>                  return 0;
>>
>>
>> so you have:
>>
>> 	if (sme_active())
>>
>> 		...
>>
>> 		if (!mem_encrypt_active())
>>
>>
>> now maybe this is all clear to you and Tom but I betcha others will get
>> confused. Probably something like "well, what should be active now, SME,
>> SEV or memory encryption in general"?
>>
>> I hope you're catching my drift.
>>
>> So if you want to *not* decrypt memory in the SEV case, then doing something
>> like this should make it a bit more clear:
>>
>>
>> 	if (sev_active())
>> 		return;
>>
>> 	return set_memory_decrypted((unsigned long)vaddr, pages);
>>
> 
> 
> I see your point. I agree it can get confusing.
> 
> 
>> along with a comment *why* we're checking here.
>>
>> But actually, I'd prefer if you had separate wrappers which are called
>> for SME and for SEV.
> 
> 
> Just a thought, maybe we can move the above if(sev_active()) check up in
> kernel/kexec_core.c because we don't need to set/clear the encryption
> masks when SEV is active so there is no need to call the wrapper.

IIRC, the wrapper was created to avoid checking in the main kexec support
and move the check down to the arch specific support. So I don't think we
should move it.

I'm not sure about the separate wrappers, I like the above code where the
arch callback returns if sev_active() is true.

Maybe what would help is to describe why there is a difference between SME
and SEV in regards to kexec. During a traditional boot under SME, SME will
encrypt the kernel, so the SME kexec kernel also needs to be un-encrypted
in order to replicate a normal SME boot. During a traditional boot under
SEV, the kernel has already been loaded encrypted, so the SEV kexec kernel
needs to be encrypted in order to replicate a normal SEV boot.

Thanks,
Tom

> 
> 
>>
>> I'll let Tom chime in too.
>>
>