On Mon, 2024-02-26 at 00:25 -0800, isaku.yamahata@xxxxxxxxx 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;
I think we could drop theselocal variables and just use tdx->tdvpr_pa and tdx->tdvpx_pa. Then we
don't have to have the assignments later.
> + 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;
Or alternatively let's move these to right before they are used. (in the current branch
> + for (i = 0; i < tdx_info->nr_tdvpx_pages; i++) {
> + 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;
> + }
> + /* vcpu_free method frees TDVPX and TDR donated to TDX */
> + return -EIO;
> + }
> + }
>
>
In the current branch tdh_vp_init() takes struct vcpu_tdx, so they would be moved right here.
What do you think?
> +
> + 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;
> +}
