Re: [PATCH RFC v9 07/51] x86/sev: Add the host SEV-SNP initialization support

[Kalra, Ashish]

Hello Boris,

On 6/21/2023 4:42 AM, Borislav Petkov wrote:
> On Sun, Jun 11, 2023 at 11:25:15PM -0500, Michael Roth wrote:
> > From: Brijesh Singh <brijesh.singh@xxxxxxx>
> >
> > The memory integrity guarantees of SEV-SNP are enforced through a new
> > structure called the Reverse Map Table (RMP). The RMP is a single data
> > structure shared across the system that contains one entry for every 4K
> > page of DRAM that may be used by SEV-SNP VMs. APM2 section 15.36 details
>
> Rather say 'APM v2, section "Secure Nested Paging (SEV-SNP)"' because
> the numbering is more likely to change than the name in the future. With
> the name, people can find it faster.
>
> > a number of steps needed to detect/enable SEV-SNP and RMP table support
> > on the host:
> >
> >   - Detect SEV-SNP support based on CPUID bit
> >   - Initialize the RMP table memory reported by the RMP base/end MSR
> >     registers and configure IOMMU to be compatible with RMP access
> >     restrictions
> >   - Set the MtrrFixDramModEn bit in SYSCFG MSR
> >   - Set the SecureNestedPagingEn and VMPLEn bits in the SYSCFG MSR
> >   - Configure IOMMU
> >
> > RMP table entry format is non-architectural and it can vary by
> > processor. It is defined by the PPR. Restrict SNP support to CPU
> > models/families which are compatible with the current RMP table entry
> > format to guard against any undefined behavior when running on other
> > system types. Future models/support will handle this through an
> > architectural mechanism to allow for broader compatibility.
>
> I'm guessing this is all for live migration between SNP hosts. If so,
> then there will have to be a guest API to handle the differences.

This is basically for the RMP table entry format/structure definition in
arch/x86/coco/sev/host.c, as this is non-architectural it is defined in 
a .c file instead of a header file, so that the structure remains 
private (and restricted to that file) to the SNP host code and not 
exposed to the rest of the kernel.

As mentioned in the comments above, future CPU models may support RMP 
table accesses in an architectural way.

> > SNP host code depends on CONFIG_KVM_AMD_SEV config flag, which may be
> > enabled even when CONFIG_AMD_MEM_ENCRYPT isn't set, so update the
> > SNP-specific IOMMU helpers used here to rely on CONFIG_KVM_AMD_SEV
> > instead of CONFIG_AMD_MEM_ENCRYPT.
>
> Does that mean that even on CONFIG_AMD_MEM_ENCRYPT=n kernels, host SNP
> can function?

Yes, host SNP is supposed to function with CONFIG_AMD_MEM_ENCRYPT=n.

CONFIG_AMD_MEM_ENCRYPT=y is needed for SNP guest.

> Do we even want that?
>
> I'd expect that a host SNP kernel should have SME enabled too even
> though it is not absolutely necessary.

Yes, we typically test host SNP kernel with SME enabled.

Thanks,
Ashish

> > Co-developed-by: Ashish Kalra <ashish.kalra@xxxxxxx>
> > Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
> > Co-developed-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
> > Signed-off-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
> > Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> > [mdr: rework commit message to be clearer about what patch does, squash
> >        in early_rmptable_check() handling from Tom]
> > Signed-off-by: Michael Roth <michael.roth@xxxxxxx>
> > ---
> >   arch/x86/coco/Makefile                   |   1 +
> >   arch/x86/coco/sev/Makefile               |   3 +
> >   arch/x86/coco/sev/host.c                 | 212 +++++++++++++++++++++++
> >   arch/x86/include/asm/disabled-features.h |   8 +-
> >   arch/x86/include/asm/msr-index.h         |  11 +-
> >   arch/x86/include/asm/sev.h               |   2 +
> >   arch/x86/kernel/cpu/amd.c                |  19 ++
> >   drivers/iommu/amd/init.c                 |   2 +-
> >   include/linux/amd-iommu.h                |   2 +-
> >   9 files changed, 256 insertions(+), 4 deletions(-)
> >   create mode 100644 arch/x86/coco/sev/Makefile
> >   create mode 100644 arch/x86/coco/sev/host.c
>
> Ignored review comments here:
>
> https://lore.kernel.org/r/Y9ubi0i4Z750gdMm@xxxxxxx
>
> Ignoring this one for now too.