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;
Date:   Tue, 2 Aug 2022 19:46:21 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     isaku.yamahata@intel.com
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        isaku.yamahata@gmail.com, Paolo Bonzini <pbonzini@redhat.com>,
        Kai Huang <kai.huang@intel.com>, Sagi Shahar <sagis@google.com>
Subject: Re: [PATCH v7 022/102] KVM: TDX: create/destroy VM structure
Message-ID: <Yul/DapNdokpeN36@google.com>
References: <cover.1656366337.git.isaku.yamahata@intel.com>
 <aa3b9b81f257d4d177ab25cb78a222d6297de97f.1656366338.git.isaku.yamahata@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <aa3b9b81f257d4d177ab25cb78a222d6297de97f.1656366338.git.isaku.yamahata@intel.com>
Precedence: bulk

On Mon, Jun 27, 2022, isaku.yamahata@intel.com wrote:
> +int tdx_vm_init(struct kvm *kvm)
> +{
> +	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> +	cpumask_var_t packages;
> +	int ret, i;
> +	u64 err;
> +
> +	/* vCPUs can't be created until after KVM_TDX_INIT_VM. */
> +	kvm->max_vcpus = 0;
> +
> +	kvm_tdx->hkid = tdx_keyid_alloc();
> +	if (kvm_tdx->hkid < 0)
> +		return -EBUSY;

We (Google) have been working through potential flows for intrahost (copyless)
migration, and one of the things that came up is that allocating the HKID during
KVM_CREATE_VM will be problematic as HKID are a relatively scarce resource.  E.g.
if all key IDs are in use, then creating a destination TDX VM will be impossible
even though intrahost migration can create succeed since the "new" would reuse
the source's HKID.

Allocating the various pages is also annoying, e.g. they'd have to be freed, but
not as directly problematic.

SEV (all flavors) has a similar problem with ASIDs.  The solution for SEV was to
not allocate an ASID during KVM_CREATE_VM and instead "activate" SEV during
KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM.

I think we should prepare for a similar future for TDX and move the HKID allocation
and all dependent resource allocation to KVM_TDX_INIT_VM.  AFAICT (and remember),
this should be a fairly simple code movement, but I'd prefer it be done before
merging TDX so that if it's not so simple, e.g. requires another sub-ioctl, then
we don't have to try and tweak KVM's ABI to enable intrahost migration.

> +
> +	ret = tdx_alloc_td_page(&kvm_tdx->tdr);
> +	if (ret)
> +		goto free_hkid;
> +
> +	kvm_tdx->tdcs = kcalloc(tdx_caps.tdcs_nr_pages, sizeof(*kvm_tdx->tdcs),
> +				GFP_KERNEL_ACCOUNT);
> +	if (!kvm_tdx->tdcs)
> +		goto free_tdr;
> +	for (i = 0; i < tdx_caps.tdcs_nr_pages; i++) {
> +		ret = tdx_alloc_td_page(&kvm_tdx->tdcs[i]);
> +		if (ret)
> +			goto free_tdcs;
> +	}