linux-kernel - Re: [PATCH v5 2/2] KVM: SVM: Provide helpers to set the error code

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: <Z75FFZqPLyJgt-4g@google.com>
Date: Tue, 25 Feb 2025 14:32:53 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Melody Wang <huibo.wang@....com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, KVM <kvm@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, Tom Lendacky <thomas.lendacky@....com>, 
	Paluri PavanKumar <pavankumar.paluri@....com>
Subject: Re: [PATCH v5 2/2] KVM: SVM: Provide helpers to set the error code

On Tue, Feb 25, 2025, Melody Wang wrote:
> @@ -3675,8 +3673,13 @@ static void snp_complete_psc(struct vcpu_svm *svm, u64 psc_ret)
>  	svm->sev_es.psc_inflight = 0;
>  	svm->sev_es.psc_idx = 0;
>  	svm->sev_es.psc_2m = false;
> -	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, psc_ret);
> -}
> +
> +	/*
> +	 * A value of zero in SW_EXITINFO1 does not guarantee that all operations have

"A value of zero" is largely redundant, and somewhat confusing.  There's no '0'
in the below code, so to understand the comment, the reader needs to know that
"no action" is a response code of '0' (and is communicated in SW_EXITINFO1,
though that's much less of a problem).

> +	 * completed or completed successfully.  PSC requests always get a "no action"
> +	 * response in SW_EXITINFO1, with a PSC-specific return code in SW_EXITINFO2.

Please wrap at ~80.  Yes, it's a "soft limit", but preferred KVM style is to
wrap at 80 unless running long makes the code more readable.  For a multi-line
comment, I don't see any reason to wrap in the mid-80s.

This is what I ended up with

	/*
	 * PSC requests always get a "no action" response in SW_EXITINFO1, with
	 * a PSC-specific return code in SW_EXITINFO2 that provides the "real"
	 * return code.  E.g. if the PSC request was interrupted, the need to
	 * retry is communicated via SW_EXITINFO2, not SW_EXITINFO1.
	 */

> +	 */
> +	svm_vmgexit_no_action(svm, psc_ret); }

Malformed change to the closing curly brace.

>  	case SVM_VMGEXIT_HV_FEATURES:
> -		ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, GHCB_HV_FT_SUPPORTED);
> +		/* Get hypervisor supported features */

This doesn't add any value.  If the logic isn't clear, then GHCB_HV_FT_SUPPORTED
needs to be renamed.

> +		svm_vmgexit_success(svm, GHCB_HV_FT_SUPPORTED);
>  
>  		ret = 1;
>  		break;