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: <20221027150558.722062-1-pgonda@google.com> <20221027150558.722062-3-pgonda@google.com>
In-Reply-To: <20221027150558.722062-3-pgonda@google.com>
From:   Dionna Amalie Glaze <dionnaglaze@google.com>
Date:   Thu, 27 Oct 2022 10:27:09 -0700
Message-ID: <CAAH4kHYTYtHBBzF-0BiTzmHHiF=mnQhRy+5Mfsj1WN4vyPoCZg@mail.gmail.com>
Subject: Re: [PATCH V3 2/2] virt: sev: Allow for retrying SNP extended requests
To:     Peter Gonda <pgonda@google.com>
Cc:     thomas.lendacky@amd.com, Borislav Petkov <bp@suse.de>,
        Michael Roth <michael.roth@amd.com>,
        Haowen Bai <baihaowen@meizu.com>,
        Yang Yingliang <yangyingliang@huawei.com>,
        Marc Orr <marcorr@google.com>,
        David Rientjes <rientjes@google.com>,
        Ashish Kalra <Ashish.Kalra@amd.com>,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

I think just no to this patch? Reasons below.

>
>         if (ghcb->save.sw_exit_info_2) {
> -               /* Number of expected pages are returned in RBX */
> +               /* For a SNP Extended Request, if the request was placed with
> +                * insufficient data pages. The host will return the number of
> +                * pages required using RBX in the GHCB. We can than retry the
> +                * call as an SNP Request to fulfill the command without getting
> +                * the extended request data.
> +                */
>                 if (exit_code == SVM_VMGEXIT_EXT_GUEST_REQUEST &&
> -                   ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_INVALID_LEN)
> -                       input->data_npages = ghcb_get_rbx(ghcb);
> -
> -               *fw_err = ghcb->save.sw_exit_info_2;
> +                   ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_INVALID_LEN) {
> +                       int npages = ghcb_get_rbx(ghcb);
> +
> +                       ghcb_clear_rax(ghcb);
> +                       ghcb_clear_rbx(ghcb);
> +
> +                       ret = sev_es_ghcb_hv_call(ghcb, &ctxt,
> +                                                 SVM_VMGEXIT_GUEST_REQUEST,
> +                                                 input->req_gpa,
> +                                                 input->resp_gpa);
> +                       if (ret)
> +                               goto e_put;
> +

I'm not keen on reissuing the call in this function. I think
issue_request should do its job of sending a request to the host and
returning the specified data, in this case the number of pages in RBX.
I know it's not particularly fun to interpret exitinfo2 in a couple
places, but it serves a purpose. We don't want this function to grow
to have special cases for all the commands that can be sent to the psp
if they don't involve data passed back through the GHCB. The
get_ghcb/put_ghcb frame is the only thing we really need to respect in
here.

The sev-guest device owns the VMPCKn keys, the message sequence
number, and the responsibility of sending a coherent response back to
user space. When we account for the host changing the certificate page
length during the request and not wanting to return to the guest
without completing the firmware call, the length might grow past the
4KiB max constant we have so far. The driver can make the choice of
issuing the request without extended data like you do here, or to
reallocate its cert_data buffer and ask for the extended data again.
It shouldn't matter to the core functionality of issuing a single
request.

When throttling comes into play and retrying needs to happen more than
once, then we're in another situation where the sev-guest driver also
owns the responsibility of trying not to get throttled too hard. My
patches suggest a 2HZ rate limit to avoid any big penalties of running
requests in a tight loop and looking like a DoS antagonist, but that
doesn't belong in arch/x86/kernel/sev.c due to the variability of
strategies.

> +                       input->data_npages = npages;
> +                       *fw_err = SNP_GUEST_REQ_INVALID_LEN;
> +               } else
> +                       *fw_err = ghcb->save.sw_exit_info_2;

I think in both branches of the conditional, fw_err gets set to
exit_info_2. See v4 of my throttling patch series.

-- 
-Dionna Glaze, PhD (she/her)