| Message ID | 20171129202701.16117-8-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x/tcg: cleanup and fix program interrupts | expand |
On 11/29/2017 08:26 PM, David Hildenbrand wrote: > s390_cpu_virt_mem_rw() must always return, so callers can react on > an exception (e.g. see ioinst_handle_stcrw()). > > However, for TCG we always have to exit the cpu loop (and restore the > cpu state before that) if we injected a program interrupt. So let's > introduce and use s390_cpu_virt_mem_handle_exc() in code that is not > purely KVM. > > Directly pass the retaddr we already have available in these functions. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/s390x/s390-pci-inst.c | 7 +++++++ > target/s390x/cpu.h | 1 + > target/s390x/ioinst.c | 20 +++++++++++++++++--- > target/s390x/mmu_helper.c | 14 ++++++++++++++ > 4 files changed, 39 insertions(+), 3 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 29.11.2017 21:26, David Hildenbrand wrote: > s390_cpu_virt_mem_rw() must always return, so callers can react on > an exception (e.g. see ioinst_handle_stcrw()). > > However, for TCG we always have to exit the cpu loop (and restore the > cpu state before that) if we injected a program interrupt. So let's > introduce and use s390_cpu_virt_mem_handle_exc() in code that is not > purely KVM. > > Directly pass the retaddr we already have available in these functions. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/s390x/s390-pci-inst.c | 7 +++++++ > target/s390x/cpu.h | 1 + > target/s390x/ioinst.c | 20 +++++++++++++++++--- > target/s390x/mmu_helper.c | 14 ++++++++++++++ > 4 files changed, 39 insertions(+), 3 deletions(-) > > diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c > index 8123705dfd..6f41083244 100644 > --- a/hw/s390x/s390-pci-inst.c > +++ b/hw/s390x/s390-pci-inst.c > @@ -163,6 +163,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra) > } > > if (s390_cpu_virt_mem_read(cpu, env->regs[r2], r2, buffer, sizeof(*reqh))) { > + s390_cpu_virt_mem_handle_exc(cpu, ra); > return 0; > } > reqh = (ClpReqHdr *)buffer; > @@ -174,6 +175,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra) > > if (s390_cpu_virt_mem_read(cpu, env->regs[r2], r2, buffer, > req_len + sizeof(*resh))) { > + s390_cpu_virt_mem_handle_exc(cpu, ra); > return 0; > } > resh = (ClpRspHdr *)(buffer + req_len); > @@ -189,6 +191,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra) > > if (s390_cpu_virt_mem_read(cpu, env->regs[r2], r2, buffer, > req_len + res_len)) { > + s390_cpu_virt_mem_handle_exc(cpu, ra); > return 0; > } [...] Having to change all these spots is kind of ugly ... and I guess it will also be quite error-prone in the future (if someone is developing new code under KVM only and then forgets to add these handlers for TCG). Can't you do that simply always at the end of s390_cpu_virt_mem_rw() instead, when the function has been called in non-checking mode (i.e. hostbuf != NULL)? Then we only need the extra-dance in places where we call the _check function? > @@ -212,9 +214,12 @@ void ioinst_handle_stcrw(S390CPU *cpu, uint32_t ipb, uintptr_t ra) > > if (s390_cpu_virt_mem_write(cpu, addr, ar, &crw, sizeof(crw)) == 0) { > setcc(cpu, cc); > - } else if (cc == 0) { > - /* Write failed: requeue CRW since STCRW is a suppressing instruction */ > - css_undo_stcrw(&crw); > + } else { > + if (cc == 0) { > + /* Write failed: requeue CRW since STCRW is suppressing */ > + css_undo_stcrw(&crw); > + } > + s390_cpu_virt_mem_handle_exc(cpu, ra); > } > } That hunk would then need to do _check first and in case there is a failure, call css_undo_stcrw() before the s390_cpu_virt_mem_handle_exc() function, I think. Alternatively, the s390_cpu_virt_mem_rw() function could get an additional parameter that points to a callback function which is used for "clean-up" right before the cpu_loop_exit ... and in this case the callback function would contain the css_undo_stcrw(). What do you think? Thomas
On 30.11.2017 12:01, Thomas Huth wrote: > On 29.11.2017 21:26, David Hildenbrand wrote: >> s390_cpu_virt_mem_rw() must always return, so callers can react on >> an exception (e.g. see ioinst_handle_stcrw()). >> >> However, for TCG we always have to exit the cpu loop (and restore the >> cpu state before that) if we injected a program interrupt. So let's >> introduce and use s390_cpu_virt_mem_handle_exc() in code that is not >> purely KVM. >> >> Directly pass the retaddr we already have available in these functions. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> hw/s390x/s390-pci-inst.c | 7 +++++++ >> target/s390x/cpu.h | 1 + >> target/s390x/ioinst.c | 20 +++++++++++++++++--- >> target/s390x/mmu_helper.c | 14 ++++++++++++++ >> 4 files changed, 39 insertions(+), 3 deletions(-) >> >> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c >> index 8123705dfd..6f41083244 100644 >> --- a/hw/s390x/s390-pci-inst.c >> +++ b/hw/s390x/s390-pci-inst.c >> @@ -163,6 +163,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra) >> } >> >> if (s390_cpu_virt_mem_read(cpu, env->regs[r2], r2, buffer, sizeof(*reqh))) { >> + s390_cpu_virt_mem_handle_exc(cpu, ra); >> return 0; >> } >> reqh = (ClpReqHdr *)buffer; >> @@ -174,6 +175,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra) >> >> if (s390_cpu_virt_mem_read(cpu, env->regs[r2], r2, buffer, >> req_len + sizeof(*resh))) { >> + s390_cpu_virt_mem_handle_exc(cpu, ra); >> return 0; >> } >> resh = (ClpRspHdr *)(buffer + req_len); >> @@ -189,6 +191,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra) >> >> if (s390_cpu_virt_mem_read(cpu, env->regs[r2], r2, buffer, >> req_len + res_len)) { >> + s390_cpu_virt_mem_handle_exc(cpu, ra); >> return 0; >> } > [...] > > Having to change all these spots is kind of ugly ... and I guess it will also > be quite error-prone in the future (if someone is developing new code under > KVM only and then forgets to add these handlers for TCG). The good thing is, it won't break KVM but only TCG. And it has been broken in TCG for years now without anybody noticing it. > > Can't you do that simply always at the end of s390_cpu_virt_mem_rw() instead, > when the function has been called in non-checking mode (i.e. hostbuf != NULL)? > Then we only need the extra-dance in places where we call the _check function? Oh god no, no such strange calling conventions from the same function. > >> @@ -212,9 +214,12 @@ void ioinst_handle_stcrw(S390CPU *cpu, uint32_t ipb, uintptr_t ra) >> >> if (s390_cpu_virt_mem_write(cpu, addr, ar, &crw, sizeof(crw)) == 0) { >> setcc(cpu, cc); >> - } else if (cc == 0) { >> - /* Write failed: requeue CRW since STCRW is a suppressing instruction */ >> - css_undo_stcrw(&crw); >> + } else { >> + if (cc == 0) { >> + /* Write failed: requeue CRW since STCRW is suppressing */ >> + css_undo_stcrw(&crw); >> + } >> + s390_cpu_virt_mem_handle_exc(cpu, ra); >> } >> } > > That hunk would then need to do _check first and in case there is a failure, > call css_undo_stcrw() before the s390_cpu_virt_mem_handle_exc() function, I think. I don't really like that. And there would be a possibility for a race. So a no from my side. > > Alternatively, the s390_cpu_virt_mem_rw() function could get an additional parameter > that points to a callback function which is used for "clean-up" right before the > cpu_loop_exit ... and in this case the callback function would contain the > css_undo_stcrw(). > > What do you think? I also thought about this, but adding two parameters to every call is even uglier. Not talking about having to construct special structs to pass through the data. Please not that the TPI handler I am about to introduce will also require to revert stuff in case there was an exception. Another option I thought about was checking in the caller if EXC_PGM has been set. But missing to add such a check is even easier. I'd prefer to keep it as is, but if there are other opinions/ideas, please speak up. Thanks for having a look! > > Thomas >
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c index 8123705dfd..6f41083244 100644 --- a/hw/s390x/s390-pci-inst.c +++ b/hw/s390x/s390-pci-inst.c @@ -163,6 +163,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra) } if (s390_cpu_virt_mem_read(cpu, env->regs[r2], r2, buffer, sizeof(*reqh))) { + s390_cpu_virt_mem_handle_exc(cpu, ra); return 0; } reqh = (ClpReqHdr *)buffer; @@ -174,6 +175,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra) if (s390_cpu_virt_mem_read(cpu, env->regs[r2], r2, buffer, req_len + sizeof(*resh))) { + s390_cpu_virt_mem_handle_exc(cpu, ra); return 0; } resh = (ClpRspHdr *)(buffer + req_len); @@ -189,6 +191,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra) if (s390_cpu_virt_mem_read(cpu, env->regs[r2], r2, buffer, req_len + res_len)) { + s390_cpu_virt_mem_handle_exc(cpu, ra); return 0; } @@ -308,6 +311,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra) out: if (s390_cpu_virt_mem_write(cpu, env->regs[r2], r2, buffer, req_len + res_len)) { + s390_cpu_virt_mem_handle_exc(cpu, ra); return 0; } setcc(cpu, cc); @@ -692,6 +696,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr, } if (s390_cpu_virt_mem_read(cpu, gaddr, ar, buffer, len)) { + s390_cpu_virt_mem_handle_exc(cpu, ra); return 0; } @@ -848,6 +853,7 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar, } if (s390_cpu_virt_mem_read(cpu, fiba, ar, (uint8_t *)&fib, sizeof(fib))) { + s390_cpu_virt_mem_handle_exc(cpu, ra); return 0; } @@ -1029,6 +1035,7 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar, out: if (s390_cpu_virt_mem_write(cpu, fiba, ar, (uint8_t *)&fib, sizeof(fib))) { + s390_cpu_virt_mem_handle_exc(cpu, ra); return 0; } diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 96abb2976b..ae61d18c0a 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -736,6 +736,7 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf, s390_cpu_virt_mem_rw(cpu, laddr, ar, dest, len, true) #define s390_cpu_virt_mem_check_write(cpu, laddr, ar, len) \ s390_cpu_virt_mem_rw(cpu, laddr, ar, NULL, len, true) +void s390_cpu_virt_mem_handle_exc(S390CPU *cpu, uintptr_t ra); /* sigp.c */ diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c index 25e0ad6f77..83c164a168 100644 --- a/target/s390x/ioinst.c +++ b/target/s390x/ioinst.c @@ -120,6 +120,7 @@ void ioinst_handle_msch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra) return; } if (s390_cpu_virt_mem_read(cpu, addr, ar, &schib, sizeof(schib))) { + s390_cpu_virt_mem_handle_exc(cpu, ra); return; } if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid) || @@ -176,6 +177,7 @@ void ioinst_handle_ssch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra) return; } if (s390_cpu_virt_mem_read(cpu, addr, ar, &orig_orb, sizeof(orb))) { + s390_cpu_virt_mem_handle_exc(cpu, ra); return; } copy_orb_from_guest(&orb, &orig_orb); @@ -212,9 +214,12 @@ void ioinst_handle_stcrw(S390CPU *cpu, uint32_t ipb, uintptr_t ra) if (s390_cpu_virt_mem_write(cpu, addr, ar, &crw, sizeof(crw)) == 0) { setcc(cpu, cc); - } else if (cc == 0) { - /* Write failed: requeue CRW since STCRW is a suppressing instruction */ - css_undo_stcrw(&crw); + } else { + if (cc == 0) { + /* Write failed: requeue CRW since STCRW is suppressing */ + css_undo_stcrw(&crw); + } + s390_cpu_virt_mem_handle_exc(cpu, ra); } } @@ -243,6 +248,8 @@ void ioinst_handle_stsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, */ if (!s390_cpu_virt_mem_check_write(cpu, addr, ar, sizeof(schib))) { s390_program_interrupt(env, PGM_OPERAND, 4, ra); + } else { + s390_cpu_virt_mem_handle_exc(cpu, ra); } return; } @@ -268,11 +275,13 @@ void ioinst_handle_stsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, if (cc != 3) { if (s390_cpu_virt_mem_write(cpu, addr, ar, &schib, sizeof(schib)) != 0) { + s390_cpu_virt_mem_handle_exc(cpu, ra); return; } } else { /* Access exceptions have a higher priority than cc3 */ if (s390_cpu_virt_mem_check_write(cpu, addr, ar, sizeof(schib)) != 0) { + s390_cpu_virt_mem_handle_exc(cpu, ra); return; } } @@ -309,6 +318,7 @@ int ioinst_handle_tsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra) /* 0 - status pending, 1 - not status pending, 3 - not operational */ if (cc != 3) { if (s390_cpu_virt_mem_write(cpu, addr, ar, &irb, irb_len) != 0) { + s390_cpu_virt_mem_handle_exc(cpu, ra); return -EFAULT; } css_do_tsch_update_subch(sch); @@ -316,6 +326,7 @@ int ioinst_handle_tsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, uintptr_t ra) irb_len = sizeof(irb) - sizeof(irb.emw); /* Access exceptions have a higher priority than cc3 */ if (s390_cpu_virt_mem_check_write(cpu, addr, ar, irb_len) != 0) { + s390_cpu_virt_mem_handle_exc(cpu, ra); return -EFAULT; } } @@ -611,6 +622,7 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra) * care of req->len here first. */ if (s390_cpu_virt_mem_read(cpu, addr, reg, buf, sizeof(ChscReq))) { + s390_cpu_virt_mem_handle_exc(cpu, ra); return; } req = (ChscReq *)buf; @@ -645,6 +657,8 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra) if (!s390_cpu_virt_mem_write(cpu, addr + len, reg, res, be16_to_cpu(res->len))) { setcc(cpu, 0); /* Command execution complete */ + } else { + s390_cpu_virt_mem_handle_exc(cpu, ra); } } diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c index 31e3f3f415..dbe2f511f8 100644 --- a/target/s390x/mmu_helper.c +++ b/target/s390x/mmu_helper.c @@ -22,6 +22,7 @@ #include "internal.h" #include "kvm_s390x.h" #include "sysemu/kvm.h" +#include "exec/exec-all.h" #include "trace.h" #include "hw/s390x/storage-keys.h" @@ -478,6 +479,9 @@ static int translate_pages(S390CPU *cpu, vaddr addr, int nr_pages, * * Copy from/to guest memory using logical addresses. Note that we inject a * program interrupt in case there is an error while accessing the memory. + * + * This function will always return (also for TCG), make sure to call + * s390_cpu_virt_mem_handle_exc() to properly exit the CPU loop. */ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf, int len, bool is_write) @@ -514,6 +518,16 @@ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf, return ret; } +void s390_cpu_virt_mem_handle_exc(S390CPU *cpu, uintptr_t ra) +{ + /* KVM will handle the interrupt automatically, TCG has to exit the TB */ +#ifdef CONFIG_TCG + if (tcg_enabled()) { + cpu_loop_exit_restore(CPU(cpu), ra); + } +#endif +} + /** * Translate a real address into a physical (absolute) address. * @param raddr the real address
s390_cpu_virt_mem_rw() must always return, so callers can react on an exception (e.g. see ioinst_handle_stcrw()). However, for TCG we always have to exit the cpu loop (and restore the cpu state before that) if we injected a program interrupt. So let's introduce and use s390_cpu_virt_mem_handle_exc() in code that is not purely KVM. Directly pass the retaddr we already have available in these functions. Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/s390x/s390-pci-inst.c | 7 +++++++ target/s390x/cpu.h | 1 + target/s390x/ioinst.c | 20 +++++++++++++++++--- target/s390x/mmu_helper.c | 14 ++++++++++++++ 4 files changed, 39 insertions(+), 3 deletions(-)