Re: [PATCH] x86/tdx: Fix crash on kexec with CONFIG_EISA

[Tom Lendacky <thomas.lendacky@amd.com>]

On 8/26/24 07:25, Kirill A. Shutemov wrote:
> On Sat, Aug 24, 2024 at 11:29:39PM +0100, Maciej W. Rozycki wrote:
>> On Thu, 22 Aug 2024, Kirill A. Shutemov wrote:
>>
>>> This issue causes real problems:
>>>
>>> 1. If the kernel is compiled with EISA support, it will attempt to probe
>>>    EISA by reading 4 bytes from the 0x0FFFD9 address (see eisa_bus_probe()).
>>>    The kernel treats this read as MMIO and accesses this memory via
>>>    shared mapping as we do for MMIO.
>>>
>>>    KVM converts memory to shared upon such access.
>>>
>>> 2. The same memory range (0xF0000-0x100000) is scanned to look for the MP
>>>    table (see mpparse_find_mptable()). However, this is not MMIO and it
>>>    is accessed via private mapping.
>>>
>>>    This will cause a crash if the memory is not private.
>>>
>>> During normal boot, the kernel scans for SMP information before probing
>>> for EISA, and it boots fine. However, the memory becomes shared and causes
>>> issues on kexec when the second kernel attempts to scan for SMP information.
>>
>>  ISTM that `eisa_bus_probe' has to be updated to `memremap' analogously to 
>> `mpparse_find_mptable', complementing changes such as commit f7750a795687 
>> ("x86, mpparse, x86/acpi, x86/PCI, x86/dmi, SFI: Use memremap() for RAM 
>> mappings") or commit 5997efb96756 ("x86/boot: Use memremap() to map the 
>> MPF and MPC data").  Both just access BIOS memory.
>>
>>  Can you please try and verify if my proposed change at: 
>> <https://lore.kernel.org/r/alpine.DEB.2.21.2408242025210.30766@angie.orcam.me.uk> 
>> has fixed the problem for you?
> 
> I like the direction your patch took. I hate sprinkling
> X86_FEATURE_TDX_GUEST checks over the kernel.
> 
> Unfortunately, it is not enough to fix the issue. memremap() in this case
> will still boil down to ioremap() that would set shared bit:
> 
> memremap()
>   arch_memremap_wb()
>     ioremap_cache()
>       __ioremap_caller(.encrytped = false)
> 
> I think arch_memremap_wb() should be mapped ioremap_encrypted() in x86
> case. See the patch below.
> 
> It seems to be working fine on TDX, but I am not sure about SEV.
> 
> Tom, any comments?

I haven't dug through the code that thoroughly, but I don't think making
arch_memremap_wb() be ioremap_encrypted() will work for SME, where some
data, e.g. setup data, is unencrypted and needs to be mapped shared.

Let me add @Ashish to the thread and have him investigate this since he
has been working on the kexec support under SNP. Can someone provide the
specific kernel options that need to be in place?

Thanks,
Tom

> 
> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> index 1d60427379c9..ef79cbef1ef8 100644
> --- a/arch/x86/include/asm/io.h
> +++ b/arch/x86/include/asm/io.h
> @@ -180,6 +180,8 @@ extern void __iomem *ioremap_prot(resource_size_t offset, unsigned long size, un
>  extern void __iomem *ioremap_encrypted(resource_size_t phys_addr, unsigned long size);
>  #define ioremap_encrypted ioremap_encrypted
>  
> +#define arch_memremap_wb ioremap_encrypted
> +
>  /**
>   * ioremap     -   map bus memory into CPU space
>   * @offset:    bus address of the memory