lkml.org 
[lkml]   [2024]   [Mar]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH v19 038/130] KVM: TDX: create/destroy VM structure
    From
    .. to continue the previous review ...

    >
    > +static int __tdx_td_init(struct kvm *kvm)
    > +{
    > + struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
    > + cpumask_var_t packages;
    > + unsigned long *tdcs_pa = NULL;
    > + unsigned long tdr_pa = 0;
    > + unsigned long va;
    > + int ret, i;
    > + u64 err;
    > +
    > + ret = tdx_guest_keyid_alloc();
    > + if (ret < 0)
    > + return ret;
    > + kvm_tdx->hkid = ret;
    > +
    > + va = __get_free_page(GFP_KERNEL_ACCOUNT);
    > + if (!va)
    > + goto free_hkid;
    > + tdr_pa = __pa(va);
    > +
    > + tdcs_pa = kcalloc(tdx_info->nr_tdcs_pages, sizeof(*kvm_tdx->tdcs_pa),
    > + GFP_KERNEL_ACCOUNT | __GFP_ZERO);
    > + if (!tdcs_pa)
    > + goto free_tdr;

    Empty line.

    > + for (i = 0; i < tdx_info->nr_tdcs_pages; i++) {
    > + va = __get_free_page(GFP_KERNEL_ACCOUNT);
    > + if (!va)
    > + goto free_tdcs;
    > + tdcs_pa[i] = __pa(va);
    > + }
    > +
    > + if (!zalloc_cpumask_var(&packages, GFP_KERNEL)) {
    > + ret = -ENOMEM;
    > + goto free_tdcs;
    > + }

    Empty line.

    > + cpus_read_lock();
    > + /*
    > + * Need at least one CPU of the package to be online in order to
    > + * program all packages for host key id. Check it.
    > + */
    > + for_each_present_cpu(i)
    > + cpumask_set_cpu(topology_physical_package_id(i), packages);
    > + for_each_online_cpu(i)
    > + cpumask_clear_cpu(topology_physical_package_id(i), packages);
    > + if (!cpumask_empty(packages)) {
    > + ret = -EIO;
    > + /*
    > + * Because it's hard for human operator to figure out the
    > + * reason, warn it.
    > + */
    > +#define MSG_ALLPKG "All packages need to have online CPU to create TD. Online CPU and retry.\n"
    > + pr_warn_ratelimited(MSG_ALLPKG);
    > + goto free_packages;
    > + }
    > +
    > + /*
    > + * Acquire global lock to avoid TDX_OPERAND_BUSY:
    > + * TDH.MNG.CREATE and other APIs try to lock the global Key Owner
    > + * Table (KOT) to track the assigned TDX private HKID. It doesn't spin
    > + * to acquire the lock, returns TDX_OPERAND_BUSY instead, and let the
    > + * caller to handle the contention. This is because of time limitation
    > + * usable inside the TDX module and OS/VMM knows better about process
    > + * scheduling.
    > + *
    > + * APIs to acquire the lock of KOT:
    > + * TDH.MNG.CREATE, TDH.MNG.KEY.FREEID, TDH.MNG.VPFLUSHDONE, and
    > + * TDH.PHYMEM.CACHE.WB. > + */

    Don't need to mention all SEAMCALLs here, but put a comment where
    appliciable, i.e., where they are used.

    /*
    * TDH.MNG.CREATE tries to grab the global TDX module and fails
    * with TDX_OPERAND_BUSY when it fails to grab. Take the global
    * lock to prevent it from failure.
    */
    > + mutex_lock(&tdx_lock);
    > + err = tdh_mng_create(tdr_pa, kvm_tdx->hkid);
    > + mutex_unlock(&tdx_lock);

    Empty line.

    > + if (err == TDX_RND_NO_ENTROPY) {
    > + ret = -EAGAIN;
    > + goto free_packages;
    > + }
    > + if (WARN_ON_ONCE(err)) {
    > + pr_tdx_error(TDH_MNG_CREATE, err, NULL);
    > + ret = -EIO;
    > + goto free_packages;
    > + }

    I would prefer more empty lines.

    > + 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.
    */
    > + 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()?

    > + return 0;
    > +
    > + /*
    > + * The sequence for freeing resources from a partially initialized TD
    > + * varies based on where in the initialization flow failure occurred.
    > + * Simply use the full teardown and destroy, which naturally play nice
    > + * with partial initialization.
    > + */
    > +teardown:
    > + for (; i < tdx_info->nr_tdcs_pages; i++) {
    > + if (tdcs_pa[i]) {
    > + free_page((unsigned long)__va(tdcs_pa[i]));
    > + tdcs_pa[i] = 0;
    > + }
    > + }
    > + if (!kvm_tdx->tdcs_pa)
    > + kfree(tdcs_pa);

    The code to "free TDCS pages in a loop and free the array" is done below
    with duplicated code. I am wondering whether we have way to eliminate one.

    But I have lost track here, so perhaps we can review again after we
    split the patch to smaller pieces.

    > + tdx_mmu_release_hkid(kvm);
    > + tdx_vm_free(kvm);
    > + return ret;
    > +
    > +free_packages:
    > + cpus_read_unlock();
    > + free_cpumask_var(packages);
    > +free_tdcs:
    > + for (i = 0; i < tdx_info->nr_tdcs_pages; i++) {
    > + if (tdcs_pa[i])
    > + free_page((unsigned long)__va(tdcs_pa[i]));
    > + }
    > + kfree(tdcs_pa);
    > + kvm_tdx->tdcs_pa = NULL;
    > +
    > +free_tdr:
    > + if (tdr_pa)
    > + free_page((unsigned long)__va(tdr_pa));
    > + kvm_tdx->tdr_pa = 0;
    > +free_hkid:
    > + if (is_hkid_assigned(kvm_tdx))
    > + tdx_hkid_free(kvm_tdx);
    > + return ret;
    > +}
    > +
    > int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
    > {
    > struct kvm_tdx_cmd tdx_cmd;
    > @@ -215,12 +664,13 @@ static int tdx_md_read(struct tdx_md_map *maps, int nr_maps)
    >
    > static int __init tdx_module_setup(void)
    > {
    > - u16 num_cpuid_config;
    > + u16 num_cpuid_config, tdcs_base_size;
    > int ret;
    > u32 i;
    >
    > struct tdx_md_map mds[] = {
    > TDX_MD_MAP(NUM_CPUID_CONFIG, &num_cpuid_config),
    > + TDX_MD_MAP(TDCS_BASE_SIZE, &tdcs_base_size),
    > };
    >
    > struct tdx_metadata_field_mapping fields[] = {
    > @@ -273,6 +723,8 @@ static int __init tdx_module_setup(void)
    > c->edx = ecx_edx >> 32;
    > }
    >
    > + tdx_info->nr_tdcs_pages = tdcs_base_size / PAGE_SIZE;
    > +

    Round up the 'tdcs_base_size' to make sure you have enough room, or put
    a WARN() here if not page aligned?

    > return 0;
    >
    > error_out:
    > @@ -319,13 +771,27 @@ int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops)
    > struct tdx_enabled enable = {
    > .err = ATOMIC_INIT(0),
    > };
    > + int max_pkgs;
    > int r = 0;
    > + int i;

    Nit: you can put the 3 into one line.

    >
    > + if (!cpu_feature_enabled(X86_FEATURE_MOVDIR64B)) {
    > + pr_warn("MOVDIR64B is reqiured for TDX\n");

    It's better to make it more clear:

    "Disable TDX: MOVDIR64B is not supported or disabled by the kernel."

    Or, to match below:

    "Cannot enable TDX w/o MOVDIR64B".

    > + return -EOPNOTSUPP;
    > + }
    > if (!enable_ept) {
    > pr_warn("Cannot enable TDX with EPT disabled\n");
    > return -EINVAL;
    > }
    >
    > + max_pkgs = topology_max_packages();
    > + tdx_mng_key_config_lock = kcalloc(max_pkgs, sizeof(*tdx_mng_key_config_lock),
    > + GFP_KERNEL);
    > + if (!tdx_mng_key_config_lock)
    > + return -ENOMEM;
    > + for (i = 0; i < max_pkgs; i++)
    > + mutex_init(&tdx_mng_key_config_lock[i]);
    > +

    Using a per-socket lock looks a little bit overkill to me. I don't know
    whether we need to do in the initial version. Will leave to others.

    Please at least add a comment to explain this is for better performance
    when creating multiple TDX guests IIUC?

    > if (!zalloc_cpumask_var(&enable.enabled, GFP_KERNEL)) {
    > r = -ENOMEM;
    > goto out;
    > @@ -350,4 +816,5 @@ int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops)
    > void tdx_hardware_unsetup(void)
    > {
    > kfree(tdx_info);
    > + kfree(tdx_mng_key_config_lock);

    The kernel actually has a mutex_destroy(). It is empty when
    CONFIG_DEBUG_LOCK_ALLOC is off, but I think it should be standard
    proceedure to also mutex_destory()?

    [...]

    \
     
     \ /
      Last update: 2024-05-27 16:13    [W:4.121 / U:0.260 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site