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;
MIME-Version: 1.0
References: <20230326144701.3039598-1-nikunj@amd.com> <20230326144701.3039598-9-nikunj@amd.com>
In-Reply-To: <20230326144701.3039598-9-nikunj@amd.com>
From:   Peter Gonda <pgonda@google.com>
Date:   Mon, 10 Apr 2023 11:14:47 -0600
Message-ID: <CAMkAt6rqsg6=Sx6Fqnf7KNOUB9YPMU6TUriZYZXbXQTvcoKzNw@mail.gmail.com>
Subject: Re: [PATCH v2 08/11] x86/sev: Add Secure TSC support for SNP guests
To:     Nikunj A Dadhania <nikunj@amd.com>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org, bp@alien8.de,
        thomas.lendacky@amd.com, dionnaglaze@google.com, seanjc@google.com,
        pbonzini@redhat.com, michael.roth@amd.com, ketanch@iitk.ac.in
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> +
>  /* #VC handler runtime per-CPU data */
>  struct sev_es_runtime_data {
>         struct ghcb ghcb_page;
> @@ -1107,7 +1111,7 @@ static void *alloc_shared_pages(size_t sz)
>         return page_address(page);
>  }
>
> -static int snp_setup_psp_messaging(struct sev_guest_platform_data *pdata)
> +static int __init snp_setup_psp_messaging(struct sev_guest_platform_data *pdata)
>  {
>         u64 gpa;
>         int ret;
> @@ -1406,6 +1410,80 @@ bool snp_assign_vmpck(struct snp_guest_dev *dev, int vmpck_id)
>  }
>  EXPORT_SYMBOL_GPL(snp_assign_vmpck);
>
> +static int __init snp_get_tsc_info(void)
> +{
> +       u8 buf[SNP_TSC_INFO_REQ_SZ + AUTHTAG_LEN];
> +       struct snp_tsc_info_resp tsc_resp = {0};
> +       struct snp_tsc_info_req tsc_req;
> +       struct snp_guest_req req;
> +       struct snp_guest_dev dev;
> +       int rc, resp_len;
> +
> +       /*
> +        * The intermediate response buffer is used while decrypting the
> +        * response payload. Make sure that it has enough space to cover the
> +        * authtag.
> +        */
> +       resp_len = sizeof(tsc_resp) + AUTHTAG_LEN;
> +       if (sizeof(buf) < resp_len)
> +               return -EINVAL;
> +
> +       /* Zero the tsc_info_req */
> +       memzero_explicit(&tsc_req, sizeof(tsc_req));
> +       memzero_explicit(&req, sizeof(req));

Whats the guidance on when we should use memzero_explicit() vs just
something like: `snp_tsc_info_resp tsc_resp = {0};`?

> +
> +       dev.pdata = platform_data;
> +       if (!snp_assign_vmpck(&dev, 0))
> +               return -EINVAL;
> +
> +       req.msg_version = MSG_HDR_VER;
> +       req.msg_type = SNP_MSG_TSC_INFO_REQ;
> +       req.req_buf = &tsc_req;
> +       req.req_sz = sizeof(tsc_req);
> +       req.resp_buf = buf;
> +       req.resp_sz = resp_len;
> +       req.fw_err = NULL;

Why do we not want the FW error code?

> +       req.exit_code = SVM_VMGEXIT_GUEST_REQUEST;
> +       rc = snp_send_guest_request(&dev, &req);
> +       if (rc)
> +               goto err_req;
> +
> +       memcpy(&tsc_resp, buf, sizeof(tsc_resp));
> +       pr_debug("%s: Valid response status %x scale %llx offset %llx factor %llx\n",
> +                __func__, tsc_resp.status, tsc_resp.tsc_scale, tsc_resp.tsc_offset,
> +                tsc_resp.tsc_factor);
> +
> +       guest_tsc_scale = tsc_resp.tsc_scale;
> +       guest_tsc_offset = tsc_resp.tsc_offset;
> +
> +err_req:
> +       /* The response buffer contains the sensitive data, explicitly clear it. */
> +       memzero_explicit(buf, sizeof(buf));
> +       memzero_explicit(&tsc_resp, sizeof(tsc_resp));
> +       memzero_explicit(&req, sizeof(req));
> +
> +       return rc;
> +}
> +