Re: [PATCH v8 13/40] x86/kernel: Make the bss.decrypted section shared in RMP table

From: Tom Lendacky <thomas.lendacky@amd.com>
To: Venu Busireddy <venu.busireddy@oracle.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>,
	Dave Hansen <dave.hansen@intel.com>,
	x86@kernel.org, linux-kernel@vger.kernel.org,
	kvm@vger.kernel.org, linux-efi@vger.kernel.org,
	platform-driver-x86@vger.kernel.org, linux-coco@lists.linux.dev,
	linux-mm@kvack.org, Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Joerg Roedel <jroedel@suse.de>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Ard Biesheuvel <ardb@kernel.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Sean Christopherson <seanjc@google.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Jim Mattson <jmattson@google.com>,
	Andy Lutomirski <luto@kernel.org>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Sergio Lopez <slp@redhat.com>, Peter Gonda <pgonda@google.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
	David Rientjes <rientjes@google.com>,
	Dov Murik <dovmurik@linux.ibm.com>,
	Tobin Feldman-Fitzthum <tobin@ibm.com>,
	Borislav Petkov <bp@alien8.de>,
	Michael Roth <michael.roth@amd.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	"Kirill A . Shutemov" <kirill@shutemov.name>,
	Andi Kleen <ak@linux.intel.com>,
	"Dr . David Alan Gilbert" <dgilbert@redhat.com>,
	tony.luck@intel.com, marcorr@google.com,
	sathyanarayanan.kuppuswamy@linux.intel.com
Subject: Re: [PATCH v8 13/40] x86/kernel: Make the bss.decrypted section shared in RMP table
Date: Thu, 6 Jan 2022 14:50:56 -0600	[thread overview]
Message-ID: <4c1067c7-a85f-79fe-38a8-2f68a94a358a@amd.com> (raw)
In-Reply-To: <YddODkUvhbWbhS3/@dt>

On 1/6/22 2:16 PM, Venu Busireddy wrote:
> On 2022-01-06 13:06:13 -0600, Tom Lendacky wrote:
>> On 1/6/22 11:40 AM, Venu Busireddy wrote:
>>> On 2022-01-05 15:39:22 -0600, Brijesh Singh wrote:
>>>>
>>>>
>>>> On 1/5/22 2:27 PM, Dave Hansen wrote:
>>>>> On 1/5/22 11:52, Brijesh Singh wrote:
>>>>>>>>             for (; vaddr < vaddr_end; vaddr += PMD_SIZE) {
>>>>>>>> +            /*
>>>>>>>> +             * When SEV-SNP is active then transition the
>>>>>>>> page to shared in the RMP
>>>>>>>> +             * table so that it is consistent with the page
>>>>>>>> table attribute change.
>>>>>>>> +             */
>>>>>>>> +            early_snp_set_memory_shared(__pa(vaddr),
>>>>>>>> __pa(vaddr), PTRS_PER_PMD);
>>>>>>>
>>>>>>> Shouldn't the first argument be vaddr as below?
>>>>>>
>>>>>> Nope, sme_postprocess_startup() is called while we are fixing the
>>>>>> initial page table and running with identity mapping (so va == pa).
>>>>>
>>>>> I'm not sure I've ever seen a line of code that wanted a comment so badly.
>>>>
>>>> The early_snp_set_memory_shared() call the PVALIDATE instruction to clear
>>>> the validated bit from the BSS region. The PVALIDATE instruction needs a
>>>> virtual address, so we need to use the identity mapped virtual address so
>>>> that PVALIDATE can clear the validated bit. I will add more comments to
>>>> clarify it.
>>>
>>> Looking forward to see your final comments explaining this. I can't
>>> still follow why, when PVALIDATE needs the virtual address, we are doing
>>> a __pa() on the vaddr.
>>
>> It's because of the phase of booting that the kernel is in. At this point,
>> the kernel is running in identity mapped mode (VA == PA). The
>> __start_bss_decrypted address is a regular kernel address, e.g. for the
>> kernel I'm on it is 0xffffffffa7600000. Since the PVALIDATE instruction
>> requires a valid virtual address, the code needs to perform a __pa() against
>> __start_bss_decrypted to get the identity mapped virtual address that is
>> currently in place.
> 
> Perhaps  my confusion stems from the fact that __pa(x) is defined either
> as "((unsigned long ) (x))" (for the cases where paddr and vaddr are
> same), or as "__phys_addr((unsigned long )(x))", where a vaddr needs to
> be converted to a paddr. If the paddr and vaddr are same in our case,
> what exactly is the _pa(vaddr) doing to the vaddr?

But they are not the same and the head64.c file is compiled without 
defining a value for __pa(), so __pa() is __phys_addr((unsigned long)(x)). 
The virtual address value of __start_bss_decrypted, for me, is 
0xffffffffa7600000, and that does not equal the physical address (take a 
look at your /proc/kallsyms). However, since the code is running identity 
mapped and with a page table without kernel virtual addresses, it cannot 
use that value. It needs to convert that value to the identity mapped 
virtual address and that is done using __pa(). Only after using __pa() on 
__start_bss_decrypted, do you get a virtual address that maps to and is 
equal to the physical address.

You may want to step through the boot code using KVM to see what the 
environment is and why things are done the way they are.

Thanks,
Tom

> 
> Venu
> 