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

[Tom Lendacky <thomas.lendacky@amd.com>]

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.
>