On Wed, Aug 11, 2021 at 10:52:55AM -0500, Tom Lendacky wrote: > On 8/11/21 7:19 AM, Kirill A. Shutemov wrote: > > On Tue, Aug 10, 2021 at 02:48:54PM -0500, Tom Lendacky wrote: > >> On 8/10/21 1:45 PM, Kuppuswamy, Sathyanarayanan wrote: > >>> > >>> > >>> On 7/27/21 3:26 PM, Tom Lendacky wrote: > >>>> diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c > >>>> index de01903c3735..cafed6456d45 100644 > >>>> --- a/arch/x86/kernel/head64.c > >>>> +++ b/arch/x86/kernel/head64.c > >>>> @@ -19,7 +19,7 @@ > >>>> #include <linux/start_kernel.h> > >>>> #include <linux/io.h> > >>>> #include <linux/memblock.h> > >>>> -#include <linux/mem_encrypt.h> > >>>> +#include <linux/protected_guest.h> > >>>> #include <linux/pgtable.h> > >>>> #include <asm/processor.h> > >>>> @@ -285,7 +285,7 @@ unsigned long __head __startup_64(unsigned long > >>>> physaddr, > >>>> * there is no need to zero it after changing the memory encryption > >>>> * attribute. > >>>> */ > >>>> - if (mem_encrypt_active()) { > >>>> + if (prot_guest_has(PATTR_MEM_ENCRYPT)) { > >>>> vaddr = (unsigned long)__start_bss_decrypted; > >>>> vaddr_end = (unsigned long)__end_bss_decrypted; > >>> > >>> > >>> Since this change is specific to AMD, can you replace PATTR_MEM_ENCRYPT > >>> with > >>> prot_guest_has(PATTR_SME) || prot_guest_has(PATTR_SEV). It is not used in > >>> TDX. > >> > >> This is a direct replacement for now. > > > > With current implementation of prot_guest_has() for TDX it breaks boot for > > me. > > > > Looking at code agains, now I *think* the reason is accessing a global > > variable from __startup_64() inside TDX version of prot_guest_has(). > > > > __startup_64() is special. If you access any global variable you need to > > use fixup_pointer(). See comment before __startup_64(). > > > > I'm not sure how you get away with accessing sme_me_mask directly from > > there. Any clues? Maybe just a luck and complier generates code just right > > for your case, I donno. > > Hmm... yeah, could be that the compiler is using rip-relative addressing > for it because it lives in the .data section?
I guess. It has to be fixed. It may break with complier upgrade or any random change around the code. BTW, does it work with clang for you? > For the static variables in mem_encrypt_identity.c I did an assembler rip > relative LEA, but probably could have passed physaddr to sme_enable() and > used a fixup_pointer() style function, instead. Sounds like a plan. > > A separate point is that TDX version of prot_guest_has() relies on > > cpu_feature_enabled() which is not ready at this point. > > Does TDX have to do anything special to make memory able to be shared with > the hypervisor? Yes. But there's nothing that required any changes in early boot. It handled in ioremap/set_memory. > You might have to use something that is available earlier > than cpu_feature_enabled() in that case (should you eventually support > kvmclock). Maybe. > > I think __bss_decrypted fixup has to be done if sme_me_mask is non-zero. > > Or just do it uncoditionally because it's NOP for sme_me_mask == 0. > > For SNP, we'll have to additionally call the HV to update the RMP to make > the memory shared. But that could also be done unconditionally since the > early_snp_set_memory_shared() routine will check for SNP before doing > anything. -- Kirill A. Shutemov _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu