Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <8d3ac4ca-055a-5d54-602c-e378643ad9cd@intel.com>
Date:   Mon, 21 Nov 2022 13:00:35 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.2
Subject: Re: [PATCH 3/6] x86/tdx: Support vmalloc() for
 tdx_enc_status_changed()
Content-Language: en-US
To:     Dexuan Cui <decui@microsoft.com>, ak@linux.intel.com,
        arnd@arndb.de, bp@alien8.de, brijesh.singh@amd.com,
        dan.j.williams@intel.com, dave.hansen@linux.intel.com,
        haiyangz@microsoft.com, hpa@zytor.com, jane.chu@oracle.com,
        kirill.shutemov@linux.intel.com, kys@microsoft.com,
        linux-arch@vger.kernel.org, linux-hyperv@vger.kernel.org,
        luto@kernel.org, mingo@redhat.com, peterz@infradead.org,
        rostedt@goodmis.org, sathyanarayanan.kuppuswamy@linux.intel.com,
        seanjc@google.com, tglx@linutronix.de, tony.luck@intel.com,
        wei.liu@kernel.org, x86@kernel.org
Cc:     linux-kernel@vger.kernel.org
References: <20221121195151.21812-1-decui@microsoft.com>
 <20221121195151.21812-4-decui@microsoft.com>
From:   Dave Hansen <dave.hansen@intel.com>
In-Reply-To: <20221121195151.21812-4-decui@microsoft.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 11/21/22 11:51, Dexuan Cui wrote:
> -static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
> +static bool tdx_enc_status_changed_for_contiguous_pages(unsigned long vaddr,
> +							int numpages, bool enc)

That naming is unfortunate.

First, it's getting way too long.

Second, you don't need two of these functions because it's contiguous or
not.  It's because tdx_enc_status_changed() only works on the direct map.

>  {
>  	phys_addr_t start = __pa(vaddr);
>  	phys_addr_t end   = __pa(vaddr + numpages * PAGE_SIZE);
> @@ -798,6 +800,47 @@ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
>  	return true;
>  }
>  
> +static bool tdx_enc_status_changed_for_vmalloc(unsigned long vaddr,
> +					       int numpages, bool enc)
> +{
> +	void *start_va = (void *)vaddr;
> +	void *end_va = start_va + numpages * PAGE_SIZE;
> +	phys_addr_t pa;
> +
> +	if (offset_in_page(vaddr) != 0)
> +		return false;
> +
> +	while (start_va < end_va) {
> +		pa = slow_virt_to_phys(start_va);
> +		if (!enc)
> +			pa |= cc_mkdec(0);
> +
> +		if (!tdx_map_gpa(pa, pa + PAGE_SIZE, enc))
> +			return false;
> +
> +		/*
> +		 * private->shared conversion requires only MapGPA call.
> +		 *
> +		 * For shared->private conversion, accept the page using
> +		 * TDX_ACCEPT_PAGE TDX module call.
> +		 */
> +		if (enc && !try_accept_one(&pa, PAGE_SIZE, PG_LEVEL_4K))
> +			return false;

Don't we support large vmalloc() mappings these days?

> +		start_va += PAGE_SIZE;
> +	}
> +
> +	return true;
> +}

I really don't like the copy-and-paste fork here.

I'd almost just rather have this *one* "vmalloc" copy that does
slow_virt_to_phys() on direct map addresses than have two copies.

Can you please look into making *one* function that works on either kind
of mapping?

> +static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
> +{
> +	if (is_vmalloc_addr((void *)vaddr))
> +		return tdx_enc_status_changed_for_vmalloc(vaddr, numpages, enc);
> +
> +	return tdx_enc_status_changed_for_contiguous_pages(vaddr, numpages, enc);
> +}
> +
>  void __init tdx_early_init(void)
>  {
>  	u64 cc_mask;