TDX has concept of private and shared memory. Private memory is
protected from VMM and shared is used for communication between guest
and host. Shared memory is mapped into guest with shared bit set in the
page table entry.

Normally memory got converted from private to shared with MapGPA
TDVMCALL before it got accessed. However, what is supposed to happen
when private memory is accessed via shared mapping is not defined.
It is left up to the VMM to decide.

This is problematic. Currently, KVM implicitly converts memory to shared
and removes the page from the secure EPT, so subsequent access via private
mapping leads to a crash.

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.

TDX behaviour on access of private memory via shared mapping has to be
clarified to avoid such crashes in the future. It takes time.

In meanwhile, avoid probing EISA for TDX guest.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Kevin Loughlin <kevinloughlin@google.com>
Cc: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: Isaku Yamahata <isaku.yamahata@intel.com>
---
 arch/x86/kernel/eisa.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/eisa.c b/arch/x86/kernel/eisa.c
index 53935b4d62e3..678244d178b6 100644
--- a/arch/x86/kernel/eisa.c
+++ b/arch/x86/kernel/eisa.c
@@ -13,7 +13,9 @@ static __init int eisa_bus_probe(void)
 {
 	void __iomem *p;
 
-	if ((xen_pv_domain() && !xen_initial_domain()) || cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
+	if ((xen_pv_domain() && !xen_initial_domain()) ||
+	    cc_platform_has(CC_ATTR_GUEST_SEV_SNP) ||
+	    cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
 		return 0;
 
 	p = ioremap(0x0FFFD9, 4);
-- 
2.43.0

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?

  Maciej

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?

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
-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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

Hello Kirill,

On 8/26/2024 10:52 AM, Tom Lendacky wrote:
> 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?

As Tom asked for, please provide the specific kernel options to test with this configuration.

Thanks, Ashish

>
> 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

On Tue, Aug 27, 2024 at 05:15:56PM -0500, Kalra, Ashish wrote:
> Hello Kirill,
> 
> On 8/26/2024 10:52 AM, Tom Lendacky wrote:
> > 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?
> 
> As Tom asked for, please provide the specific kernel options to test
> with this configuration.

It is not about testing a specific configuration. The question is if it
safe for memremap() to map all WB memory as encrypted by default.

Looks like it is safe for TDX, but I am not sure about SME/SEV.

Maybe we want a specific flag to make memremap() map WB memory as
decrypted/shared. Make everything encrypted by default seems like a sane
default.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Hello Kirill,

On 8/28/2024 1:21 AM, Kirill A. Shutemov wrote:
> On Tue, Aug 27, 2024 at 05:15:56PM -0500, Kalra, Ashish wrote:
>> Hello Kirill,
>>
>> On 8/26/2024 10:52 AM, Tom Lendacky wrote:
>>> 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?
>> As Tom asked for, please provide the specific kernel options to test
>> with this configuration.
> It is not about testing a specific configuration. The question is if it
> safe for memremap() to map all WB memory as encrypted by default.
>
> Looks like it is safe for TDX, but I am not sure about SME/SEV.

For SEV it may make sense, but for SME we don't want memremap() to map all WB memory as encrypted by default.

>
> Maybe we want a specific flag to make memremap() map WB memory as
> decrypted/shared. Make everything encrypted by default seems like a sane
> default.

What are MEMREMAP_ENC, MEMREMAP_DEC flags being used for currently ?

Thanks, Ashish

On Wed, Aug 28, 2024 at 02:43:23PM -0500, Kalra, Ashish wrote:
> Hello Kirill,
> 
> On 8/28/2024 1:21 AM, Kirill A. Shutemov wrote:
> > On Tue, Aug 27, 2024 at 05:15:56PM -0500, Kalra, Ashish wrote:
> >> Hello Kirill,
> >>
> >> On 8/26/2024 10:52 AM, Tom Lendacky wrote:
> >>> 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?
> >> As Tom asked for, please provide the specific kernel options to test
> >> with this configuration.
> > It is not about testing a specific configuration. The question is if it
> > safe for memremap() to map all WB memory as encrypted by default.
> >
> > Looks like it is safe for TDX, but I am not sure about SME/SEV.
> 
> For SEV it may make sense, but for SME we don't want memremap() to map
> all WB memory as encrypted by default.

Could you elaborate why?

I mean if it is specific memory ranges that can be identified as such we
could make exception for them.

> >
> > Maybe we want a specific flag to make memremap() map WB memory as
> > decrypted/shared. Make everything encrypted by default seems like a sane
> > default.
> 
> What are MEMREMAP_ENC, MEMREMAP_DEC flags being used for currently ?

Good question.

I see zero use of MEMREMAP_ENC in the kernel. And MEMREMAP_DEC only used
for setup data which I believe AMD thing.

If it is the only memory that must be decrypted, I guess we can make it
work with encrypted by default.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Hello Kirill,

On 8/29/2024 7:53 AM, Kirill A. Shutemov wrote:
> On Wed, Aug 28, 2024 at 02:43:23PM -0500, Kalra, Ashish wrote:
>> Hello Kirill,
>>
>> On 8/28/2024 1:21 AM, Kirill A. Shutemov wrote:
>>>>>> 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?
>>>> As Tom asked for, please provide the specific kernel options to test
>>>> with this configuration.
>>> It is not about testing a specific configuration. The question is if it
>>> safe for memremap() to map all WB memory as encrypted by default.
>>>
>>> Looks like it is safe for TDX, but I am not sure about SME/SEV.
>> For SEV it may make sense, but for SME we don't want memremap() to map
>> all WB memory as encrypted by default.
> Could you elaborate why?
>
> I mean if it is specific memory ranges that can be identified as such we
> could make exception for them.
Looks like that the exception is already made for some of these memory ranges with MEMREMAP_DEC flag set along with MEMREMAP_WB.
>>> Maybe we want a specific flag to make memremap() map WB memory as
>>> decrypted/shared. Make everything encrypted by default seems like a sane
>>> default.
>> What are MEMREMAP_ENC, MEMREMAP_DEC flags being used for currently ?
> Good question.
>
> I see zero use of MEMREMAP_ENC in the kernel. And MEMREMAP_DEC only used
> for setup data which I believe AMD thing.
>
> If it is the only memory that must be decrypted, I guess we can make it
> work with encrypted by default.

Yes, and this looks pretty much covered with:

arch_memremap_can_ram_remap(..)

-> if (CC_ATTR_HOST_MEM_REMAP)

--> memremap_is_setup_data()

---> memremap(.., MEMREMAP_WB | MEMREMAP_DEC);

It does look like that it can work with encrypted by default and any memory ranges which need special handling and need to be setup as decrypted can use MEMREMAP_WB | MEMREMAP_DEC flag.

Thanks, Ashish

On Fri, Aug 30, 2024 at 03:47:50PM -0500, Kalra, Ashish wrote:
> Hello Kirill,
> 
> On 8/29/2024 7:53 AM, Kirill A. Shutemov wrote:
> > On Wed, Aug 28, 2024 at 02:43:23PM -0500, Kalra, Ashish wrote:
> >> Hello Kirill,
> >>
> >> On 8/28/2024 1:21 AM, Kirill A. Shutemov wrote:
> >>>>>> 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?
> >>>> As Tom asked for, please provide the specific kernel options to test
> >>>> with this configuration.
> >>> It is not about testing a specific configuration. The question is if it
> >>> safe for memremap() to map all WB memory as encrypted by default.
> >>>
> >>> Looks like it is safe for TDX, but I am not sure about SME/SEV.
> >> For SEV it may make sense, but for SME we don't want memremap() to map
> >> all WB memory as encrypted by default.
> > Could you elaborate why?
> >
> > I mean if it is specific memory ranges that can be identified as such we
> > could make exception for them.
> Looks like that the exception is already made for some of these memory ranges with MEMREMAP_DEC flag set along with MEMREMAP_WB.
> >>> Maybe we want a specific flag to make memremap() map WB memory as
> >>> decrypted/shared. Make everything encrypted by default seems like a sane
> >>> default.
> >> What are MEMREMAP_ENC, MEMREMAP_DEC flags being used for currently ?
> > Good question.
> >
> > I see zero use of MEMREMAP_ENC in the kernel. And MEMREMAP_DEC only used
> > for setup data which I believe AMD thing.
> >
> > If it is the only memory that must be decrypted, I guess we can make it
> > work with encrypted by default.
> 
> Yes, and this looks pretty much covered with:
> 
> arch_memremap_can_ram_remap(..)
> 
> -> if (CC_ATTR_HOST_MEM_REMAP)
> 
> --> memremap_is_setup_data()
> 
> ---> memremap(.., MEMREMAP_WB | MEMREMAP_DEC);
> 
> It does look like that it can work with encrypted by default and any memory ranges which need special handling and need to be setup as decrypted can use MEMREMAP_WB | MEMREMAP_DEC flag.
> 

Could you actually test the patch that makes memremap() map WB memory as
encrypted by default:

https://lore.kernel.org/all/g3tswlyhrnuzfqf2upq6h23manyrzhnxttnay66nycy2moi4es@5n4oblzpzcjc/

?

If it works, I will submit a proper patch.


-- 
  Kiryl Shutsemau / Kirill A. Shutemov