linux-kernel - Re: [PATCH v2] KVM: SVM: Convert plain error code numbers to defines

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: <Z0-nO-iyICRy_m5S@google.com>
Date: Tue, 3 Dec 2024 16:50:03 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: "Melody (Huibo) Wang" <huibo.wang@....com>
Cc: LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>, 
	Paolo Bonzini <pbonzini@...hat.com>, Tom Lendacky <thomas.lendacky@....com>, KVM <kvm@...r.kernel.org>, 
	Pavan Kumar Paluri <papaluri@....com>
Subject: Re: [PATCH v2] KVM: SVM: Convert plain error code numbers to defines

On Tue, Dec 03, 2024, Melody (Huibo) Wang wrote:
> Hi Sean,
> 
> On 12/2/2024 4:11 PM, Sean Christopherson wrote:
> 
> > 
> > E.g. something like this?  Definitely feel free to suggest better names.
> > 
> > static inline void svm_vmgexit_set_return_code(struct vcpu_svm *svm,
> > 					       u64 response, u64 data)
> > {
> > 	ghcb_set_sw_exit_info_1(svm->sev_es.ghcb, response);
> > 	ghcb_set_sw_exit_info_2(svm->sev_es.ghcb, data);
> > }
> > 
> If I make this function more generic where the exit info is set for both KVM
> and the guest, then maybe I can write something like this:

I like the idea, but I actually think it's better to keep the guest and host code
separate in the case, because the guest code should actually set a triple, e.g.

static __always_inline void sev_es_vmgexit_set_exit_info(struct ghcb *ghcb,
							 u64 exit_code,
							 u64 exit_info_1,
							 u64 exit_info_2)
{
	ghcb_set_sw_exit_code(ghcb, exit_code);
	ghcb_set_sw_exit_info_1(ghcb, exit_info_1);
	ghcb_set_sw_exit_info_2(ghcb, exit_info_2);
}

I'm not totally opposed to sharing code, but I think it will be counter-productive
in this specific case.  E.g. the guest version needs to be __always_inline so that
it can be used in noinstr code.

> void ghcb_set_exit_info(struct ghcb *ghcb,
>                       u64 info1, u64 info2)
> {
> 	ghcb_set_sw_exit_info_1(ghcb, info1);
> 	ghcb_set_sw_exit_info_2(ghcb, info2);
> 
> }
> This way we can address every possible case that sets the exit info - not only KVM. 
> 
> And I am not sure about the wrappers for each specific case because we will
> have too many, too specific small functions, but if you want them I can add
> them.

I count three.  We have far, far more wrappers VMX's is_exception_n(), and IMO
those wrappers make the code significantly more readable.