Re: [kvm-unit-tests PATCH v3 17/17] x86 AMD SEV-ES: Add test cases

From: Marc Orr <marcorr@google.com>
To: Zixuan Wang <zxwang42@gmail.com>
Cc: Varad Gautam <varad.gautam@suse.com>,
	kvm list <kvm@vger.kernel.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Andrew Jones <drjones@redhat.com>,
	"Hyunwook (Wooky) Baek" <baekhw@google.com>,
	Tom Roeder <tmroeder@google.com>,
	Erdem Aktas <erdemaktas@google.com>,
	David Rientjes <rientjes@google.com>,
	Sean Christopherson <seanjc@google.com>,
	"Singh, Brijesh" <brijesh.singh@amd.com>,
	"Lendacky, Thomas" <Thomas.Lendacky@amd.com>,
	Joerg Roedel <jroedel@suse.de>,
	bp@suse.de
Subject: Re: [kvm-unit-tests PATCH v3 17/17] x86 AMD SEV-ES: Add test cases
Date: Tue, 19 Oct 2021 07:14:47 -0700	[thread overview]
Message-ID: <CAA03e5HL0aiByPGiO5mescTHNM=DT69Kx=ep=cS-De8u+tvaMA@mail.gmail.com> (raw)
In-Reply-To: <CAEDJ5ZQbXK=Gtf_QH2PMNEOBo++7vsa84zZ3G8rzM=TH+JUrQQ@mail.gmail.com>

On Mon, Oct 18, 2021 at 9:38 PM Zixuan Wang <zxwang42@gmail.com> wrote:
>
> On Mon, Oct 18, 2021 at 4:47 AM Varad Gautam <varad.gautam@suse.com> wrote:
> >
> > Hi Zixuan,
> >
> > On 10/4/21 10:49 PM, Zixuan Wang wrote:
> > > From: Zixuan Wang <zixuanwang@google.com>
> > > +static int test_sev_es_msr(void)
> > > +{
> > > +     /*
> > > +      * With SEV-ES, rdmsr/wrmsr trigger #VC exception. If #VC is handled
> > > +      * correctly, rdmsr/wrmsr should work like without SEV-ES and not crash
> > > +      * the guest VM.
> > > +      */
> > > +     u64 val = 0x1234;
> > > +     wrmsr(MSR_TSC_AUX, val);
> > > +     if(val != rdmsr(MSR_TSC_AUX)) {
> > > +             return EXIT_FAILURE;
> >
> > See note below.
> >
> > > +     }
> > > +
> > > +     return EXIT_SUCCESS;
> > > +}
> > > +
> > >  int main(void)
> > >  {
> > >       int rtn;
> > >       rtn = test_sev_activation();
> > >       report(rtn == EXIT_SUCCESS, "SEV activation test.");
> > > +     rtn = test_sev_es_activation();
> > > +     report(rtn == EXIT_SUCCESS, "SEV-ES activation test.");
> > > +     rtn = test_sev_es_msr();
> >
> > There is nothing SEV-ES specific about this function, it only wraps
> > rdmsr/wrmsr, which are supposed to generate #VC exceptions on SEV-ES.
> > Since the same scenario can be covered by running the msr testcase
> > as a SEV-ES guest and observing if it crashes, does testing
> > rdmsr/wrmsr one more time here gain us any new information?
> >
> > Also, the function gets called from main() even if
> > test_sev_es_activation() failed or SEV-ES was inactive.
> >
> > Note: More broadly, what are you looking to test for here?
> > 1. wrmsr/rdmsr correctness (rdmsr reads what wrmsr wrote)? or,
> > 2. A #VC exception not causing a guest crash on SEV-ES?
> >
> > If you are looking to test 1., I suggest letting it be covered by
> > the generic testcases for msr.
> >
> > If you are looking to test 2., perhaps a better test is to trigger
> > all scenarios that would cause a #VC exception (eg. test_sev_es_vc_exit)
> > and check that a SEV-ES guest survives.
> >
> > Regards,
> > Varad
> >
>
> Hi Varad,
>
> This test case does not bring any SEV-related functionality testing.
> Instead, it is provided for development, i.e., one can check if SEV is
> properly set up by monitoring if this test case runs fine without
> crashes.
>
> Since this test case is causing some confusion and does not bring any
> functionality testing, I can remove it from the next version. We can
> still verify the SEV setup process by checking if an existing test
> case (e.g., x86/msr.c) runs without crashes in a SEV guest.
>
> It's hard for me to develop a meaningful SEV test case, because I just
> finished my Google internship and thus lost access to SEV-enabled
> machines.

Removing this test case is fine. Though, it is convenient. But I
agree, it's redundant. Maybe we can tag any tests that are good to run
under SEV and/or SEV-ES via the `groups` field in the
x86/unittests.cfg file. The name `groups` is plural. So I assume that
a test can be a member of multiple groups. But I see no examples.