On 4/04/2024 6:24 am, Yamahata, Isaku wrote:
On Thu, Mar 28, 2024 at 12:33:35PM +1300, "Huang, Kai" <kai.huang@xxxxxxxxx> wrote:+ kvm_tdx->tdr_pa = tdr_pa; + + for_each_online_cpu(i) { + int pkg = topology_physical_package_id(i); + + if (cpumask_test_and_set_cpu(pkg, packages)) + continue; + + /* + * Program the memory controller in the package with an + * encryption key associated to a TDX private host key id + * assigned to this TDR. Concurrent operations on same memory + * controller results in TDX_OPERAND_BUSY. Avoid this race by + * mutex. + */IIUC the race can only happen when you are creating multiple TDX guests simulatenously? Please clarify this in the comment. And I even don't think you need all these TDX module details: /* * Concurrent run of TDH.MNG.KEY.CONFIG on the same * package resluts in TDX_OPERAND_BUSY. When creating * multiple TDX guests simultaneously this can run * concurrently. Take the per-package lock to * serialize. */As pointed by Chao, those mutex will be dropped. https://lore.kernel.org/kvm/ZfpwIespKy8qxWWE@chao-email/ Also we would simplify cpu masks to track which package is online/offline, which cpu to use for each package somehow.
Please see my reply there. I might be missing something, though.
+ mutex_lock(&tdx_mng_key_config_lock[pkg]); + ret = smp_call_on_cpu(i, tdx_do_tdh_mng_key_config, + &kvm_tdx->tdr_pa, true); + mutex_unlock(&tdx_mng_key_config_lock[pkg]); + if (ret) + break; + } + cpus_read_unlock(); + free_cpumask_var(packages); + if (ret) { + i = 0; + goto teardown; + } + + kvm_tdx->tdcs_pa = tdcs_pa; + for (i = 0; i < tdx_info->nr_tdcs_pages; i++) { + err = tdh_mng_addcx(kvm_tdx->tdr_pa, tdcs_pa[i]); + if (err == TDX_RND_NO_ENTROPY) { + /* Here it's hard to allow userspace to retry. */ + ret = -EBUSY; + goto teardown; + } + if (WARN_ON_ONCE(err)) { + pr_tdx_error(TDH_MNG_ADDCX, err, NULL); + ret = -EIO; + goto teardown; + } + } + + /* + * Note, TDH_MNG_INIT cannot be invoked here. TDH_MNG_INIT requires a dedicated + * ioctl() to define the configure CPUID values for the TD. + */Then, how about renaming this function to __tdx_td_create()?So do we want to rename also ioctl name for consistency? i.e. KVM_TDX_INIT_VM => KVM_TDX_CREATE_VM.
Hmm.. but this __tdx_td_create() (the __tdx_td_init() in this patch) is called via kvm_x86_ops->vm_init(), but not IOCTL()?
If I read correctly, only TDH.MNG.INIT is called via IOCTL(), in that sense it makes more sense to name the IOCTL() as KVM_TDX_INIT_VM.
I don't have strong opinion those names. Maybe
KVM_TDX_{INIT, CREATE, or CONFIG}_VM?
And we can rename the function name to match it.
