Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 20 Apr 2022 09:39:47 +0200
From:   Vasant Karasulli <vkarasulli@suse.de>
To:     Sean Christopherson <seanjc@google.com>
Cc:     linux-kernel@vger.kernel.org, jroedel@suse.de, kvm@vger.kernel.org,
        bp@alien8.de, x86@kernel.org, thomas.lendacky@amd.com,
        varad.gautam@suse.com
Subject: Re: [PATCH v6 2/4] x86/tests: Add tests for AMD SEV-ES #VC handling
 Add KUnit based tests to validate Linux's VC handling for instructions cpuid
 and wbinvd. These tests: 1. install a kretprobe on the #VC handler
 (sev_es_ghcb_hv_call, to access GHCB before/after the resulting VMGEXIT). 2.
 trigger an NAE by executing either cpuid or wbinvd. 3. check that the
 kretprobe was hit with the right exit_code available in GHCB.
Message-ID: <Yl+4w1OK9E7+qRqP@vasant-suse>
References: <20220318094532.7023-1-vkarasulli@suse.de>
 <20220318094532.7023-3-vkarasulli@suse.de>
 <Ykzrb1uyPZ2AKWos@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Ykzrb1uyPZ2AKWos@google.com>
Precedence: bulk

On Mi 06-04-22 01:22:55, Sean Christopherson wrote:
> The shortlog and changelog are all messed up.  Ditto for the other patches in this
> series.

I am really sorry about that. I had sent another mail with the same patch version
with subject line corrected.
https://lore.kernel.org/kvm/20220318104646.8313-1-vkarasulli@suse.de/T/#t
>
> On Fri, Mar 18, 2022, Vasant Karasulli wrote:
> > Signed-off-by: Vasant Karasulli <vkarasulli@suse.de>
> > ---
> >  arch/x86/tests/Makefile      |   2 +
> >  arch/x86/tests/sev-test-vc.c | 114 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 116 insertions(+)
> >  create mode 100644 arch/x86/tests/sev-test-vc.c
>
> ...
>
> > +int sev_es_test_vc_init(struct kunit *test)
> > +{
> > +	int ret;
> > +
> > +	if (!cc_platform_has(CC_ATTR_GUEST_STATE_ENCRYPT)) {
> > +		kunit_info(test, "Not a SEV-ES guest. Skipping.");
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	memset(&hv_call_krp, 0, sizeof(hv_call_krp));
> > +	hv_call_krp.entry_handler = hv_call_krp_entry;
> > +	hv_call_krp.handler = hv_call_krp_ret;
> > +	hv_call_krp.maxactive = 100;
> > +	hv_call_krp.data_size = sizeof(unsigned long);
> > +	hv_call_krp.kp.symbol_name = "sev_es_ghcb_hv_call";
> > +	hv_call_krp.kp.addr = 0;
> > +
> > +	ret = register_kretprobe(&hv_call_krp);
> > +	if (ret) {
> > +		kunit_info(test, "Could not register kretprobe. Skipping.");
> > +		goto out;
> > +	}
> > +
> > +	test->priv = kunit_kzalloc(test, sizeof(u64), GFP_KERNEL);
>
> Allocating 8 bytes and storing the pointer an 8-byte field is rather pointless :-)

Yes, I will remove this in the next version.
>
> > +	if (!test->priv) {
> > +		ret = -ENOMEM;
> > +		kunit_info(test, "Could not allocate. Skipping.");
> > +		goto out;
> > +	}
> > +
> > +out:
> > +	return ret;
> > +}
> > +
> > +void sev_es_test_vc_exit(struct kunit *test)
> > +{
> > +	if (test->priv)
> > +		kunit_kfree(test, test->priv);
> > +
> > +	if (hv_call_krp.kp.addr)
> > +		unregister_kretprobe(&hv_call_krp);
> > +}
> > +
> > +#define check_op(kt, ec, op)			\
> > +do {						\
> > +	struct kunit *t = (struct kunit *) kt;	\
> > +	op;					\
> > +	KUNIT_EXPECT_EQ(t, (typeof(ec)) ec,	\
> > +		*((typeof(ec) *)(t->priv)));		\
> > +} while (0)
> > +
> > +static void sev_es_nae_cpuid(struct kunit *test)
> > +{
> > +	unsigned int cpuid_fn = 0x8000001f;
> > +
> > +	check_op(test, SVM_EXIT_CPUID, native_cpuid_eax(cpuid_fn));
>
> Are there plans to go beyond basic checks?  Neat idea, but it seems like it will
> be prone to bitrot since it requires a somewhat esoteric setup and an opt-in config.
> And odds are very good that if the kernel can make it this far as an SEV-ES guest,
> it's gotten the basics right.

I will definitely think about adding more checks and performing these checks
early enough in the guest run.

Thanks for your feedback.

Thanks,
Vasant Karasulli
Kernel generalist
www.suse.com<http://www.suse.com>
[https://www.suse.com/assets/img/social-platforms-suse-logo.png]<http://www.suse.com/>
SUSE - Open Source Solutions for Enterprise Servers & Cloud<http://www.suse.com/>
Modernize your infrastructure with SUSE Linux Enterprise servers, cloud technology for IaaS, and SUSE's software-defined storage.
www.suse.com