Re: [PATCH 1/2] KVM: SVM: Convert plain error code numbers to defines

["Paluri, PavanKumar" <papaluri@amd.com>]

Hi Sean,

On 12/16/2024 6:41 PM, Sean Christopherson wrote:
> On Fri, Dec 06, 2024, Melody Wang wrote:
>> Convert VMGEXIT SW_EXITINFO1 codes from plain numbers to proper defines.
>>
>> No functionality changed.
>>
>> Signed-off-by: Melody Wang <huibo.wang@amd.com>
>> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
>> Reviewed-by: Pavan Kumar Paluri <papaluri@amd.com>
>> ---
>>  arch/x86/include/asm/sev-common.h |  8 ++++++++
>>  arch/x86/kvm/svm/sev.c            | 12 ++++++------
>>  arch/x86/kvm/svm/svm.c            |  2 +-
>>  3 files changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
>> index 98726c2b04f8..01d4744e880a 100644
>> --- a/arch/x86/include/asm/sev-common.h
>> +++ b/arch/x86/include/asm/sev-common.h
>> @@ -209,6 +209,14 @@ struct snp_psc_desc {
>>  
>>  #define GHCB_RESP_CODE(v)		((v) & GHCB_MSR_INFO_MASK)
>>  
>> +/*
>> + * Error codes of the GHCB SW_EXITINFO1 related to GHCB input that can be
>> + * communicated back to the guest
>> + */
>> +#define GHCB_HV_RESP_SUCCESS		0
> 
> Somewhat of a nit, but I don't think "SUCCESS" is appropriate due to the bizarre
> return codes for Page State Change (PSC) requests.  For unknown reasons (really,
> why!?!?), PSC requests apparently always get back '0', but then put a bunch of
> errors into SW_EXITINFO2, including cases that are clearly not "success".
> 
> FWIW, "no action" isn't much better, because it too directly conflicts with
> the documentation for PSC:
> 
>   The page state change request was interrupted, retry the request.
>                                                  ^^^^^^^^^^^^^^^^^
> I'm all for having svm_vmgexit_success(), but I think the macro needs to be
> NO_ACTION (even though it too is flawed), because I strongly suspect that patch 2
> deliberately avoided SUCCESS in snp_handle_guest_req() and snp_complete_psc()
> specifically because you knew SUCCESS would be misleading.
> 
>> +#define GHCB_HV_RESP_ISSUE_EXCEPTION	1
>> +#define GHCB_HV_RESP_MALFORMED_INPUT	2
> 
> Where is '2' actually documented?  I looked all over the GHCB and the only ones
> I can find are '0' and '1'.
> 
>   0x0000
>     o No action requested by the hypervisor.
>   0x0001
>     o The hypervisor has requested an exception be issued
> 

GHCB spec (Pub# 56421, Rev:2.03), section 4.1 Invoking VMGEXIT documents
0x0002 as well.

0x0002
 o The hypervisor encountered malformed input for the VMGEXIT.

Thanks,
Pavan

> And again, somewhat of a nit, but PSC ruins all the fun once more, because it
> quite clearly has multiple "malformed input" responses.  So if PSC can get rejected
> with "bad input", why on earth would it not use GHCB_HV_RESP_MALFORMED_INPUT?
> 
>   o SW_EXITINFO2[31:0] == 0x00000001
>     The page_state_change_header structure is not valid
> 
>   o SW_EXITINFO2[31:0] == 0x00000002
>     The page_state_change_entry structure, identified by
>     page_state_change_header.cur_entry, is not valid.