Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
From:   "Wang, Wei W" <wei.w.wang@intel.com>
To:     "Yamahata, Isaku" <isaku.yamahata@intel.com>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
CC:     "Yamahata, Isaku" <isaku.yamahata@intel.com>,
        "isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        "Aktas, Erdem" <erdemaktas@google.com>,
        "Christopherson,, Sean" <seanjc@google.com>,
        "Shahar, Sagi" <sagis@google.com>,
        David Matlack <dmatlack@google.com>,
        "Huang, Kai" <kai.huang@intel.com>,
        Zhi Wang <zhi.wang.linux@gmail.com>,
        "Chen, Bo2" <chen.bo@intel.com>,
        "Yuan, Hang" <hang.yuan@intel.com>,
        "Zhang, Tina" <tina.zhang@intel.com>,
        "gkirkpatrick@google.com" <gkirkpatrick@google.com>
Subject: RE: [PATCH v16 059/116] KVM: TDX: Create initial guest memory
Thread-Topic: [PATCH v16 059/116] KVM: TDX: Create initial guest memory
Thread-Index: AQHaAEyrBpVPWseLVU+0s47fedmgi7B+Z8cg
Date:   Fri, 17 Nov 2023 12:56:32 +0000
Message-ID: <DS0PR11MB6373EC1033F88008D3B71568DCB7A@DS0PR11MB6373.namprd11.prod.outlook.com>
References: <cover.1697471314.git.isaku.yamahata@intel.com>
 <edccd3a8ee2ca8d96baca097546bc131f1ef3b79.1697471314.git.isaku.yamahata@intel.com>
In-Reply-To: <edccd3a8ee2ca8d96baca097546bc131f1ef3b79.1697471314.git.isaku.yamahata@intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?nhuTUdloUVrUL0WSY9Prg8chGxccEkJ/cUXckHNegrMRa/pnHbnJkMZJlG9W?=
 =?us-ascii?Q?Ith4AQsYhgGhGskjwHjghbZ2r7rCFfKMJ1FQzHfjJGhN8JO7kgaC97TAjG1o?=
 =?us-ascii?Q?DAcfGxV8o9hwlppu9OXQ4uAimZ1R7/V1dHTuqaLoBU9cBuRxHcsnGsni05et?=
 =?us-ascii?Q?ktoDM3BdMDKrOU1eCBdmSX7o1Bihxib2ypbj4KlVEduQF09JKvrwhLfgo4IA?=
 =?us-ascii?Q?araKEASLnLlxNIbyn8bCan832yiIzC0IsVrdYqNtNVPYV5CIPZqOsrnX3KcN?=
 =?us-ascii?Q?cKjzjPJ6aBg3ZDMiWnHIlf44IcHguweLyJrTf2iI0CBYMx7gbJvM+VJ1fgcI?=
 =?us-ascii?Q?2BOzXYwKjWBY9JrcROv42sDmoLp+XKeN4iWP2eH74zGbqpQrO8x2VHC7aSjc?=
 =?us-ascii?Q?4vdmV28DetkAksiOeF8lhyW/UNnInkFRlbTAispgb5rIwljuvL4tcyLI0WQz?=
 =?us-ascii?Q?h8zXpcXKRJubnp+nzj6RmnORCJe9AKmI7uJijd7IH3AxBFgT9z3lymdTSGoH?=
 =?us-ascii?Q?YdzBVZiO5TecnWo7axteti3bxKaBOj4oaMxtoeYLMentXcoB2XEfecmChssY?=
 =?us-ascii?Q?vWWKr+7mtlVCcaL4clwJbM6uNdm4LPYD/Z08ru0KbO7AkK/5WzzHL+i1Zz6i?=
 =?us-ascii?Q?hw4NtCUYM//FJFjULgelpSwIjmOOfYGleQ2V2acZN3xO7aiQgDmURrFrh4Lp?=
 =?us-ascii?Q?4dhBFGyME/PnRjdOnpaFchmyygtYU41iunP57IWyEC6yUymVcl59bPC27gsD?=
 =?us-ascii?Q?tlGsuNuAd+aBfkNhmmvTdIt32vRKHeEOFiPYkM5BrNFUVcjvJtVRph+mwHia?=
 =?us-ascii?Q?UUnB8H4a6OGlmCt9CMj9xTKxHF6uFO9KmLmjNY+3IszLJcxqO0oyUn6aNyGh?=
 =?us-ascii?Q?KUlLvwiJ5q2GrDzilONIEZMSzXydynd1V9GxMtLm0A/SC4GchDw2kN47BQ7G?=
 =?us-ascii?Q?VCXid5e7PfW1rylhJIKDyGi2UhfAJ677aB4XLgpNJDwpwrFcZKVoIpN/CbOT?=
 =?us-ascii?Q?D8G3rWzR6a9GudaKF8+q9BptdWbeW52vUmUBZ2xse2zoU8HqCqeYDa2kU1wh?=
 =?us-ascii?Q?mO8XVqXpSpflgsgte4cvzUiiEnhRqaOKIxnTHenQGuRuCNmd30x7oDgQ9Ozq?=
 =?us-ascii?Q?OFFn8xGIUgDa1YqvhS5NntLQ9M8U3yF/fpZ4MwaOVMjPXgRZasKBVtaYdDdU?=
 =?us-ascii?Q?Wmzcrlooc0QYkWlR+uI/Cu1amsq0MbV9wNFQt5IenXcS8VbWANxoAdwv4L//?=
 =?us-ascii?Q?XwKEkoYJ2kP/Um5+Et8nbQcfJnB7s47Yx2N3MjtAQrtbQNoqoHXvyYRcIpIl?=
 =?us-ascii?Q?rzBBBAfXl8DwThmB0YxtHcrzpp/RcuoIQDYvxpI0RxtWEyOlrODSN24VmOvd?=
 =?us-ascii?Q?b9RVpYPfLcrywI1roIwfHqCvQXgWfw7rLUNsIsEUXPG17rtlldAbHx+ELl56?=
 =?us-ascii?Q?yc+cVj4U19c9yhd0nfg/bG5Y9pncUt4TT3rPAOSs6zoEJPxUEJVDpTDFnayy?=
 =?us-ascii?Q?xGjj3ZEZuQ2woC8iDBoJ3SGSk7cf+vRJJpbrCvGTqwQ+TMHet6HxUvZNuRSS?=
 =?us-ascii?Q?b9FIFE1ssq30fdD26JYwAN+PbbpvSIWmWd1G0Q9M?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB6373.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 030d0376-2dbb-480f-5837-08dbe76c9ffc
X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Nov 2023 12:56:32.9460
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 2mBsXMvyFi7/sGY6uYlEqN5zTGagdTeytKx/quSG51EA0L8syrkc7e+bcG5yeuIeDhXE8RFtZO2v2oiauiuCEQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB6848
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Fri, 17 Nov 2023 04:57:04 -0800 (PST)

On Tuesday, October 17, 2023 12:14 AM, isaku.yamahata@intel.com wrote:
> Because the guest memory is protected in TDX, the creation of the initial=
 guest
> memory requires a dedicated TDX module API, tdh_mem_page_add, instead of
> directly copying the memory contents into the guest memory in the case of
> the default VM type.  KVM MMU page fault handler callback, private_page_a=
dd,
> handles it.
>=20
> Define new subcommand, KVM_TDX_INIT_MEM_REGION, of VM-scoped
> KVM_MEMORY_ENCRYPT_OP.  It assigns the guest page, copies the initial
> memory contents into the guest memory, encrypts the guest memory.  At the
> same time, optionally it extends memory measurement of the TDX guest.  It
> calls the KVM MMU page fault(EPT-violation) handler to trigger the callba=
cks
> for it.
>=20
> Reported-by: gkirkpatrick@google.com
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
>=20
> ---
> v15 -> v16:
> - add check if nr_pages isn't large with
>   (nr_page << PAGE_SHIFT) >> PAGE_SHIFT
>=20
> v14 -> v15:
> - add a check if TD is finalized or not to tdx_init_mem_region()
> - return -EAGAIN when partial population
> ---
>  arch/x86/include/uapi/asm/kvm.h       |   9 ++
>  arch/x86/kvm/mmu/mmu.c                |   1 +
>  arch/x86/kvm/vmx/tdx.c                | 167 +++++++++++++++++++++++++-
>  arch/x86/kvm/vmx/tdx.h                |   2 +
>  tools/arch/x86/include/uapi/asm/kvm.h |   9 ++
>  5 files changed, 185 insertions(+), 3 deletions(-)
>=20
> diff --git a/arch/x86/include/uapi/asm/kvm.h
> b/arch/x86/include/uapi/asm/kvm.h index 311a7894b712..a1815fcbb0be
> 100644
> --- a/arch/x86/include/uapi/asm/kvm.h
> +++ b/arch/x86/include/uapi/asm/kvm.h
> @@ -572,6 +572,7 @@ enum kvm_tdx_cmd_id {
>  	KVM_TDX_CAPABILITIES =3D 0,
>  	KVM_TDX_INIT_VM,
>  	KVM_TDX_INIT_VCPU,
> +	KVM_TDX_INIT_MEM_REGION,
>=20
>  	KVM_TDX_CMD_NR_MAX,
>  };
> @@ -645,4 +646,12 @@ struct kvm_tdx_init_vm {
>  	struct kvm_cpuid2 cpuid;
>  };
>=20
> +#define KVM_TDX_MEASURE_MEMORY_REGION	(1UL << 0)
> +
> +struct kvm_tdx_init_mem_region {
> +	__u64 source_addr;
> +	__u64 gpa;
> +	__u64 nr_pages;
> +};
> +
>  #endif /* _ASM_X86_KVM_H */
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index
> 107cf27505fe..63a4efd1e40a 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -5652,6 +5652,7 @@ int kvm_mmu_load(struct kvm_vcpu *vcpu)
>  out:
>  	return r;
>  }
> +EXPORT_SYMBOL(kvm_mmu_load);
>=20
>  void kvm_mmu_unload(struct kvm_vcpu *vcpu)  { diff --git
> a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c index
> a5f1b3e75764..dc17c212cb38 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -470,6 +470,21 @@ void tdx_load_mmu_pgd(struct kvm_vcpu *vcpu,
> hpa_t root_hpa, int pgd_level)
>  	td_vmcs_write64(to_tdx(vcpu), SHARED_EPT_POINTER, root_hpa &
> PAGE_MASK);  }
>=20
> +static void tdx_measure_page(struct kvm_tdx *kvm_tdx, hpa_t gpa) {
> +	struct tdx_module_args out;
> +	u64 err;
> +	int i;
> +
> +	for (i =3D 0; i < PAGE_SIZE; i +=3D TDX_EXTENDMR_CHUNKSIZE) {
> +		err =3D tdh_mr_extend(kvm_tdx->tdr_pa, gpa + i, &out);
> +		if (KVM_BUG_ON(err, &kvm_tdx->kvm)) {
> +			pr_tdx_error(TDH_MR_EXTEND, err, &out);
> +			break;
> +		}
> +	}
> +}
> +
>  static void tdx_unpin(struct kvm *kvm, kvm_pfn_t pfn)  {
>  	struct page *page =3D pfn_to_page(pfn);
> @@ -533,6 +548,61 @@ static int tdx_sept_page_aug(struct kvm *kvm, gfn_t
> gfn,
>  	return 0;
>  }
>=20
> +static int tdx_sept_page_add(struct kvm *kvm, gfn_t gfn,
> +			     enum pg_level level, kvm_pfn_t pfn) {
> +	struct kvm_tdx *kvm_tdx =3D to_kvm_tdx(kvm);
> +	hpa_t hpa =3D pfn_to_hpa(pfn);
> +	gpa_t gpa =3D gfn_to_gpa(gfn);
> +	struct tdx_module_args out;
> +	hpa_t source_pa;
> +	bool measure;
> +	u64 err;
> +
> +	/*
> +	 * KVM_INIT_MEM_REGION, tdx_init_mem_region(), supports only 4K
> page
> +	 * because tdh_mem_page_add() supports only 4K page.
> +	 */
> +	if (KVM_BUG_ON(level !=3D PG_LEVEL_4K, kvm))
> +		return -EINVAL;
> +
> +	/*
> +	 * In case of TDP MMU, fault handler can run concurrently.  Note
> +	 * 'source_pa' is a TD scope variable, meaning if there are multiple
> +	 * threads reaching here with all needing to access 'source_pa', it
> +	 * will break.  However fortunately this won't happen, because below
> +	 * TDH_MEM_PAGE_ADD code path is only used when VM is being
> created
> +	 * before it is running, using KVM_TDX_INIT_MEM_REGION ioctl
> (which
> +	 * always uses vcpu 0's page table and protected by vcpu->mutex).
> +	 */
> +	if (KVM_BUG_ON(kvm_tdx->source_pa =3D=3D INVALID_PAGE, kvm)) {
> +		tdx_unpin(kvm, pfn);
> +		return -EINVAL;
> +	}
> +
> +	source_pa =3D kvm_tdx->source_pa &
> ~KVM_TDX_MEASURE_MEMORY_REGION;
> +	measure =3D kvm_tdx->source_pa &
> KVM_TDX_MEASURE_MEMORY_REGION;
> +	kvm_tdx->source_pa =3D INVALID_PAGE;
> +
> +	do {
> +		err =3D tdh_mem_page_add(kvm_tdx->tdr_pa, gpa, hpa,
> source_pa,
> +				       &out);
> +		/*
> +		 * This path is executed during populating initial guest memory
> +		 * image. i.e. before running any vcpu.  Race is rare.
> +		 */
> +	} while (unlikely(err =3D=3D TDX_ERROR_SEPT_BUSY));
> +	if (KVM_BUG_ON(err, kvm)) {
> +		pr_tdx_error(TDH_MEM_PAGE_ADD, err, &out);
> +		tdx_unpin(kvm, pfn);
> +		return -EIO;
> +	} else if (measure)
> +		tdx_measure_page(kvm_tdx, gpa);
> +
> +	return 0;
> +
> +}
> +
>  static int tdx_sept_set_private_spte(struct kvm *kvm, gfn_t gfn,
>  				     enum pg_level level, kvm_pfn_t pfn)  { @@
> -555,9 +625,7 @@ static int tdx_sept_set_private_spte(struct kvm *kvm, gf=
n_t
> gfn,
>  	if (likely(is_td_finalized(kvm_tdx)))
>  		return tdx_sept_page_aug(kvm, gfn, level, pfn);
>=20
> -	/* TODO: tdh_mem_page_add() comes here for the initial memory. */
> -
> -	return 0;
> +	return tdx_sept_page_add(kvm, gfn, level, pfn);
>  }
>=20
>  static int tdx_sept_drop_private_spte(struct kvm *kvm, gfn_t gfn, @@ -12=
65,6
> +1333,96 @@ void tdx_flush_tlb_current(struct kvm_vcpu *vcpu)
>  	tdx_track(vcpu->kvm);
>  }
>=20
> +#define TDX_SEPT_PFERR	(PFERR_WRITE_MASK |
> PFERR_GUEST_ENC_MASK)
> +
> +static int tdx_init_mem_region(struct kvm *kvm, struct kvm_tdx_cmd
> +*cmd) {
> +	struct kvm_tdx *kvm_tdx =3D to_kvm_tdx(kvm);
> +	struct kvm_tdx_init_mem_region region;
> +	struct kvm_vcpu *vcpu;
> +	struct page *page;
> +	int idx, ret =3D 0;
> +	bool added =3D false;
> +
> +	/* Once TD is finalized, the initial guest memory is fixed. */
> +	if (is_td_finalized(kvm_tdx))
> +		return -EINVAL;
> +
> +	/* The BSP vCPU must be created before initializing memory regions.
> */
> +	if (!atomic_read(&kvm->online_vcpus))
> +		return -EINVAL;
> +
> +	if (cmd->flags & ~KVM_TDX_MEASURE_MEMORY_REGION)
> +		return -EINVAL;
> +
> +	if (copy_from_user(&region, (void __user *)cmd->data, sizeof(region)))
> +		return -EFAULT;
> +
> +	/* Sanity check */
> +	if (!IS_ALIGNED(region.source_addr, PAGE_SIZE) ||
> +	    !IS_ALIGNED(region.gpa, PAGE_SIZE) ||
> +	    !region.nr_pages ||
> +	    region.nr_pages & GENMASK_ULL(63, 63 - PAGE_SHIFT) ||
> +	    region.gpa + (region.nr_pages << PAGE_SHIFT) <=3D region.gpa ||
> +	    !kvm_is_private_gpa(kvm, region.gpa) ||
> +	    !kvm_is_private_gpa(kvm, region.gpa + (region.nr_pages <<
> PAGE_SHIFT)))
> +		return -EINVAL;
> +
> +	vcpu =3D kvm_get_vcpu(kvm, 0);
> +	if (mutex_lock_killable(&vcpu->mutex))
> +		return -EINTR;
> +
> +	vcpu_load(vcpu);
> +	idx =3D srcu_read_lock(&kvm->srcu);
> +
> +	kvm_mmu_reload(vcpu);
> +
> +	while (region.nr_pages) {
> +		if (signal_pending(current)) {
> +			ret =3D -ERESTARTSYS;
> +			break;
> +		}
> +
> +		if (need_resched())
> +			cond_resched();
> +
> +		/* Pin the source page. */
> +		ret =3D get_user_pages_fast(region.source_addr, 1, 0, &page);
> +		if (ret < 0)
> +			break;
> +		if (ret !=3D 1) {
> +			ret =3D -ENOMEM;
> +			break;
> +		}
> +
> +		kvm_tdx->source_pa =3D pfn_to_hpa(page_to_pfn(page)) |
> +				     (cmd->flags &
> KVM_TDX_MEASURE_MEMORY_REGION);
> +

Is it fundamentally correct to take a userspace mapped page to add as a TD =
private page?
Maybe take the corresponding page from gmem and do a copy to it?
For example:
ret =3D get_user_pages_fast(region.source_addr, 1, 0, &user_page);
...
kvm_gmem_get_pfn(kvm, gfn_to_memslot(kvm, gfn), gfn, &gmem_pfn, NULL);
memcpy(__va(gmem_pfn << PAGE_SHIFT), page_to_virt(user_page), PAGE_SIZE);
kvm_tdx->source_pa =3D pfn_to_hpa(gmem_pfn) |
                                     (cmd->flags & KVM_TDX_MEASURE_MEMORY_R=
EGION);