On Fri, Apr 15, 2022 at 11:30 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> 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));
Yeah, if we have a helper that resolves most of my concerns. (More on
this below.)
> > - 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.
Agreed. But having to also "Bring Your Own #VC Handler" makes it even
harder. Which is my point.
If we have helpers to load a #VC handler, then that resolves most of
my concerns. Though, I still think having a default #VC handler for
string IO is better than not having one. (More on that below.)
> 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.
Hmmm... yeah, if this patch really does get vetoed then rather than
throw it away maybe we can convert it to be loaded with a helper now.
Note: I hear your arguments, but I still don't agree with throwing
away this patch. At least not based on the arguments made in this
email thread. I think having a default #VC handler to handle string IO
is better than not having one. Individual tests can always override
it. From reading the other email thread on the decoder, I get the
sense that the real reason you're opposed to this patch is because
you're opposed to pulling in the Linux decoder. I don't follow that
patch as well as this one. So that may or may not be a valid reason to
nuke this patch. I'll leave that for others to discuss.
> > - 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?
Fair point. But having the #VC handler doesn't prevent tests from
pivoting to their own handler when needed.
