Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 2/5 V6] x86/ioremap: strengthen the logic in
 early_memremap_pgprot_adjust() to adjust encryption mask
To:     Dave Young <dyoung@redhat.com>
Cc:     linux-kernel@vger.kernel.org, mingo@redhat.com, tglx@linutronix.de,
        hpa@zytor.com, ebiederm@xmission.com, joro@8bytes.org,
        thomas.lendacky@amd.com, kexec@lists.infradead.org,
        iommu@lists.linux-foundation.org, bhe@redhat.com,
        Ard Biesheuvel <ard.biesheuvel@linaro.org>,
        linux-efi <linux-efi@vger.kernel.org>
References: <20180831081930.31561-1-lijiang@redhat.com>
 <20180831081930.31561-3-lijiang@redhat.com>
 <20180903024512.GA2568@dhcp-128-65.nay.redhat.com>
 <d2229538-51d5-ead7-c7e5-f4afe5b53693@redhat.com>
 <20180904004417.GA9842@dhcp-128-65.nay.redhat.com>
 <20180904012902.GB9842@dhcp-128-65.nay.redhat.com>
 <20180904015131.GA10877@dhcp-128-65.nay.redhat.com>
From:   lijiang <lijiang@redhat.com>
Message-ID: <3a9ae246-0c3a-753a-41e2-f2788a882c50@redhat.com>
Date:   Wed, 5 Sep 2018 14:35:07 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20180904015131.GA10877@dhcp-128-65.nay.redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

在 2018年09月04日 09:51, Dave Young 写道:
> On 09/04/18 at 09:29am, Dave Young wrote:
>> On 09/04/18 at 08:44am, Dave Young wrote:
>>> On 09/03/18 at 10:06pm, lijiang wrote:
>>>> 在 2018年09月03日 10:45, Dave Young 写道:
>>>>> On 08/31/18 at 04:19pm, Lianbo Jiang wrote:
>>>>>> For kdump kernel, when SME is enabled, the acpi table and dmi table will need
>>>>>> to be remapped without the memory encryption mask. So we have to strengthen
>>>>>> the logic in early_memremap_pgprot_adjust(), which makes us have an opportunity
>>>>>> to adjust the memory encryption mask.
>>>>>>
>>>>>> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
>>>>>> ---
>>>>>>  arch/x86/mm/ioremap.c | 9 ++++++++-
>>>>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>>>>>> index e01e6c695add..f9d9a39955f3 100644
>>>>>> --- a/arch/x86/mm/ioremap.c
>>>>>> +++ b/arch/x86/mm/ioremap.c
>>>>>> @@ -689,8 +689,15 @@ pgprot_t __init early_memremap_pgprot_adjust(resource_size_t phys_addr,
>>>>>>  	encrypted_prot = true;
>>>>>>  
>>>>>>  	if (sme_active()) {
>>>>>> +                /*
>>>>>> +                 * In kdump kernel, the acpi table and dmi table will need
>>>>>> +                 * to be remapped without the memory encryption mask. Here
>>>>>> +                 * we have to strengthen the logic to adjust the memory
>>>>>> +                 * encryption mask.
>>>>>
>>>>> Assume the acpi/dmi tables are identical for both 1st kernel and kdump
>>>>> kernel, I'm not sure what is the difference, why need special handling
>>>>> for kdump. Can you add more explanations?
>>>>>
>>>>
>>>> Ok, i will use a dmi example to explain this issue.
>>>>
>>>> There are significant differences about E820 between the 1st kernel and kdump kernel. I pasted them at bottom.
>>>>
>>>> Firstly, we need to know how they are called.
>>>> __acpi_map_table()\                                                        / early_memremap_is_setup_data()
>>>>                    |-> early_memremap()-> early_memremap_pgprot_adjust()-> | memremap_is_efi_data()
>>>>  dmi_early_remap()/                                                        \ memremap_should_map_decrypted()-> e820__get_entry_type()
>>>>
>>>> Secondly, we also need to understand the memremap_should_map_decrypted(), which is illustrated by the fake code.
>>>> static bool memremap_should_map_decrypted(resource_size_t phys_addr,
>>>>                                           unsigned long size)
>>>> {
>>>>
>>>>     /* code ... */
>>>>
>>>>     switch (e820__get_entry_type(phys_addr, phys_addr + size - 1)) {
>>>>         case E820_TYPE_RESERVED:
>>>>         case E820_TYPE_ACPI:
>>>>         case E820_TYPE_NVS:
>>>>         case E820_TYPE_UNUSABLE:
>>>>                 /* For SEV, these areas are encrypted */
>>>>                 if (sev_active())
>>>>                         break;
>>>>                 /* Fallthrough */
>>>>
>>>>         case E820_TYPE_PRAM:
>>>>                 /* For SME, these areas are decrypted */
>>>>                 return true;
>>>>         default:
>>>>                 /* these areas are encrypted by default*/
>>>>                 break;
>>>>     }
>>>>
>>>>     return false;
>>>> }
>>>>
>>>> For the dmi case, the dmi base address is 0x6286b000 in my test machine.
>>>>
>>>> In the 1st kernel, the e820__get_entry_type() can get a valid entry and type by the dmi address, and we can also find the dmi base address from e820.
>>>> (see the 1st kernel log)
>>>> 0x6286b000 ∈ [mem 0x000000006286b000-0x000000006286efff]
>>>> So, these areas are decrypted according to the memremap_should_map_decrypted().
>>>>
>>>> In kdump kernel, the dmi base address is still 0x6286b000, but we can not find the dmi base address from e820 any more. The e820__get_entry_type() can
>>>> not get a valid entry and type by the dmi base address, it will go into the default branch. That is to say, these areas become encrypted. In fact, these
>>>> areas are also decrypted, so we have to strengthen the logic of adjusting the memory encryption mask.
>>>>
>>>>
>>>> The 1st kernel log:
>>>>
>>>> [    0.000000] BIOS-provided physical RAM map:
>>>> [    0.000000] BIOS-e820: [mem 0x0000000000000000-0x000000000008bfff] usable
>>>> [    0.000000] BIOS-e820: [mem 0x000000000008c000-0x000000000009ffff] reserved
>>>> [    0.000000] BIOS-e820: [mem 0x00000000000e0000-0x00000000000fffff] reserved
>>>> [    0.000000] BIOS-e820: [mem 0x0000000000100000-0x0000000029920fff] usable
>>>> [    0.000000] BIOS-e820: [mem 0x0000000029921000-0x0000000029921fff] reserved
>>>> [    0.000000] BIOS-e820: [mem 0x0000000029922000-0x0000000062256fff] usable
>>>> [    0.000000] BIOS-e820: [mem 0x0000000062257000-0x0000000062356fff] reserved
>>>> [    0.000000] BIOS-e820: [mem 0x0000000062357000-0x000000006235cfff] ACPI data
>>>> [    0.000000] BIOS-e820: [mem 0x000000006235d000-0x00000000623dbfff] usable
>>>> [    0.000000] BIOS-e820: [mem 0x00000000623dc000-0x000000006261bfff] reserved
>>>> [    0.000000] BIOS-e820: [mem 0x000000006261c000-0x000000006263dfff] usable
>>>> [    0.000000] BIOS-e820: [mem 0x000000006263e000-0x000000006269dfff] reserved
>>>> [    0.000000] BIOS-e820: [mem 0x000000006269e000-0x00000000627d6fff] usable
>>>> [    0.000000] BIOS-e820: [mem 0x00000000627d7000-0x00000000627e3fff] ACPI data
>>>> [    0.000000] BIOS-e820: [mem 0x00000000627e4000-0x00000000627e4fff] ACPI NVS
>>>> [    0.000000] BIOS-e820: [mem 0x00000000627e5000-0x00000000627e8fff] ACPI data
>>>> [    0.000000] BIOS-e820: [mem 0x00000000627e9000-0x00000000627eafff] usable
>>>> [    0.000000] BIOS-e820: [mem 0x00000000627eb000-0x00000000627ebfff] ACPI data
>>>> [    0.000000] BIOS-e820: [mem 0x00000000627ec000-0x000000006286afff] usable
>>>> [    0.000000] BIOS-e820: [mem 0x000000006286b000-0x000000006286efff] reserved
>>>> [    0.000000] BIOS-e820: [mem 0x000000006286f000-0x00000000682f8fff] usable
>>>> [    0.000000] BIOS-e820: [mem 0x00000000682f9000-0x0000000068b05fff] reserved
>>>> [    0.000000] BIOS-e820: [mem 0x0000000068b06000-0x0000000068b09fff] ACPI NVS
>>>> [    0.000000] BIOS-e820: [mem 0x0000000068b0a000-0x0000000068b1afff] reserved
>>>> [    0.000000] BIOS-e820: [mem 0x0000000068b1b000-0x0000000068b1dfff] ACPI NVS
>>>> [    0.000000] BIOS-e820: [mem 0x0000000068b1e000-0x0000000071d1dfff] usable
>>>> [    0.000000] BIOS-e820: [mem 0x0000000071d1e000-0x0000000071d2dfff] reserved
>>>> [    0.000000] BIOS-e820: [mem 0x0000000071d2e000-0x0000000071d3dfff] ACPI NVS
>>>> [    0.000000] BIOS-e820: [mem 0x0000000071d3e000-0x0000000071d4dfff] ACPI data
>>>> [    0.000000] BIOS-e820: [mem 0x0000000071d4e000-0x0000000077ffffff] usable
>>>> [    0.000000] BIOS-e820: [mem 0x0000000078000000-0x000000008fffffff] reserved
>>>> [    0.000000] BIOS-e820: [mem 0x00000000fed80000-0x00000000fed80fff] reserved
>>>> [    0.000000] BIOS-e820: [mem 0x0000000100000000-0x000000087effffff] usable
>>>> [    0.000000] BIOS-e820: [mem 0x000000087f000000-0x000000087fffffff] reserved
>>>>
>>>> The kdump kernel log:
>>>>
>>>> [    0.000000] BIOS-provided physical RAM map:
>>>> [    0.000000] BIOS-e820: [mem 0x0000000000001000-0x000000000008bfff] usable
>>>> [    0.000000] BIOS-e820: [mem 0x0000000052000000-0x0000000061ffffff] usable
>>>> [    0.000000] BIOS-e820: [mem 0x00000000622ee000-0x0000000062300fff] ACPI data
>>>> [    0.000000] BIOS-e820: [mem 0x0000000062301000-0x0000000062301fff] ACPI NVS
>>>> [    0.000000] BIOS-e820: [mem 0x0000000062703000-0x0000000062703fff] ACPI data
>>>> [    0.000000] BIOS-e820: [mem 0x0000000062735000-0x0000000062737fff] ACPI data
>>>> [    0.000000] BIOS-e820: [mem 0x000000006273a000-0x000000006273afff] ACPI data
>>>> [    0.000000] BIOS-e820: [mem 0x0000000068b06000-0x0000000068b09fff] ACPI NVS
>>>> [    0.000000] BIOS-e820: [mem 0x0000000068b1b000-0x0000000068b1dfff] ACPI NVS
>>>> [    0.000000] BIOS-e820: [mem 0x0000000071d2e000-0x0000000071d3dfff] ACPI NVS
>>>> [    0.000000] BIOS-e820: [mem 0x0000000071d3e000-0x0000000071d4dfff] ACPI data
>>>> [    0.000000] BIOS-e820: [mem 0x00000007fe000000-0x000000087df70fff] usable
>>>>
>>>
>>> Can you provide the efi memmap dmesg?  boot with efi=debug?
>>
>> The right way should be checking the efi mem types instead of only
>> checking is_kdump_kernel.
>>
>> Something like below, probably also check the region size with something
>> like efi_mem_range_type(addr, size), return -EINVAL in case cross
>> different type efi memory desc, added efi people in cc:
>>
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index c63a545ec199..4a24e138c0d0 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -527,6 +527,13 @@ static bool memremap_should_map_decrypted(resource_size_t phys_addr,
>>  		break;
>>  	}
>>  
>> +	if (is_kdump_kernel() {
>> +		switch (efi_mem_type(phys_addr)) {
>> +		/* refer to arch/x86/boot/compressed/eboot.c -> setup_e820()*/
>> +		case ...
>> +		}
>> +	}
>> +
>>  	return false;
>>  }
>>  
> 
> Hold on, I suppose kexec reboot also need this, but if it works without
> a fix then there might be thing to be made clear.
> 
> kexec-tools will read /proc/iomem and recreate the e820 ranges for 2nd
> kernel so in theory we should be fine without a fix.
> 

As previously mentioned, there are also many differences between kexec and kdump. In general,
kexec needs to look at all of available physical memory, but kdump doesn't need.

For kexec, kexec-tools will read /sys/firmware/memmap and recreate the e820 ranges for the 2nd
kernel. If it fails, will use /proc/iomem.

For kdump, kexec-tools will read /proc/iomem and recreate the e820 ranges for kdump kernel.
BTW: we can not get the range of persistent memory from /proc/iomem. So e820 ranges don't contain
the persistent memory in kdump kernel, this is the real reason why i need to strengthen the logic
of adjusting memory encryption mask.

If kexec-tools also use /sys/firmware/memmap for kdump(like kexec), kdump kernel can also work
without a fix, but the kexec-tools will have to be modified. Are you sure that you want me to
fix kexec-tools instead of kernel?


Thanks.

> Can you debug the kexec-tools load process about the e820 creating code
> path?
> 
>>
>>>
>>>>>> +                 */
>>>>>>  		if (early_memremap_is_setup_data(phys_addr, size) ||
>>>>>> -		    memremap_is_efi_data(phys_addr, size))
>>>>>> +		    memremap_is_efi_data(phys_addr, size) ||
>>>>>> +		    is_kdump_kernel())
>>>>>>  			encrypted_prot = false;
>>>>>>  	}
>>>>>>  
>>>>>> -- 
>>>>>> 2.17.1
>>>>>>
>>>>>
>>>>> Thanks
>>>>> Dave
>>>>>
> 
> Thanks
> Dave
>