Received-SPF: pass (google.com: domain of linux-kernel+bounces-110653-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 21 Mar 2024 13:43:45 -0700
From: Isaku Yamahata <isaku.yamahata@intel.com>
To: Chao Gao <chao.gao@intel.com>
Cc: isaku.yamahata@intel.com, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org, isaku.yamahata@gmail.com,
	Paolo Bonzini <pbonzini@redhat.com>, erdemaktas@google.com,
	Sean Christopherson <seanjc@google.com>,
	Sagi Shahar <sagis@google.com>, Kai Huang <kai.huang@intel.com>,
	chen.bo@intel.com, hang.yuan@intel.com, tina.zhang@intel.com,
	Sean Christopherson <sean.j.christopherson@intel.com>,
	isaku.yamahata@linux.intel.com
Subject: Re: [PATCH v19 044/130] KVM: TDX: Do TDX specific vcpu initialization
Message-ID: <20240321204345.GQ1994522@ls.amr.corp.intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <d6a21fe6ea9eb53c24b6527ef8e5a07f0c2e8806.1708933498.git.isaku.yamahata@intel.com>
 <ZfvI8t7SlfIsxbmT@chao-email>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <ZfvI8t7SlfIsxbmT@chao-email>

On Thu, Mar 21, 2024 at 01:43:14PM +0800,
Chao Gao <chao.gao@intel.com> wrote:

> >+/* VMM can pass one 64bit auxiliary data to vcpu via RCX for guest BIOS. */
> >+static int tdx_td_vcpu_init(struct kvm_vcpu *vcpu, u64 vcpu_rcx)
> >+{
> >+	struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
> >+	struct vcpu_tdx *tdx = to_tdx(vcpu);
> >+	unsigned long *tdvpx_pa = NULL;
> >+	unsigned long tdvpr_pa;
> >+	unsigned long va;
> >+	int ret, i;
> >+	u64 err;
> >+
> >+	if (is_td_vcpu_created(tdx))
> >+		return -EINVAL;
> >+
> >+	/*
> >+	 * vcpu_free method frees allocated pages.  Avoid partial setup so
> >+	 * that the method can't handle it.
> >+	 */
> >+	va = __get_free_page(GFP_KERNEL_ACCOUNT);
> >+	if (!va)
> >+		return -ENOMEM;
> >+	tdvpr_pa = __pa(va);
> >+
> >+	tdvpx_pa = kcalloc(tdx_info->nr_tdvpx_pages, sizeof(*tdx->tdvpx_pa),
> >+			   GFP_KERNEL_ACCOUNT);
> >+	if (!tdvpx_pa) {
> >+		ret = -ENOMEM;
> >+		goto free_tdvpr;
> >+	}
> >+	for (i = 0; i < tdx_info->nr_tdvpx_pages; i++) {
> >+		va = __get_free_page(GFP_KERNEL_ACCOUNT);
> >+		if (!va) {
> >+			ret = -ENOMEM;
> >+			goto free_tdvpx;
> >+		}
> >+		tdvpx_pa[i] = __pa(va);
> >+	}
> >+
> >+	err = tdh_vp_create(kvm_tdx->tdr_pa, tdvpr_pa);
> >+	if (KVM_BUG_ON(err, vcpu->kvm)) {
> >+		ret = -EIO;
> >+		pr_tdx_error(TDH_VP_CREATE, err, NULL);
> >+		goto free_tdvpx;
> >+	}
> >+	tdx->tdvpr_pa = tdvpr_pa;
> >+
> >+	tdx->tdvpx_pa = tdvpx_pa;
> >+	for (i = 0; i < tdx_info->nr_tdvpx_pages; i++) {
> 
> Can you merge the for-loop above into this one? then ...
> 
> >+		err = tdh_vp_addcx(tdx->tdvpr_pa, tdvpx_pa[i]);
> >+		if (KVM_BUG_ON(err, vcpu->kvm)) {
> >+			pr_tdx_error(TDH_VP_ADDCX, err, NULL);
> 
> >+			for (; i < tdx_info->nr_tdvpx_pages; i++) {
> >+				free_page((unsigned long)__va(tdvpx_pa[i]));
> >+				tdvpx_pa[i] = 0;
> >+			}
> 
> ... no need to free remaining pages.

Makes sense. Let me clean up this.


> >+			/* vcpu_free method frees TDVPX and TDR donated to TDX */
> >+			return -EIO;
> >+		}
> >+	}
> >+
> >+	err = tdh_vp_init(tdx->tdvpr_pa, vcpu_rcx);
> >+	if (KVM_BUG_ON(err, vcpu->kvm)) {
> >+		pr_tdx_error(TDH_VP_INIT, err, NULL);
> >+		return -EIO;
> >+	}
> >+
> >+	vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
> >+	tdx->td_vcpu_created = true;
> >+	return 0;
> >+
> >+free_tdvpx:
> >+	for (i = 0; i < tdx_info->nr_tdvpx_pages; i++) {
> >+		if (tdvpx_pa[i])
> >+			free_page((unsigned long)__va(tdvpx_pa[i]));
> >+		tdvpx_pa[i] = 0;
> >+	}
> >+	kfree(tdvpx_pa);
> >+	tdx->tdvpx_pa = NULL;
> >+free_tdvpr:
> >+	if (tdvpr_pa)
> >+		free_page((unsigned long)__va(tdvpr_pa));
> >+	tdx->tdvpr_pa = 0;
> >+
> >+	return ret;
> >+}
> >+
> >+int tdx_vcpu_ioctl(struct kvm_vcpu *vcpu, void __user *argp)
> >+{
> >+	struct msr_data apic_base_msr;
> >+	struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
> >+	struct vcpu_tdx *tdx = to_tdx(vcpu);
> >+	struct kvm_tdx_cmd cmd;
> >+	int ret;
> >+
> >+	if (tdx->initialized)
> >+		return -EINVAL;
> >+
> >+	if (!is_hkid_assigned(kvm_tdx) || is_td_finalized(kvm_tdx))
> 
> These checks look random e.g., I am not sure why is_td_created() isn't check here.
> 
> A few helper functions and boolean variables are added to track which stage the
> TD or TD vCPU is in. e.g.,
> 
> is_hkid_assigned()
> is_td_finalized()
> is_td_created()
> tdx->initialized
> td_vcpu_created
> 
> Insteading of doing this, I am wondering if adding two state machines for
> TD and TD vCPU would make the implementation clear and easy to extend.

Let me look into the state machine. Originally I hoped we don't need it, but
it seems to deserve the state machine..


> >+		return -EINVAL;
> >+
> >+	if (copy_from_user(&cmd, argp, sizeof(cmd)))
> >+		return -EFAULT;
> >+
> >+	if (cmd.error)
> >+		return -EINVAL;
> >+
> >+	/* Currently only KVM_TDX_INTI_VCPU is defined for vcpu operation. */
> >+	if (cmd.flags || cmd.id != KVM_TDX_INIT_VCPU)
> >+		return -EINVAL;
> 
> Even though KVM_TD_INIT_VCPU is the only supported command, it is worthwhile to
> use a switch-case statement. New commands can be added easily without the need
> to refactor this function first.

Yes. For KVM_MAP_MEMORY, I will make KVM_TDX_INIT_MEM_REGION vcpu ioctl instead
of vm ioctl because it is consistent and scalable.  We'll have switch statement
in the next respin.

> >+
> >+	/*
> >+	 * As TDX requires X2APIC, set local apic mode to X2APIC.  User space
> >+	 * VMM, e.g. qemu, is required to set CPUID[0x1].ecx.X2APIC=1 by
> >+	 * KVM_SET_CPUID2.  Otherwise kvm_set_apic_base() will fail.
> >+	 */
> >+	apic_base_msr = (struct msr_data) {
> >+		.host_initiated = true,
> >+		.data = APIC_DEFAULT_PHYS_BASE | LAPIC_MODE_X2APIC |
> >+		(kvm_vcpu_is_reset_bsp(vcpu) ? MSR_IA32_APICBASE_BSP : 0),
> >+	};
> >+	if (kvm_set_apic_base(vcpu, &apic_base_msr))
> >+		return -EINVAL;
> 
> Exporting kvm_vcpu_is_reset_bsp() and kvm_set_apic_base() should be done
> here (rather than in a previous patch).

Sure.


> >+
> >+	ret = tdx_td_vcpu_init(vcpu, (u64)cmd.data);
> >+	if (ret)
> >+		return ret;
> >+
> >+	tdx->initialized = true;
> >+	return 0;
> >+}
> >+
> 
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index c002761bb662..2bd4b7c8fa51 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -6274,6 +6274,12 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> > 	case KVM_SET_DEVICE_ATTR:
> > 		r = kvm_vcpu_ioctl_device_attr(vcpu, ioctl, argp);
> > 		break;
> >+	case KVM_MEMORY_ENCRYPT_OP:
> >+		r = -ENOTTY;
> 
> Maybe -EINVAL is better. Because previously trying to call this on vCPU fd
> failed with -EINVAL given ...

Oh, ok. Will change it.  I followed VM ioctl case as default value. But vcpu
ioctl seems to have -EINVAL as default value.
-- 
Isaku Yamahata <isaku.yamahata@intel.com>