Message-ID: <f664f44d-fc0a-1af7-070c-306ab100bbc9@intel.com>
Date:   Wed, 1 Mar 2023 15:37:00 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH RFC v8 48/56] KVM: SVM: Add SNP-specific handling for
 memory attribute updates
Content-Language: en-US
To:     Michael Roth <michael.roth@amd.com>, kvm@vger.kernel.org
Cc:     linux-coco@lists.linux.dev, linux-mm@kvack.org,
        linux-crypto@vger.kernel.org, x86@kernel.org,
        linux-kernel@vger.kernel.org, tglx@linutronix.de, mingo@redhat.com,
        jroedel@suse.de, thomas.lendacky@amd.com, hpa@zytor.com,
        ardb@kernel.org, pbonzini@redhat.com, seanjc@google.com,
        vkuznets@redhat.com, jmattson@google.com, luto@kernel.org,
        dave.hansen@linux.intel.com, slp@redhat.com, pgonda@google.com,
        peterz@infradead.org, srinivas.pandruvada@linux.intel.com,
        rientjes@google.com, dovmurik@linux.ibm.com, tobin@ibm.com,
        bp@alien8.de, vbabka@suse.cz, kirill@shutemov.name,
        ak@linux.intel.com, tony.luck@intel.com, marcorr@google.com,
        sathyanarayanan.kuppuswamy@linux.intel.com, alpergun@google.com,
        dgilbert@redhat.com, jarkko@kernel.org, ashish.kalra@amd.com,
        nikunj.dadhania@amd.com
References: <20230220183847.59159-1-michael.roth@amd.com>
 <20230220183847.59159-49-michael.roth@amd.com>
From:   Dave Hansen <dave.hansen@intel.com>
In-Reply-To: <20230220183847.59159-49-michael.roth@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/20/23 10:38, Michael Roth wrote:
> +		/*
> +		 * TODO: The RMP entry's hugepage bit is ignored for
> +		 * shared/unassigned pages. Either handle looping through each
> +		 * sub-page as part of snp_make_page_shared(), or remove the
> +		 * level argument.
> +		 */
> +		if (op == SNP_PAGE_STATE_PRIVATE && order &&
> +		    IS_ALIGNED(gfn, 1 << order) && (gfn + (1 << order)) <= end) {
> +			level = order_to_level(order);
> +			npages = 1 << order;
> +		}

That's a wee bit obtuse.

First of all, I assume that the 'RFC' is because of these TODOs and they
won't survive to the point when you ask for this to be merged.

BTW, what keeps the restrictedmem_get_page() offset and the gfn aligned?

Let's start with this:

> +static inline u8 order_to_level(int order)
> +{
> +	BUILD_BUG_ON(KVM_MAX_HUGEPAGE_LEVEL > PG_LEVEL_1G);
> +
> +	if (order >= KVM_HPAGE_GFN_SHIFT(PG_LEVEL_1G))
> +		return PG_LEVEL_1G;
> +
> +	if (order >= KVM_HPAGE_GFN_SHIFT(PG_LEVEL_2M))
> +		return PG_LEVEL_2M;
> +
> +	return PG_LEVEL_4K;
> +}

Right now, 'order' comes only from restrictedmem_get_page(), which I dug
out of:

> https://github.com/mdroth/linux/commit/c6792672cd11737fd255dff10b2d5b6bccc626a8

That order is *only* filled in by THPs.  That makes the PG_LEVEL_1G
stuff here kinda silly.  I guess it might be seen as thorough, but it's
dead code.  I'd probably just make this work on order==9 || order==0 and
warn on anything else.

I'd also highly recommend some comments about how racy this all is.  I
guess it probably works, but it would be good to add some comments about
page splits and collapsing.

It's also not obvious why this only cares about private pages.

Anyway, this is the exact kind of thing where I really like a
well-commented helper:

bool can_install_large_rmp_entry(gfn, order)
{
	// small pages, blah blah
	if (!order)
		return false;

	// The region being updated must be aligned
	if (!IS_ALIGNED(gfn, 1 << order))
		return false;
	// ... and fit
	if (gfn + (1 << order)) > end)
		return false;

	return true;	
}

Which gets used like this:

	if (op == SNP_PAGE_STATE_PRIVATE &&
	    can_install_large_rmp_entry(gfn, order)) {
		level = ...
	}