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

[Sean Christopherson <seanjc@google.com>]

On Fri, Apr 15, 2022, Marc Orr 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.

(a) that's debatable, (b) it's a moot point because we can and should add a helper
to do the dirty work.  E.g.

  static void sev_es_do_string_io(..., int port, int size, int count, void *data);

I say it's debatable because it's not like this is the most pleasant code to read:

	asm volatile("cld \n\t"
		     "movw %0, %%dx \n\t"
		     "rep outsb \n\t"
		     : : "i"((short)TESTDEV_IO_PORT),
		       "S"(st1), "c"(sizeof(st1) - 1));

> - 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 strongly disagree.  This

	static char st1[] = "abcdefghijklmnop";

	static void test_stringio(void)
	{
		unsigned char r = 0;
		asm volatile("cld \n\t"
			"movw %0, %%dx \n\t"
			"rep outsb \n\t"
			: : "i"((short)TESTDEV_IO_PORT),
			"S"(st1), "c"(sizeof(st1) - 1));
		asm volatile("inb %1, %0\n\t" : "=a"(r) : "i"((short)TESTDEV_IO_PORT));
		report(r == st1[sizeof(st1) - 2], "outsb up"); /* last char */

		asm volatile("std \n\t"
			"movw %0, %%dx \n\t"
			"rep outsb \n\t"
			: : "i"((short)TESTDEV_IO_PORT),
			"S"(st1 + sizeof(st1) - 2), "c"(sizeof(st1) - 1));
		asm volatile("cld \n\t" : : );
		asm volatile("in %1, %0\n\t" : "=a"(r) : "i"((short)TESTDEV_IO_PORT));
		report(r == st1[0], "outsb down");
	}

is not easy to write or read.
  
I'm envisioning SEV-ES string I/O tests will be things like:

	sev_es_outsb(..., TESTDEV_IO_PORT, sizeof(st1) - 1, st1);

	sev_es_outsb_backwards(..., TESTDEV_IO_PORT, sizeof(st1) - 1,
			       st1 + sizeof(st1) - 2));

where sev_es_outsb() is a wrapper to sev_es_do_string_io() or whatever and fills
in all the appropriate SEV-ES IOIO_* constants.

Yes, we can and probably should add wrappers for the raw string I/O tests too.
But, no matter what, somehwere there has to be a helper to translate raw string
I/O into SEV-ES string I/O.  I don't see why doing that in the #VC handler is any
easier than doing it in a helper.

> - 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.

The #VC handler does things like:

	ghcb_count = sizeof(ghcb->shared_buffer) / io_bytes;

to explicitly not mess up the _guest_ kernel.  The proposed #VC handler literally
cannot generate:

  - a string I/O request larger than 2032 bytes
  - does not reside inside the GHCB's internal buffer
  - spans multiple pages
  - points at illegal memory

And so on an so forth.  And if we add helpers to allow that, then what value does
the #VC handler provide since adding a wrapper to follow the Linux guest approach
would be trivial?

> 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.