| Message ID | 20171128203326.6062-8-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x/tcg: cleanup and fix program interrupts | expand |
On Tue, 28 Nov 2017 21:33:17 +0100 David Hildenbrand <david@redhat.com> wrote: > s390_cpu_virt_mem_rw() must always return, so callers can react on > an exception (e.g. see ioinst_handle_stcrw()). For TCG, there was one > case where a cpu loop exit was triggered. Fix that up. > > 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 | 21 ++++++++++++++++++--- > target/s390x/mmu_helper.c | 19 ++++++++++++++++++- > 4 files changed, 44 insertions(+), 4 deletions(-) > > diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c > index 4b6d38f946..0851bf6fef 100644 > --- a/target/s390x/ioinst.c > +++ b/target/s390x/ioinst.c > @@ -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))) { > program_interrupt_ra(env, PGM_OPERAND, 4, ra); > + } else { > + s390_cpu_virt_mem_handle_exc(cpu, ra); This looks a bit odd if you don't realize that kvm already handled the exception. But I don't really have a better idea. > } > return; > } > @@ -645,9 +657,12 @@ 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); > } > } > > + unrelated whitespace > #define SCHM_REG1_RES(_reg) (_reg & 0x000000000ffffffc) > #define SCHM_REG1_MBK(_reg) ((_reg & 0x00000000f0000000) >> 28) > #define SCHM_REG1_UPD(_reg) ((_reg & 0x0000000000000002) >> 1) > diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c > index 31e3f3f415..39da9aeef4 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" > > @@ -458,7 +459,7 @@ static int translate_pages(S390CPU *cpu, vaddr addr, int nr_pages, > } > if (!address_space_access_valid(&address_space_memory, pages[i], > TARGET_PAGE_SIZE, is_write)) { > - program_interrupt(env, PGM_ADDRESSING, ILEN_AUTO); > + trigger_pgm_exception(env, PGM_ADDRESSING, ILEN_AUTO); Why did you change this? > return -EFAULT; > } > addr += TARGET_PAGE_SIZE; > @@ -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,19 @@ 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 Please move the #ifdef/#endif to the beginning of the line. > + if (tcg_enabled()) { > + if (ra) { > + cpu_restore_state(CPU(cpu), ra); > + } > + cpu_loop_exit(CPU(cpu)); > + } > + #endif > +} > + > /** > * Translate a real address into a physical (absolute) address. > * @param raddr the real address
> This looks a bit odd if you don't realize that kvm already handled the > exception. But I don't really have a better idea. > >> } >> return; >> } > >> @@ -645,9 +657,12 @@ 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); >> } >> } >> >> + > > unrelated whitespace ack, will drop. > >> #define SCHM_REG1_RES(_reg) (_reg & 0x000000000ffffffc) >> #define SCHM_REG1_MBK(_reg) ((_reg & 0x00000000f0000000) >> 28) >> #define SCHM_REG1_UPD(_reg) ((_reg & 0x0000000000000002) >> 1) >> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c >> index 31e3f3f415..39da9aeef4 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" >> >> @@ -458,7 +459,7 @@ static int translate_pages(S390CPU *cpu, vaddr addr, int nr_pages, >> } >> if (!address_space_access_valid(&address_space_memory, pages[i], >> TARGET_PAGE_SIZE, is_write)) { >> - program_interrupt(env, PGM_ADDRESSING, ILEN_AUTO); >> + trigger_pgm_exception(env, PGM_ADDRESSING, ILEN_AUTO); > > Why did you change this? "For TCG, there was one case where a cpu loop exit was triggered. Fix that up." Wasn't worth a separate patch :) > >> return -EFAULT; >> } >> addr += TARGET_PAGE_SIZE; >> @@ -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,19 @@ 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 > > Please move the #ifdef/#endif to the beginning of the line. Oh, yes. > >> + if (tcg_enabled()) { >> + if (ra) { >> + cpu_restore_state(CPU(cpu), ra); >> + } >> + cpu_loop_exit(CPU(cpu)); >> + } >> + #endif >> +} >> + >> /** >> * Translate a real address into a physical (absolute) address. >> * @param raddr the real address >
On Wed, 29 Nov 2017 16:54:30 +0100 David Hildenbrand <david@redhat.com> wrote: > >> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c > >> index 31e3f3f415..39da9aeef4 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" > >> > >> @@ -458,7 +459,7 @@ static int translate_pages(S390CPU *cpu, vaddr addr, int nr_pages, > >> } > >> if (!address_space_access_valid(&address_space_memory, pages[i], > >> TARGET_PAGE_SIZE, is_write)) { > >> - program_interrupt(env, PGM_ADDRESSING, ILEN_AUTO); > >> + trigger_pgm_exception(env, PGM_ADDRESSING, ILEN_AUTO); > > > > Why did you change this? > > "For TCG, there was one case where a cpu loop exit was triggered. Fix > that up." > > Wasn't worth a separate patch :) Well, maybe it is, to avoid dumb questions :) > > > > >> return -EFAULT; > >> } > >> addr += TARGET_PAGE_SIZE;
On 29.11.2017 16:59, Cornelia Huck wrote: > On Wed, 29 Nov 2017 16:54:30 +0100 > David Hildenbrand <david@redhat.com> wrote: > > >>>> diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c >>>> index 31e3f3f415..39da9aeef4 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" >>>> >>>> @@ -458,7 +459,7 @@ static int translate_pages(S390CPU *cpu, vaddr addr, int nr_pages, >>>> } >>>> if (!address_space_access_valid(&address_space_memory, pages[i], >>>> TARGET_PAGE_SIZE, is_write)) { >>>> - program_interrupt(env, PGM_ADDRESSING, ILEN_AUTO); >>>> + trigger_pgm_exception(env, PGM_ADDRESSING, ILEN_AUTO); >>> >>> Why did you change this? >> >> "For TCG, there was one case where a cpu loop exit was triggered. Fix >> that up." >> >> Wasn't worth a separate patch :) > > Well, maybe it is, to avoid dumb questions :) ... splitting it up :) Thanks!
On 11/28/2017 08:33 PM, David Hildenbrand wrote: > +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()) { > + if (ra) { > + cpu_restore_state(CPU(cpu), ra); > + } > + cpu_loop_exit(CPU(cpu)); > + } In addition to everything else, cpu_loop_exit_restore(CPU(cpu), ra); r~
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c index 15ba8f55ae..3671432a29 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 e21d63ae04..617f9b0148 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -735,6 +735,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 4b6d38f946..0851bf6fef 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))) { program_interrupt_ra(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,9 +657,12 @@ 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); } } + #define SCHM_REG1_RES(_reg) (_reg & 0x000000000ffffffc) #define SCHM_REG1_MBK(_reg) ((_reg & 0x00000000f0000000) >> 28) #define SCHM_REG1_UPD(_reg) ((_reg & 0x0000000000000002) >> 1) diff --git a/target/s390x/mmu_helper.c b/target/s390x/mmu_helper.c index 31e3f3f415..39da9aeef4 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" @@ -458,7 +459,7 @@ static int translate_pages(S390CPU *cpu, vaddr addr, int nr_pages, } if (!address_space_access_valid(&address_space_memory, pages[i], TARGET_PAGE_SIZE, is_write)) { - program_interrupt(env, PGM_ADDRESSING, ILEN_AUTO); + trigger_pgm_exception(env, PGM_ADDRESSING, ILEN_AUTO); return -EFAULT; } addr += TARGET_PAGE_SIZE; @@ -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,19 @@ 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()) { + if (ra) { + cpu_restore_state(CPU(cpu), ra); + } + cpu_loop_exit(CPU(cpu)); + } + #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()). For TCG, there was one case where a cpu loop exit was triggered. Fix that up. 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 | 21 ++++++++++++++++++--- target/s390x/mmu_helper.c | 19 ++++++++++++++++++- 4 files changed, 44 insertions(+), 4 deletions(-)