linux-kernel - Re: [PATCH v13 1/4] virt/coco/sev-guest: Add throttling awareness

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e0d52194-d478-b33e-851b-d5067f4a7669@amd.com>
Date:   Mon, 30 Jan 2023 10:36:56 -0600
From:   Tom Lendacky <thomas.lendacky@....com>
To:     Borislav Petkov <bp@...en8.de>,
        Dionna Glaze <dionnaglaze@...gle.com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Joerg Roedel <jroedel@...e.de>,
        Peter Gonda <pgonda@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Venu Busireddy <venu.busireddy@...cle.com>,
        Michael Roth <michael.roth@....com>,
        "Kirill A. Shutemov" <kirill@...temov.name>,
        Michael Sterritt <sterritt@...gle.com>
Subject: Re: [PATCH v13 1/4] virt/coco/sev-guest: Add throttling awareness

On 1/30/23 05:13, Borislav Petkov wrote:
> On Tue, Jan 24, 2023 at 09:14:52PM +0000, Dionna Glaze wrote:
>> The host is permitted and encouraged to throttle guest requests to the
>> AMD-SP since it is a shared resource across all VMs. Without
>> throttling-awareness, the host returning an error will immediately lock
>> out access to the VMPCK, which makes the VM less useful as it can't
>> attest itself. Since throttling is expected to be a common occurrence, a
>> cooperative host can return a VMM error code that the request was
>> throttled.
> 
> So where is this concept of guest throttling documented?
> 
> It sounds like this is something hypervisors do and it is all fine and
> great to do that but where does it say: yes, this is what we do and this
> is the usual behavior that's expected from guests and HVs to adhere to
> when accessing this shared resource?
> 
> Tom, is that in the spec somewhere perhaps? Or was this decided upon on
> some call?

Yes, this is part of the GHCB 2.02 specification document that is in the 
process of being published.

Thanks,
Tom

> 
> In any case, I'd like for us to document it somewhere eventually if that
> hasn't happened yet so that all parties are clear on what is supposed to
> happen and what the protocol is.
> 
>> +retry:
>>   	/*
>>   	 * Call firmware to process the request. In this function the encrypted
>>   	 * message enters shared memory with the host. So after this call the
>> @@ -346,6 +347,14 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in
>>   	 */
>>   	rc = snp_issue_guest_request(exit_code, &snp_dev->input, &err);
>>   
>> +	/*
>> +	 * The host may return SNP_GUEST_REQ_ERR_EBUSY if the request has been
>> +	 * throttled. Retry in the driver to avoid returning and reusing the
>> +	 * message sequence number on a different message.
>> +	 */
>> +	if (err == SNP_GUEST_REQ_ERR_BUSY)
>> +		goto retry;
> 
> I don't like even potential endless loops.
> 
> How about you turn this into a loop with a sufficiently large retry
> count which, when depleted, gets this request failed with a -ETIMEDOUT
> or what not?
> 
> You could also stick a cond_resched() in that loop so that it can take a
> breather between the requests and doesn't hammer the hw as much.
> 
> Thx.
>