| Message ID | 20220224105451.5035-12-varad.gautam@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add #VC exception handling for AMD SEV-ES | expand |
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.
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. Regards,
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.
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.
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.
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.
On Fri, Apr 15, 2022 at 11:30 AM Sean Christopherson <seanjc@google.com> wrote: > > 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)); 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.
On Fri, Apr 15, 2022, Marc Orr wrote: > On Fri, Apr 15, 2022 at 11:30 AM Sean Christopherson <seanjc@google.com> wrote: > > 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. What test is ever going to do its own string port I/O? String MMIO is a different beast because REP MOVS and REP STOS can be generated by the compiler almost at will, e.g. memcpy(), memset(), struct initialization, random for-loops, etc... Port I/O on the other requires very deliberate code. I doubt it's even possible to generate string port I/O without resorting to assembly. Outside of emulator.c, the next closest instance is the use of KVM's "force emulation prefix", which happens to sometimes decode as INSL due to using byte 0x6d :-) realmode.c's print_serial() has string I/O, but (a) it's #ifdef'd out by default and (b) would be trivial to convert to a common helper. In other words, any test that does string I/O is going to have to either open code it in inline asm or call a helper. I'd much prefer we enable the latter. > 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. Yeah, they're very intertwined, not having to pull in a massive decoder is a big motivation for not wanting string I/O support in the #VC handler. But, even were that not the case, IMO bouncing through the #VC handler for string I/O is asinine because the source of the #VC _knows_ that it wants to do string I/O. Just call a helper.
diff --git a/lib/x86/amd_sev_vc.c b/lib/x86/amd_sev_vc.c index e8285f2..4fdf596 100644 --- a/lib/x86/amd_sev_vc.c +++ b/lib/x86/amd_sev_vc.c @@ -300,10 +300,46 @@ static enum es_result vc_ioio_exitinfo(struct es_em_ctxt *ctxt, u64 *exitinfo) return ES_OK; } +static enum es_result vc_insn_string_read(struct es_em_ctxt *ctxt, + void *src, unsigned char *buf, + unsigned int data_size, + unsigned int count, + bool backwards) +{ + int i, b = backwards ? -1 : 1; + + for (i = 0; i < count; i++) { + void *s = src + (i * data_size * b); + unsigned char *d = buf + (i * data_size); + + memcpy(d, s, data_size); + } + + return ES_OK; +} + +static enum es_result vc_insn_string_write(struct es_em_ctxt *ctxt, + void *dst, unsigned char *buf, + unsigned int data_size, + unsigned int count, + bool backwards) +{ + int i, s = backwards ? -1 : 1; + + for (i = 0; i < count; i++) { + void *d = dst + (i * data_size * s); + unsigned char *b = buf + (i * data_size); + + memcpy(d, b, data_size); + } + + return ES_OK; +} + static enum es_result vc_handle_ioio(struct ghcb *ghcb, struct es_em_ctxt *ctxt) { struct ex_regs *regs = ctxt->regs; - u64 exit_info_1; + u64 exit_info_1, exit_info_2; enum es_result ret; ret = vc_ioio_exitinfo(ctxt, &exit_info_1); @@ -311,7 +347,75 @@ static enum es_result vc_handle_ioio(struct ghcb *ghcb, struct es_em_ctxt *ctxt) return ret; if (exit_info_1 & IOIO_TYPE_STR) { - ret = ES_VMM_ERROR; + /* (REP) INS/OUTS */ + + bool df = ((regs->rflags & X86_EFLAGS_DF) == X86_EFLAGS_DF); + unsigned int io_bytes, exit_bytes; + unsigned int ghcb_count, op_count; + unsigned long es_base; + u64 sw_scratch; + + /* + * For the string variants with rep prefix the amount of in/out + * operations per #VC exception is limited so that the kernel + * has a chance to take interrupts and re-schedule while the + * instruction is emulated. + */ + io_bytes = (exit_info_1 >> 4) & 0x7; + ghcb_count = sizeof(ghcb->shared_buffer) / io_bytes; + + op_count = (exit_info_1 & IOIO_REP) ? regs->rcx : 1; + exit_info_2 = op_count < ghcb_count ? op_count : ghcb_count; + exit_bytes = exit_info_2 * io_bytes; + + es_base = 0; + + /* Read bytes of OUTS into the shared buffer */ + if (!(exit_info_1 & IOIO_TYPE_IN)) { + ret = vc_insn_string_read(ctxt, + (void *)(es_base + regs->rsi), + ghcb->shared_buffer, io_bytes, + exit_info_2, df); + if (ret) + return ret; + } + + /* + * Issue an VMGEXIT to the HV to consume the bytes from the + * shared buffer or to have it write them into the shared buffer + * depending on the instruction: OUTS or INS. + */ + sw_scratch = __pa(ghcb) + offsetof(struct ghcb, shared_buffer); + ghcb_set_sw_scratch(ghcb, sw_scratch); + ret = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_IOIO, + exit_info_1, exit_info_2); + if (ret != ES_OK) + return ret; + + /* Read bytes from shared buffer into the guest's destination. */ + if (exit_info_1 & IOIO_TYPE_IN) { + ret = vc_insn_string_write(ctxt, + (void *)(es_base + regs->rdi), + ghcb->shared_buffer, io_bytes, + exit_info_2, df); + if (ret) + return ret; + + if (df) + regs->rdi -= exit_bytes; + else + regs->rdi += exit_bytes; + } else { + if (df) + regs->rsi -= exit_bytes; + else + regs->rsi += exit_bytes; + } + + if (exit_info_1 & IOIO_REP) + regs->rcx -= exit_info_2; + + ret = regs->rcx ? ES_RETRY : ES_OK; } else { /* IN/OUT into/from rAX */