Re: [PATCH v2] Remove the memory encryption mask to obtain the true physical address

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



在 2019年01月29日 03:45, Kazuhito Hagio 写道:
> On 1/28/2019 9:24 AM, Lendacky, Thomas wrote:
>> On 1/27/19 11:46 PM, Lianbo Jiang wrote:
>>> For AMD machine with SME feature, if SME is enabled in the first
>>> kernel, the crashed kernel's page table(pgd/pud/pmd/pte) contains
>>> the memory encryption mask, so makedumpfile needs to remove the
>>> memory encryption mask to obtain the true physical address.
>>>
>>> Signed-off-by: Lianbo Jiang <lijiang@xxxxxxxxxx>
>>> ---
>>> Changes since v1:
>>> 1. Merge them into a patch.
>>> 2. The sme_mask is not an enum number, remove it.
>>> 3. Sanity check whether the sme_mask is in vmcoreinfo.
>>> 4. Deal with the huge pages case.
>>> 5. Cover the 5-level path.
>>>
>>>  arch/x86_64.c  | 30 +++++++++++++++++-------------
>>>  makedumpfile.c |  4 ++++
>>>  makedumpfile.h |  1 +
>>>  3 files changed, 22 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/arch/x86_64.c b/arch/x86_64.c
>>> index 537fb78..7b3ed10 100644
>>> --- a/arch/x86_64.c
>>> +++ b/arch/x86_64.c
>>> @@ -291,6 +291,7 @@ __vtop4_x86_64(unsigned long vaddr, unsigned long pagetable)
>>>  	unsigned long page_dir, pgd, pud_paddr, pud_pte, pmd_paddr, pmd_pte;
>>>  	unsigned long pte_paddr, pte;
>>>  	unsigned long p4d_paddr, p4d_pte;
>>> +	unsigned long sme_me_mask = ~0UL;
>>>
>>>  	/*
>>>  	 * Get PGD.
>>> @@ -302,6 +303,9 @@ __vtop4_x86_64(unsigned long vaddr, unsigned long pagetable)
>>>  			return NOT_PADDR;
>>>  	}
>>>
>>> +	if (NUMBER(sme_mask) != NOT_FOUND_NUMBER)
>>> +		sme_me_mask = ~(NUMBER(sme_mask));
>>
>> This is a bit confusing since this isn't the sme_me_mask any more, but the
>> complement. Might want to somehow rename this so that it doesn't cause any
>> confusion.
>>
>>> +
>>>  	if (check_5level_paging()) {
>>>  		page_dir += pgd5_index(vaddr) * sizeof(unsigned long);
>>>  		if (!readmem(PADDR, page_dir, &pgd, sizeof pgd)) {
>>> @@ -309,7 +313,7 @@ __vtop4_x86_64(unsigned long vaddr, unsigned long pagetable)
>>>  			return NOT_PADDR;
>>>  		}
>>>  		if (info->vaddr_for_vtop == vaddr)
>>> -			MSG("  PGD : %16lx => %16lx\n", page_dir, pgd);
>>> +			MSG("  PGD : %16lx => %16lx\n", page_dir, (pgd & sme_me_mask));
>>
>> No need to remove the mask here.  You're just printing out the value of
>> the entry. It might be nice to know whether the encryption bit is set or
>> not - after all, ENTRY_MASK is still part of this value.
> 
> Agreed.
> 
>>
>>>
>>>  		if (!(pgd & _PAGE_PRESENT)) {
>>>  			ERRMSG("Can't get a valid pgd.\n");
>>> @@ -318,20 +322,20 @@ __vtop4_x86_64(unsigned long vaddr, unsigned long pagetable)
>>>  		/*
>>>  		 * Get P4D.
>>>  		 */
>>> -		p4d_paddr  = pgd & ENTRY_MASK;
>>> +		p4d_paddr  = pgd & ENTRY_MASK & sme_me_mask;
>>
>> This goes back to my original comment that you should just make a local
>> variable that is "ENTRY_MASK & ~(NUMBER(sme_mask))" since you are
>> performing this ANDing everywhere ENTRY_MASK is used - except then you
>> miss the one at the very end of this routine on the return statement.
> 
> This was my idea I said to Lianbo before seeing your comment, but
> yes, including ENTRY_MASK in a local variable is better than that.
> Thanks for your good suggestion.
> 
> As for the variable's name, I think that "entry_mask" is good enough,
> but any better name?
> 
>   unsigned long entry_mask = ENTRY_MASK;
> 
>   if (NUMBER(sme_mask) != NOT_FOUND_NUMBER)
>       entry_mask &= ~(NUMBER(sme_mask));
>   ...
>   p4d_paddr = pgd & entry_mask;
> 
Ok. Thanks.

> And, I found that the find_vmemmap_x86_64() function also uses the
> page table for the -e option and looks to be affected by SME.
> Lianbo, would you fix the function, too?
> 

Yes, it's my pleasure. Thank you, Kazu.

I will fix this function in patch v3.

Regards,
Lianbo

> Thanks,
> Kazu
> 
>>
>>>  		p4d_paddr += p4d_index(vaddr) * sizeof(unsigned long);
>>>  		if (!readmem(PADDR, p4d_paddr, &p4d_pte, sizeof p4d_pte)) {
>>>  			ERRMSG("Can't get p4d_pte (p4d_paddr:%lx).\n", p4d_paddr);
>>>  			return NOT_PADDR;
>>>  		}
>>>  		if (info->vaddr_for_vtop == vaddr)
>>> -			MSG("  P4D : %16lx => %16lx\n", p4d_paddr, p4d_pte);
>>> +			MSG("  P4D : %16lx => %16lx\n", p4d_paddr, (p4d_pte & sme_me_mask));
>>>
>>>  		if (!(p4d_pte & _PAGE_PRESENT)) {
>>>  			ERRMSG("Can't get a valid p4d_pte.\n");
>>>  			return NOT_PADDR;
>>>  		}
>>> -		pud_paddr  = p4d_pte & ENTRY_MASK;
>>> +		pud_paddr  = p4d_pte & ENTRY_MASK & sme_me_mask;
>>>  	}else {
>>>  		page_dir += pgd_index(vaddr) * sizeof(unsigned long);
>>>  		if (!readmem(PADDR, page_dir, &pgd, sizeof pgd)) {
>>> @@ -339,13 +343,13 @@ __vtop4_x86_64(unsigned long vaddr, unsigned long pagetable)
>>>  			return NOT_PADDR;
>>>  		}
>>>  		if (info->vaddr_for_vtop == vaddr)
>>> -			MSG("  PGD : %16lx => %16lx\n", page_dir, pgd);
>>> +			MSG("  PGD : %16lx => %16lx\n", page_dir, (pgd & sme_me_mask));
>>>
>>>  		if (!(pgd & _PAGE_PRESENT)) {
>>>  			ERRMSG("Can't get a valid pgd.\n");
>>>  			return NOT_PADDR;
>>>  		}
>>> -		pud_paddr  = pgd & ENTRY_MASK;
>>> +		pud_paddr  = pgd & ENTRY_MASK & sme_me_mask;
>>>  	}
>>>
>>>  	/*
>>> @@ -357,47 +361,47 @@ __vtop4_x86_64(unsigned long vaddr, unsigned long pagetable)
>>>  		return NOT_PADDR;
>>>  	}
>>>  	if (info->vaddr_for_vtop == vaddr)
>>> -		MSG("  PUD : %16lx => %16lx\n", pud_paddr, pud_pte);
>>> +		MSG("  PUD : %16lx => %16lx\n", pud_paddr, (pud_pte & sme_me_mask));
>>>
>>>  	if (!(pud_pte & _PAGE_PRESENT)) {
>>>  		ERRMSG("Can't get a valid pud_pte.\n");
>>>  		return NOT_PADDR;
>>>  	}
>>>  	if (pud_pte & _PAGE_PSE)	/* 1GB pages */
>>> -		return (pud_pte & ENTRY_MASK & PUD_MASK) +
>>> +		return (pud_pte & ENTRY_MASK & PUD_MASK & sme_me_mask) +
>>>  			(vaddr & ~PUD_MASK);
>>>
>>>  	/*
>>>  	 * Get PMD.
>>>  	 */
>>> -	pmd_paddr  = pud_pte & ENTRY_MASK;
>>> +	pmd_paddr  = pud_pte & ENTRY_MASK & sme_me_mask;
>>>  	pmd_paddr += pmd_index(vaddr) * sizeof(unsigned long);
>>>  	if (!readmem(PADDR, pmd_paddr, &pmd_pte, sizeof pmd_pte)) {
>>>  		ERRMSG("Can't get pmd_pte (pmd_paddr:%lx).\n", pmd_paddr);
>>>  		return NOT_PADDR;
>>>  	}
>>>  	if (info->vaddr_for_vtop == vaddr)
>>> -		MSG("  PMD : %16lx => %16lx\n", pmd_paddr, pmd_pte);
>>> +		MSG("  PMD : %16lx => %16lx\n", pmd_paddr, (pmd_pte & sme_me_mask));
>>>
>>>  	if (!(pmd_pte & _PAGE_PRESENT)) {
>>>  		ERRMSG("Can't get a valid pmd_pte.\n");
>>>  		return NOT_PADDR;
>>>  	}
>>>  	if (pmd_pte & _PAGE_PSE)	/* 2MB pages */
>>> -		return (pmd_pte & ENTRY_MASK & PMD_MASK) +
>>> +		return (pmd_pte & ENTRY_MASK & PMD_MASK & sme_me_mask) +
>>>  			(vaddr & ~PMD_MASK);
>>>
>>>  	/*
>>>  	 * Get PTE.
>>>  	 */
>>> -	pte_paddr  = pmd_pte & ENTRY_MASK;
>>> +	pte_paddr  = pmd_pte & ENTRY_MASK & sme_me_mask;
>>>  	pte_paddr += pte_index(vaddr) * sizeof(unsigned long);
>>>  	if (!readmem(PADDR, pte_paddr, &pte, sizeof pte)) {
>>>  		ERRMSG("Can't get pte (pte_paddr:%lx).\n", pte_paddr);
>>>  		return NOT_PADDR;
>>>  	}
>>>  	if (info->vaddr_for_vtop == vaddr)
>>> -		MSG("  PTE : %16lx => %16lx\n", pte_paddr, pte);
>>> +		MSG("  PTE : %16lx => %16lx\n", pte_paddr, (pte & sme_me_mask));
>>>
>>>  	if (!(pte & _PAGE_PRESENT)) {
>>>  		ERRMSG("Can't get a valid pte.\n");
>>
>> Just after here is where I think an "& sme_me_mask" is missing...
>>
>> Thanks,
>> Tom
>>
>>> diff --git a/makedumpfile.c b/makedumpfile.c
>>> index 8923538..2237eb8 100644
>>> --- a/makedumpfile.c
>>> +++ b/makedumpfile.c
>>> @@ -977,6 +977,8 @@ next_page:
>>>  	read_size = MIN(info->page_size - PAGEOFFSET(paddr), size);
>>>
>>>  	pgaddr = PAGEBASE(paddr);
>>> +	if (NUMBER(sme_mask) != NOT_FOUND_NUMBER)
>>> +		pgaddr = pgaddr & ~(NUMBER(sme_mask));
>>>  	pgbuf = cache_search(pgaddr, read_size);
>>>  	if (!pgbuf) {
>>>  		++cache_miss;
>>> @@ -2276,6 +2278,7 @@ write_vmcoreinfo_data(void)
>>>  	WRITE_NUMBER("NR_FREE_PAGES", NR_FREE_PAGES);
>>>  	WRITE_NUMBER("N_ONLINE", N_ONLINE);
>>>  	WRITE_NUMBER("pgtable_l5_enabled", pgtable_l5_enabled);
>>> +	WRITE_NUMBER("sme_mask", sme_mask);
>>>
>>>  	WRITE_NUMBER("PG_lru", PG_lru);
>>>  	WRITE_NUMBER("PG_private", PG_private);
>>> @@ -2672,6 +2675,7 @@ read_vmcoreinfo(void)
>>>  	READ_NUMBER("NR_FREE_PAGES", NR_FREE_PAGES);
>>>  	READ_NUMBER("N_ONLINE", N_ONLINE);
>>>  	READ_NUMBER("pgtable_l5_enabled", pgtable_l5_enabled);
>>> +	READ_NUMBER("sme_mask", sme_mask);
>>>
>>>  	READ_NUMBER("PG_lru", PG_lru);
>>>  	READ_NUMBER("PG_private", PG_private);
>>> diff --git a/makedumpfile.h b/makedumpfile.h
>>> index 73813ed..e97b2e7 100644
>>> --- a/makedumpfile.h
>>> +++ b/makedumpfile.h
>>> @@ -1912,6 +1912,7 @@ struct number_table {
>>>  	long	NR_FREE_PAGES;
>>>  	long	N_ONLINE;
>>>  	long	pgtable_l5_enabled;
>>> +	long	sme_mask;
>>>
>>>  	/*
>>>   	* Page flags
>>>

_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec




[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux