On Fri, Apr 15, 2022, Marc Orr wrote:
> On Fri, Apr 15, 2022 at 9:57 AM Sean Christopherson <seanjc@xxxxxxxxxx> 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.
