Re: [kvm-unit-tests PATCH v3 11/11] x86: AMD SEV-ES: Handle string IO for IOIO #VC

From: Marc Orr <marcorr@google.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Joerg Roedel <jroedel@suse.de>,
	Varad Gautam <varad.gautam@suse.com>,
	kvm list <kvm@vger.kernel.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Andrew Jones <drjones@redhat.com>,
	Zixuan Wang <zxwang42@gmail.com>,
	Erdem Aktas <erdemaktas@google.com>,
	David Rientjes <rientjes@google.com>,
	"Singh, Brijesh" <brijesh.singh@amd.com>,
	"Lendacky, Thomas" <Thomas.Lendacky@amd.com>,
	bp@suse.de
Subject: Re: [kvm-unit-tests PATCH v3 11/11] x86: AMD SEV-ES: Handle string IO for IOIO #VC
Date: Fri, 15 Apr 2022 10:42:28 -0700	[thread overview]
Message-ID: <CAA03e5EZS2ydJTabsoqa5H+Pe1BL58BA+8F=m+r3qmhJURiA-w@mail.gmail.com> (raw)
In-Reply-To: <CAA03e5ETN6dhjSpPYTAGicCuKGjaTe-kVvAaMDC1=_EONfL=Sw@mail.gmail.com>

On Fri, Apr 15, 2022 at 10:22 AM Marc Orr <marcorr@google.com> wrote:
>
> On Fri, Apr 15, 2022 at 9:57 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Fri, Apr 08, 2022, Joerg Roedel wrote:
> > > On Wed, Apr 06, 2022 at 01:50:29AM +0000, Sean Christopherson wrote:
> > > > On Thu, Feb 24, 2022, Varad Gautam wrote:
> > > > > Using Linux's IOIO #VC processing logic.
> > > >
> > > > How much string I/O is there in KUT?  I assume it's rare, i.e. avoiding it entirely
> > > > is probably less work in the long run.
> > >
> > > The problem is that SEV-ES support will silently break if someone adds
> > > it unnoticed and without testing changes on SEV-ES.
> >
> > But IMO that is extremely unlikely to happen.  objdump + grep shows that the only
> > string I/O in KUT comes from the explicit asm in emulator.c and amd_sev.c.  And
> > the existence of amd_sev.c's version suggests that emulator.c isn't supported.
> > I.e. this is being added purely for an SEV specific test, which is silly.
> >
> > And it's not like we're getting validation coverage of the exit_info, that also
> > comes from software in vc_ioio_exitinfo().
> >
> > Burying this in the #VC handler makes it so much harder to understand what is
> > actually be tested, and will make it difficult to test the more interesting edge
> > cases.  E.g. I'd really like to see a test that requests string I/O emulation for
> > a buffer that's beyond the allowed size, straddles multiple pages, walks into
> > non-existent memory, etc.., and doing those with a direct #VMGEXIT will be a lot
> > easier to write and read then bouncing through the #VC handler.
>
> For the record, I like the current approach of implementing a #VC
> handler within kvm-unit-tests itself for the string IO.
>
> Rationale:
> - Makes writing string IO tests easy.
> - We get some level of testing of the #VC handler in the guest kernel
> in the sense that this #VC handler is based on that one. So if we find
> an issue in this handler we know we probably need to fix that same
> issue in the guest kernel #VC handler.
> - I don't follow the argument that having a direct #VMGEXIT in the
> test itself makes the test easerit to write and read. It's going to
> add a lot of extra code to the test that makes it hard to parse the
> actual string IO operations and expectations IMHO.
> - I agree that writing test cases to straddle multiple pages, walk
> into non-existent memory, etc. is an excellent idea. But I don't
> follow how exposing the test itself to the #VC exit makes this easier.
> Worst case, the kvm-unit-tests can be extended with some sort of
> helper to expose to the test the scratch buffer size and whether it's
> embedded in the GHCB or external.

Also, having this handler does not stop anyone from contributing a
more elaborate test that turns out to need to implement its own #VC
handling.