lkml.org 
[lkml]   [2022]   [Jul]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH Part2 v6 03/49] x86/sev: Add the host SEV-SNP initialization support
    On Mon, Jun 20, 2022 at 11:02:01PM +0000, Ashish Kalra wrote:
    > +/*
    > + * The first 16KB from the RMP_BASE is used by the processor for the
    > + * bookkeeping, the range need to be added during the RMP entry lookup.

    needs

    > +static int __snp_enable(unsigned int cpu)
    > +{
    > + u64 val;
    > +
    > + if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
    > + return 0;
    > +
    > + rdmsrl(MSR_AMD64_SYSCFG, val);
    > +
    > + val |= MSR_AMD64_SYSCFG_SNP_EN;
    > + val |= MSR_AMD64_SYSCFG_SNP_VMPL_EN;
    > +
    > + wrmsrl(MSR_AMD64_SYSCFG, val);
    > +
    > + return 0;
    > +}
    > +
    > +static __init void snp_enable(void *arg)
    > +{
    > + __snp_enable(smp_processor_id());
    > +}

    Get rid of that silly wrapper - you're not even using that @cpu argument.

    > +static bool get_rmptable_info(u64 *start, u64 *len)
    > +{
    > + u64 calc_rmp_sz, rmp_sz, rmp_base, rmp_end, nr_pages;
    > +
    > + rdmsrl(MSR_AMD64_RMP_BASE, rmp_base);
    > + rdmsrl(MSR_AMD64_RMP_END, rmp_end);
    > +
    > + if (!rmp_base || !rmp_end) {
    > + pr_info("Memory for the RMP table has not been reserved by BIOS\n");

    pr_err

    > + return false;
    > + }
    > +
    > + rmp_sz = rmp_end - rmp_base + 1;
    > +
    > + /*
    > + * Calculate the amount the memory that must be reserved by the BIOS to
    > + * address the full system RAM. The reserved memory should also cover the

    "... address the whole RAM."

    > + * RMP table itself.
    > + *
    > + * See PPR Family 19h Model 01h, Revision B1 section 2.1.4.2 for more
    > + * information on memory requirement.

    That section number will change over time - if you want to refer to some
    section just use its title so that people can at least grep for the
    relevant text.

    > + */
    > + nr_pages = totalram_pages();
    > + calc_rmp_sz = (((rmp_sz >> PAGE_SHIFT) + nr_pages) << 4) + RMPTABLE_CPU_BOOKKEEPING_SZ;

    use totalram_pages() directly and get rid of nr_pages.

    > +
    > + if (calc_rmp_sz > rmp_sz) {
    > + pr_info("Memory reserved for the RMP table does not cover full system RAM (expected 0x%llx got 0x%llx)\n",
    > + calc_rmp_sz, rmp_sz);

    pr_err

    > + return false;
    > + }
    > +
    > + *start = rmp_base;
    > + *len = rmp_sz;
    > +
    > + pr_info("RMP table physical address 0x%016llx - 0x%016llx\n", rmp_base, rmp_end);

    "RMP table physical address range: ...[0x.. - 0x..]"

    > +
    > + return true;
    > +}
    > +
    > +static __init int __snp_rmptable_init(void)

    s/int/bool/

    > +{
    > + u64 rmp_base, sz;
    > + void *start;
    > + u64 val;
    > +
    > + if (!get_rmptable_info(&rmp_base, &sz))
    > + return 1;
    > +
    > + start = memremap(rmp_base, sz, MEMREMAP_WB);
    > + if (!start) {
    > + pr_err("Failed to map RMP table 0x%llx+0x%llx\n", rmp_base, sz);
    ^^^^^^

    either write the size in decimal or do a normal interval.

    > + return 1;
    > + }
    > +
    > + /*
    > + * Check if SEV-SNP is already enabled, this can happen if we are coming from

    Who is "we"?

    Pls get rid of all "we" in the comments and use passive formulations.

    > + * kexec boot.
    > + */
    > + rdmsrl(MSR_AMD64_SYSCFG, val);
    > + if (val & MSR_AMD64_SYSCFG_SNP_EN)
    > + goto skip_enable;
    > +
    > + /* Initialize the RMP table to zero */
    > + memset(start, 0, sz);

    Do I understand it correctly that in the kexec case the second, kexec-ed
    kernel is reusing the previous kernel's RMP table so it should not be
    cleared?

    > +
    > + /* Flush the caches to ensure that data is written before SNP is enabled. */
    > + wbinvd_on_all_cpus();
    > +
    > + /* Enable SNP on all CPUs. */
    > + on_each_cpu(snp_enable, NULL, 1);
    > +
    > +skip_enable:
    > + rmptable_start = (unsigned long)start;
    > + rmptable_end = rmptable_start + sz;
    > +
    > + return 0;
    > +}
    > +
    > +static int __init snp_rmptable_init(void)
    > +{
    > + if (!boot_cpu_has(X86_FEATURE_SEV_SNP))

    cpu_feature_enabled

    > + return 0;
    > +
    > + if (!iommu_sev_snp_supported())
    > + goto nosnp;
    > +
    > + if (__snp_rmptable_init())
    > + goto nosnp;
    > +
    > + cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "x86/rmptable_init:online", __snp_enable, NULL);
    > +
    > + return 0;
    > +
    > +nosnp:
    > + setup_clear_cpu_cap(X86_FEATURE_SEV_SNP);
    > + return 1;
    > +}
    > +
    > +/*
    > + * This must be called after the PCI subsystem. This is because before enabling
    > + * the SNP feature we need to ensure that IOMMU supports the SEV-SNP feature.
    > + * The iommu_sev_snp_support() is used for checking the feature, and it is
    > + * available after subsys_initcall().

    I'd much more appreciate here a short formulation explaining why is
    IOMMU needed for SNP rather than the obvious.

    Thx.

    --
    Regards/Gruss,
    Boris.

    https://people.kernel.org/tglx/notes-about-netiquette

    \
     
     \ /
      Last update: 2022-07-17 12:02    [W:8.897 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site