s390x/tcg: cleanup and fix program interrupts
cover s390x/tcg: cleanup and fix program interrupts
I have quite some patches on my queue for 2.12. (booting Fedora 26/27 guests, floating interrupts, machine checks, missing instructions ...)
So let's start slowly :) This series gets rid of program_interrupt() and potential_page_fault(). We now always properly restore the cpu state when injecting/delivering a program interrupt. So there is no need to update the state via potential_page_fault() anymore.
In addition, handling for program interrupts comming via s390_cpu_virt_mem_rw() was missing something for the TCG case. Also fixed.
David Hildenbrand (15):
s390x/tcg: introduce and use program_interrupt_ra() s390x/tcg: get rid of runtime_exception()
s390x/tcg: rip out dead tpi code
s390x/ioinst: pass the retaddr to all IO instructions s390x/pci: pass the retaddr to all PCI instructions s390x/diag: pass the retaddr into handle_diag_308()
s390x: handle exceptions during s390_cpu_virt_mem_rw() correctly (TCG) s390x/tcg: io instructions don't need potential_page_fault() s390x/tcg: use program_interrupt_ra() in SCLP Service Call s390x/tcg: use program_interrupt_ra() in DIAG
s390x/tcg: use program_interrupt_ra() in per_check_exception() s390x/tcg: use program_interrupt_ra() in SACF
s390x/tcg: use program_interrupt_ra() in STSI
s390x/tcg: drop program_interrupt()
s390x/tcg: drop potential_page_fault()
hw/s390x/css.c | 6 --- hw/s390x/s390-pci-inst.c | 90 ++++++++++++++++++---------------- hw/s390x/s390-pci-inst.h | 16 ++++--- include/hw/s390x/css.h | 1 - target/s390x/cc_helper.c | 2 +- target/s390x/cpu.h | 4 +- target/s390x/crypto_helper.c | 7 +-- target/s390x/diag.c | 14 +++--- target/s390x/excp_helper.c | 5 +- target/s390x/fpu_helper.c | 2 +- target/s390x/int_helper.c | 14 +++--- target/s390x/internal.h | 35 +++++++------- target/s390x/interrupt.c | 9 +++- target/s390x/ioinst.c | 112 ++++++++++++++++++++----------------------- target/s390x/kvm.c | 42 ++++++++-------- target/s390x/mem_helper.c | 35 +++++--------- target/s390x/misc_helper.c | 50 +++++++------------ target/s390x/mmu_helper.c | 19 +++++++- target/s390x/translate.c | 27 +---------- 19 files changed, 226 insertions(+), 264 deletions(-)
Signature · 1 line
2.14.3
PATCH 1/15 s390x/tcg: introduce and use program_interrupt_ra()
Allows to easily convert more callers of program_interrupt() and to easily introduce new exceptions without forgetting about the cpu state reset.
Use program_interrupt_ra() in places where we already had the same pattern.
--- target/s390x/cpu.h | 2 ++ target/s390x/crypto_helper.c | 7 ++----- target/s390x/excp_helper.c | 5 +---- target/s390x/interrupt.c | 13 +++++++++++++ target/s390x/mem_helper.c | 35 +++++++++++------------------------ target/s390x/misc_helper.c | 3 +-- 6 files changed, 30 insertions(+), 35 deletions(-)
target/s390x/cpu.h +2 -0
@@ -720,6 +720,8 @@ void s390_io_interrupt(uint16_t subchannel_id, uint16_t subchannel_nr,/* automatically detect the instruction length */ #define ILEN_AUTO 0xffvoid program_interrupt (CPUS390XState *env,uint32_t code,int ilen); +void program_interrupt_ra (CPUS390XState *env,uint32_t code,int ilen, +uintptr_t ra);/* service interrupts are floating therefore we must not pass an cpustate */ void s390_sclp_extint (uint32_t parm);
target/s390x/crypto_helper.c +2 -5
@@ -23,7 +23,6 @@ uint32_t HELPER(msa)(CPUS390XState *env, uint32_t r1, uint32_t r2, uint32_t r3,const uintptr_t ra =GETPC ();const uint8_t mod = env->regs[0 ] &0x80ULL ;const uint8_t fc = env->regs[0 ] &0x7fULL ; -CPUState *cs =CPU (s390_env_get_cpu (env));uint8_t subfunc[16 ] = {0 };uint64_t param_addr;int i;
@@ -35,8 +34,7 @@ uint32_t HELPER(msa)(CPUS390XState *env, uint32_t r1, uint32_t r2, uint32_t r3,case S390_FEAT_TYPE_PCKMO :case S390_FEAT_TYPE_PCC :if (mod) { -cpu_restore_state (cs, ra); -program_interrupt (env,PGM_SPECIFICATION ,4 ); +program_interrupt_ra (env,PGM_SPECIFICATION ,4 , ra);return 0 ; }break ;
@@ -44,8 +42,7 @@ uint32_t HELPER(msa)(CPUS390XState *env, uint32_t r1, uint32_t r2, uint32_t r3,s390_get_feat_block (type, subfunc);if (!test_be_bit (fc, subfunc)) { -cpu_restore_state (cs, ra); -program_interrupt (env,PGM_SPECIFICATION ,4 ); +program_interrupt_ra (env,PGM_SPECIFICATION ,4 , ra);return 0 ; }
target/s390x/excp_helper.c +1 -4
@@ -554,10 +554,7 @@ void s390x_cpu_do_unaligned_access(CPUState *cs, vaddr addr,S390CPU *cpu =S390_CPU (cs);CPUS390XState *env = &cpu->env; -if (retaddr) { -cpu_restore_state (cs, retaddr); - } - program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO); + program_interrupt_ra(env, PGM_SPECIFICATION, ILEN_AUTO, retaddr); }#endif /* CONFIG_USER_ONLY */
target/s390x/interrupt.c +13 -0
@@ -53,6 +53,19 @@ void program_interrupt(CPUS390XState *env, uint32_t code, int ilen) } } +void program_interrupt_ra (CPUS390XState *env,uint32_t code,int ilen, +uintptr_t ra) +{ +S390CPU *cpu =s390_env_get_cpu (env); + +#ifdef CONFIG_TCG +if (tcg_enabled () && ra) { +cpu_restore_state (CPU (cpu), ra); + } +#endif +program_interrupt (env, code, ilen); +} +#if !defined(CONFIG_USER_ONLY )static void cpu_inject_service (S390CPU *cpu,uint32_t param) {
target/s390x/mem_helper.c +11 -24
@@ -85,9 +85,7 @@ static inline void check_alignment(CPUS390XState *env, uint64_t v, int wordsize, uintptr_t ra) {if (v % wordsize) { -CPUState *cs =CPU (s390_env_get_cpu (env)); -cpu_restore_state (cs, ra); -program_interrupt (env,PGM_SPECIFICATION ,6 ); +program_interrupt_ra (env,PGM_SPECIFICATION ,6 , ra); } }
@@ -545,8 +543,7 @@ void HELPER(srst)(CPUS390XState *env, uint32_t r1, uint32_t r2)/* Bits 32-55 must contain all 0. */ if (env->regs[0 ] &0xffffff00u ) { -cpu_restore_state (ENV_GET_CPU (env), ra); -program_interrupt (env,PGM_SPECIFICATION ,6 ); +program_interrupt_ra (env,PGM_SPECIFICATION ,6 , ra); } str =get_address (env, r2);
@@ -583,8 +580,7 @@ void HELPER(srstu)(CPUS390XState *env, uint32_t r1, uint32_t r2)/* Bits 32-47 of R0 must be zero. */ if (env->regs[0 ] &0xffff0000u ) { -cpu_restore_state (ENV_GET_CPU (env), ra); -program_interrupt (env,PGM_SPECIFICATION ,6 ); +program_interrupt_ra (env,PGM_SPECIFICATION ,6 , ra); } str =get_address (env, r2);
@@ -1600,8 +1596,7 @@ static uint32_t do_csst(CPUS390XState *env, uint32_t r3, uint64_t a1,return cc; spec_exception: -cpu_restore_state (ENV_GET_CPU (env), ra); -program_interrupt (env,PGM_SPECIFICATION ,6 ); +program_interrupt_ra (env,PGM_SPECIFICATION ,6 , ra);g_assert_not_reached (); }
@@ -1865,8 +1860,7 @@ void HELPER(idte)(CPUS390XState *env, uint64_t r1, uint64_t r2, uint32_t m4)uint16_t entries, i, index =0 ;if (r2 &0xff000 ) { -cpu_restore_state (cs, ra); -program_interrupt (env,PGM_SPECIFICATION ,4 ); +program_interrupt_ra (env,PGM_SPECIFICATION ,4 , ra); }if (!(r2 &0x800 )) {
@@ -2014,8 +2008,7 @@ uint64_t HELPER(lra)(CPUS390XState *env, uint64_t addr)/* XXX incomplete - has more corner cases */ if (!(env->psw.mask &PSW_MASK_64 ) && (addr >>32 )) { -cpu_restore_state (cs,GETPC ()); -program_interrupt (env,PGM_SPECIAL_OP ,2 ); +program_interrupt_ra (env,PGM_SPECIAL_OP ,2 ,GETPC ()); } old_exc = cs->exception_index;
@@ -2185,7 +2178,6 @@ uint32_t HELPER(mvcos)(CPUS390XState *env, uint64_t dest, uint64_t src,const uint8_t psw_as = (env->psw.mask &PSW_MASK_ASC ) >>PSW_SHIFT_ASC ;const uint64_t r0 = env->regs[0 ];const uintptr_t ra =GETPC (); -CPUState *cs =CPU (s390_env_get_cpu (env));uint8_t dest_key, dest_as, dest_k, dest_a;uint8_t src_key, src_as, src_k, src_a;uint64_t val;
@@ -2195,8 +2187,7 @@ uint32_t HELPER(mvcos)(CPUS390XState *env, uint64_t dest, uint64_t src, __func__, dest, src, len);if (!(env->psw.mask &PSW_MASK_DAT )) { -cpu_restore_state (cs, ra); -program_interrupt (env,PGM_SPECIAL_OP ,6 ); +program_interrupt_ra (env,PGM_SPECIAL_OP ,6 , ra); }/* OAC (operand access control) for the first operand -> dest */
@@ -2227,17 +2218,14 @@ uint32_t HELPER(mvcos)(CPUS390XState *env, uint64_t dest, uint64_t src, }if (dest_a && dest_as ==AS_HOME && (env->psw.mask &PSW_MASK_PSTATE )) { -cpu_restore_state (cs, ra); -program_interrupt (env,PGM_SPECIAL_OP ,6 ); +program_interrupt_ra (env,PGM_SPECIAL_OP ,6 , ra); }if (!(env->cregs[0 ] &CR0_SECONDARY ) && (dest_as ==AS_SECONDARY || src_as ==AS_SECONDARY )) { -cpu_restore_state (cs, ra); -program_interrupt (env,PGM_SPECIAL_OP ,6 ); +program_interrupt_ra (env,PGM_SPECIAL_OP ,6 , ra); }if (!psw_key_valid (env, dest_key) || !psw_key_valid (env, src_key)) { -cpu_restore_state (cs, ra); -program_interrupt (env,PGM_PRIVILEGED ,6 ); +program_interrupt_ra (env,PGM_PRIVILEGED ,6 , ra); } len =wrap_length (env, len);
@@ -2251,8 +2239,7 @@ uint32_t HELPER(mvcos)(CPUS390XState *env, uint64_t dest, uint64_t src, (env->psw.mask & PSW_MASK_PSTATE)) {qemu_log_mask (LOG_UNIMP ,"%s: AR-mode and PSTATE support missing\n" , __func__); -cpu_restore_state (cs, ra); -program_interrupt (env,PGM_ADDRESSING ,6 ); +program_interrupt_ra (env,PGM_ADDRESSING ,6 , ra); }/* FIXME: a) LAP
target/s390x/misc_helper.c +1 -2
@@ -519,8 +519,7 @@ uint32_t HELPER(stfle)(CPUS390XState *env, uint64_t addr)int i;if (addr &0x7 ) { -cpu_restore_state (ENV_GET_CPU (env), ra); -program_interrupt (env,PGM_SPECIFICATION ,4 ); +program_interrupt_ra (env,PGM_SPECIFICATION ,4 , ra); }prepare_stfl ();
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2.12 01/15] s390x/tcg: introduce and use program_interrupt_ra()
Quoted 22 lines · On Tue, 28 Nov 2017 21:33:11 +0100 David Hildenbrand <[email protected]> wrote:
Allows to easily convert more callers of
program_interrupt()and to easily introduce new exceptions without forgetting about the cpu state reset.Use
program_interrupt_ra()in places where we already had the same pattern.Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/cpu.h | 2 ++ target/s390x/crypto_helper.c | 7 ++----- target/s390x/excp_helper.c | 5 +---- target/s390x/interrupt.c | 13 +++++++++++++ target/s390x/mem_helper.c | 35 +++++++++++------------------------ target/s390x/misc_helper.c | 3 +-- 6 files changed, 30 insertions(+), 35 deletions(-)target/s390x/interrupt.c +0
-0
target/s390x/interrupt.c void program_interrupt(CPUS390XState *env, uint32_t code, int ilen) · line 59
Quoted 7 lines
target/s390x/interrupt.c +4
-0@@ -53,6 +53,19 @@ void program_interrupt(CPUS390XState *env, uint32_t code, int ilen) } } +void program_interrupt_ra (CPUS390XState *env,uint32_t code,int ilen, +uintptr_t ra) +{ +S390CPU *cpu =s390_env_get_cpu (env);
Move this under the if?
Quoted 12 lines
target/s390x/interrupt.c +9
-0+ +#ifdef CONFIG_TCG +if (tcg_enabled () && ra) { +cpu_restore_state (CPU (cpu), ra); + } +#endif +program_interrupt (env, code, ilen); +} +#if !defined(CONFIG_USER_ONLY )static void cpu_inject_service (S390CPU *cpu,uint32_t param) {
Re: [Qemu-devel] [PATCH v1 for-2.12 01/15] s390x/tcg: introduce and use program_interrupt_ra()
Quoted 28 lines · Cornelia Huck
Quoted 25 lines · On Tue, 28 Nov 2017 21:33:11 +0100 David Hildenbrand <[email protected]> wrote:
Allows to easily convert more callers of
program_interrupt()and to easily introduce new exceptions without forgetting about the cpu state reset.Use
program_interrupt_ra()in places where we already had the same pattern.Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/cpu.h | 2 ++ target/s390x/crypto_helper.c | 7 ++----- target/s390x/excp_helper.c | 5 +---- target/s390x/interrupt.c | 13 +++++++++++++ target/s390x/mem_helper.c | 35 +++++++++++------------------------ target/s390x/misc_helper.c | 3 +-- 6 files changed, 30 insertions(+), 35 deletions(-)target/s390x/interrupt.c +0
-0@@ -53,6 +53,19 @@ void program_interrupt(CPUS390XState *env, uint32_t code, int ilen) } }
Quoted 7 lines
target/s390x/interrupt.c void program_interrupt(CPUS390XState *env, uint32_t code, int ilen) · line 59
Quoted 4 lines
target/s390x/interrupt.c +4
-0+void program_interrupt_ra (CPUS390XState *env,uint32_t code,int ilen, +uintptr_t ra) +{ +S390CPU *cpu =s390_env_get_cpu (env);Move this under the if?
Indeed, thanks.
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v1 for-2.12 01/15] s390x/tcg: introduce and use program_interrupt_ra()
In reply to David Hildenbrandtarget/s390x/interrupt.c void program_interrupt(CPUS390XState *env, uint32_t code, int ilen) · line 65
Quoted 7 lines · David Hildenbrand
target/s390x/interrupt.c +7
-0+S390CPU *cpu =s390_env_get_cpu (env); + +#ifdef CONFIG_TCG +if (tcg_enabled () && ra) { +cpu_restore_state (CPU (cpu), ra); + } +#endif
FWIW, I have a patch queued for 2.12 that removes the RA != 0 check protecting calls to cpu_restore_state. That condition is already handled inside and we don't need to duplicate it.
Otherwise,
r~
PATCH 2/15 s390x/tcg: get rid of runtime_exception()
In reply to David HildenbrandLet's use program_interrupt_ra() instead.
--- target/s390x/fpu_helper.c | 2 +- target/s390x/int_helper.c | 14 +++++++------- target/s390x/internal.h | 2 -- target/s390x/misc_helper.c | 16 ---------------- 4 files changed, 8 insertions(+), 26 deletions(-)
target/s390x/fpu_helper.c +1 -1
@@ -44,7 +44,7 @@ static void ieee_exception(CPUS390XState *env, uint32_t dxc, uintptr_t retaddr)/* Install the DXC code. */ env->fpc = (env->fpc & ~0xff00 ) | (dxc <<8 );/* Trap. */ -runtime_exception (env,PGM_DATA , retaddr); +program_interrupt_ra (env,PGM_DATA ,ILEN_AUTO , retaddr); }/* Should be called after any operation that may raise IEEE exceptions. */
target/s390x/int_helper.c +7 -7
@@ -39,7 +39,7 @@ int64_t HELPER(divs32)(CPUS390XState *env, int64_t a, int64_t b64)int64_t q;if (b ==0 ) { -runtime_exception (env,PGM_FIXPT_DIVIDE ,GETPC ()); +program_interrupt_ra (env,PGM_FIXPT_DIVIDE ,ILEN_AUTO ,GETPC ()); } ret = q = a / b;
@@ -47,7 +47,7 @@ int64_t HELPER(divs32)(CPUS390XState *env, int64_t a, int64_t b64)/* Catch non-representable quotient. */ if (ret != q) { -runtime_exception (env,PGM_FIXPT_DIVIDE ,GETPC ()); +program_interrupt_ra (env,PGM_FIXPT_DIVIDE ,ILEN_AUTO ,GETPC ()); }return ret;
@@ -60,7 +60,7 @@ uint64_t HELPER(divu32)(CPUS390XState *env, uint64_t a, uint64_t b64)uint64_t q;if (b ==0 ) { -runtime_exception (env,PGM_FIXPT_DIVIDE ,GETPC ()); +program_interrupt_ra (env,PGM_FIXPT_DIVIDE ,ILEN_AUTO ,GETPC ()); } ret = q = a / b;
@@ -68,7 +68,7 @@ uint64_t HELPER(divu32)(CPUS390XState *env, uint64_t a, uint64_t b64)/* Catch non-representable quotient. */ if (ret != q) { -runtime_exception (env,PGM_FIXPT_DIVIDE ,GETPC ()); +program_interrupt_ra (env,PGM_FIXPT_DIVIDE ,ILEN_AUTO ,GETPC ()); }return ret;
@@ -79,7 +79,7 @@ int64_t HELPER(divs64)(CPUS390XState *env, int64_t a, int64_t b) {/* Catch divide by zero, and non-representable quotient (MIN / -1). */ if (b ==0 || (b ==-1 && a == (1ll <<63 ))) { -runtime_exception (env,PGM_FIXPT_DIVIDE ,GETPC ()); +program_interrupt_ra (env,PGM_FIXPT_DIVIDE ,ILEN_AUTO ,GETPC ()); } env->retxl = a % b;return a / b;
@@ -92,7 +92,7 @@ uint64_t HELPER(divu64)(CPUS390XState *env, uint64_t ah, uint64_t al,uint64_t ret;/* Signal divide by zero. */ if (b ==0 ) { -runtime_exception (env,PGM_FIXPT_DIVIDE ,GETPC ()); +program_interrupt_ra (env,PGM_FIXPT_DIVIDE ,ILEN_AUTO ,GETPC ()); }if (ah ==0 ) {/* 64 -> 64/64 case */
@@ -106,7 +106,7 @@ uint64_t HELPER(divu64)(CPUS390XState *env, uint64_t ah, uint64_t al, env->retxl = a % b; ret = q;if (ret != q) { -runtime_exception (env,PGM_FIXPT_DIVIDE ,GETPC ()); +program_interrupt_ra (env,PGM_FIXPT_DIVIDE ,ILEN_AUTO ,GETPC ()); }#else S390CPU *cpu =s390_env_get_cpu (env);
target/s390x/internal.h +0 -2
@@ -408,8 +408,6 @@ int mmu_translate_real(CPUS390XState *env, target_ulong raddr, int rw,/* misc_helper.c */ -void QEMU_NORETURN runtime_exception(CPUS390XState *env, int excp, -uintptr_t retaddr);int handle_diag_288 (CPUS390XState *env,uint64_t r1,uint64_t r3);void handle_diag_308 (CPUS390XState *env,uint64_t r1,uint64_t r3);
target/s390x/misc_helper.c +0 -16
@@ -45,22 +45,6 @@ #define HELPER_LOG(x...)#endif -/* Raise an exception dynamically from a helper function. */ -void QEMU_NORETURN runtime_exception(CPUS390XState *env, int excp, -uintptr_t retaddr) -{ -CPUState *cs =CPU (s390_env_get_cpu (env)); - - cs->exception_index =EXCP_PGM ; - env->int_pgm_code = excp; - env->int_pgm_ilen =ILEN_AUTO ; - -/* Use the (ultimate) callers address to find the insn that trapped. */ -cpu_restore_state (cs, retaddr); - -cpu_loop_exit (cs); -} -/* Raise an exception statically from a TB. */ void HELPER (exception )(CPUS390XState *env,uint32_t excp) {
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2.12 02/15] s390x/tcg: get rid of runtime_exception()
Quoted 9 lines · David Hildenbrand
Let's use
program_interrupt_ra()instead.Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/fpu_helper.c | 2 +- target/s390x/int_helper.c | 14 +++++++------- target/s390x/internal.h | 2 -- target/s390x/misc_helper.c | 16 ---------------- 4 files changed, 8 insertions(+), 26 deletions(-)
r~
PATCH 3/15 s390x/tcg: rip out dead tpi code
In reply to David HildenbrandIt is broken and not even wired up. We'll add a new handler soon, but that will live somewhere else.
--- hw/s390x/css.c | 6 ------ include/hw/s390x/css.h | 1 - target/s390x/internal.h | 1 - target/s390x/ioinst.c | 26 -------------------------- 4 files changed, 34 deletions(-)
hw/s390x/css.c +0 -6
@@ -1723,12 +1723,6 @@ void css_undo_stcrw(CRW *crw)QTAILQ_INSERT_HEAD (&channel_subsys.pending_crws, crw_cont, sibling); } -int css_do_tpi (IOIntCode *int_code,int lowcore) -{ -/* No pending interrupts for !KVM. */ -return 0 ; - } -int css_collect_chp_desc (int m,uint8_t cssid,uint8_t f_chpid,uint8_t l_chpid,int rfmt,void *buf) {
include/hw/s390x/css.h +0 -1
@@ -248,7 +248,6 @@ int css_do_tsch_get_irb(SubchDev *sch, IRB *irb, int *irb_len);void css_do_tsch_update_subch (SubchDev *sch);int css_do_stcrw (CRW *crw);void css_undo_stcrw (CRW *crw); -int css_do_tpi (IOIntCode *int_code,int lowcore);int css_collect_chp_desc (int m,uint8_t cssid,uint8_t f_chpid,uint8_t l_chpid,int rfmt,void *buf);void css_do_schm (uint8_t mbk,int update,int dct,uint64_t mbo);
target/s390x/internal.h +0 -1
@@ -388,7 +388,6 @@ void ioinst_handle_stcrw(S390CPU *cpu, uint32_t ipb);void ioinst_handle_stsch (S390CPU *cpu,uint64_t reg1,uint32_t ipb);int ioinst_handle_tsch (S390CPU *cpu,uint64_t reg1,uint32_t ipb);void ioinst_handle_chsc (S390CPU *cpu,uint32_t ipb); -int ioinst_handle_tpi (S390CPU *cpu,uint32_t ipb);void ioinst_handle_schm (S390CPU *cpu,uint64_t reg1,uint64_t reg2,uint32_t ipb);void ioinst_handle_rsch (S390CPU *cpu,uint64_t reg1);
target/s390x/ioinst.c +0 -26
@@ -647,32 +647,6 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb) } } -int ioinst_handle_tpi (S390CPU *cpu,uint32_t ipb) -{ -CPUS390XState *env = &cpu->env; -uint64_t addr; -int lowcore; -IOIntCode int_code; -hwaddr len; -int ret; -uint8_t ar; - -trace_ioinst ("tpi" ); - addr =decode_basedisp_s (env, ipb, &ar); -if (addr &3 ) { -program_interrupt (env,PGM_SPECIFICATION ,4 ); -return -EIO ; - } - - lowcore = addr ?0 :1 ; - len = lowcore ?8 /* two words */ :12 /* three words */ ; - ret =css_do_tpi (&int_code, lowcore); -if (ret ==1 ) { -s390_cpu_virt_mem_write (cpu, lowcore ?184 : addr, ar, &int_code, len); - } -return ret; -} -#define SCHM_REG1_RES (_reg) (_reg & 0x000000000ffffffc)#define SCHM_REG1_MBK (_reg) ((_reg & 0x00000000f0000000) >> 28)#define SCHM_REG1_UPD (_reg) ((_reg & 0x0000000000000002) >> 1)
Signature · 1 line
2.14.3
Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2.12 03/15] s390x/tcg: rip out dead tpi code
Quoted 10 lines · David Hildenbrand
It is broken and not even wired up. We'll add a new handler soon, but that will live somewhere else.
Signed-off-by David Hildenbrand <[email protected]>--- hw/s390x/css.c | 6 ------ include/hw/s390x/css.h | 1 - target/s390x/internal.h | 1 - target/s390x/ioinst.c | 26 -------------------------- 4 files changed, 34 deletions(-)
Re: [Qemu-devel] [PATCH v1 for-2.12 03/15] s390x/tcg: rip out dead tpi code
In reply to David HildenbrandQuoted 10 lines · David Hildenbrand
It is broken and not even wired up. We'll add a new handler soon, but that will live somewhere else.
Signed-off-by David Hildenbrand <[email protected]>--- hw/s390x/css.c | 6 ------ include/hw/s390x/css.h | 1 - target/s390x/internal.h | 1 - target/s390x/ioinst.c | 26 -------------------------- 4 files changed, 34 deletions(-)
r~
PATCH 4/15 s390x/ioinst: pass the retaddr to all IO instructions
In reply to David HildenbrandTCG needs the retaddr when injecting an interrupt. Let's just pass it along and use 0 for KVM. The value will be completely ignored for KVM.
Convert program_interrupt() to program_interrupt_ra() directly, making use of the passed address.
--- target/s390x/internal.h | 29 +++++++++++--------- target/s390x/ioinst.c | 67 +++++++++++++++++++++++----------------------- target/s390x/kvm.c | 26 +++++++++--------- target/s390x/misc_helper.c | 20 +++++++------- 4 files changed, 73 insertions(+), 69 deletions(-)
target/s390x/internal.h +16 -13
@@ -379,20 +379,23 @@ void cpu_inject_stop(S390CPU *cpu);/* ioinst.c */ -void ioinst_handle_xsch (S390CPU *cpu,uint64_t reg1); -void ioinst_handle_csch (S390CPU *cpu,uint64_t reg1); -void ioinst_handle_hsch (S390CPU *cpu,uint64_t reg1); -void ioinst_handle_msch (S390CPU *cpu,uint64_t reg1,uint32_t ipb); -void ioinst_handle_ssch (S390CPU *cpu,uint64_t reg1,uint32_t ipb); -void ioinst_handle_stcrw (S390CPU *cpu,uint32_t ipb); -void ioinst_handle_stsch (S390CPU *cpu,uint64_t reg1,uint32_t ipb); -int ioinst_handle_tsch (S390CPU *cpu,uint64_t reg1,uint32_t ipb); -void ioinst_handle_chsc (S390CPU *cpu,uint32_t ipb); +void ioinst_handle_xsch (S390CPU *cpu,uint64_t reg1,uintptr_t ra); +void ioinst_handle_csch (S390CPU *cpu,uint64_t reg1,uintptr_t ra); +void ioinst_handle_hsch (S390CPU *cpu,uint64_t reg1,uintptr_t ra); +void ioinst_handle_msch (S390CPU *cpu,uint64_t reg1,uint32_t ipb, +uintptr_t ra); +void ioinst_handle_ssch (S390CPU *cpu,uint64_t reg1,uint32_t ipb, +uintptr_t ra); +void ioinst_handle_stcrw (S390CPU *cpu,uint32_t ipb,uintptr_t ra); +void ioinst_handle_stsch (S390CPU *cpu,uint64_t reg1,uint32_t ipb, +uintptr_t ra); +int ioinst_handle_tsch (S390CPU *cpu,uint64_t reg1,uint32_t ipb,uintptr_t ra); +void ioinst_handle_chsc (S390CPU *cpu,uint32_t ipb,uintptr_t ra);void ioinst_handle_schm (S390CPU *cpu,uint64_t reg1,uint64_t reg2, -uint32_t ipb); -void ioinst_handle_rsch (S390CPU *cpu,uint64_t reg1); -void ioinst_handle_rchp (S390CPU *cpu,uint64_t reg1); -void ioinst_handle_sal (S390CPU *cpu,uint64_t reg1); +uint32_t ipb,uintptr_t ra); +void ioinst_handle_rsch (S390CPU *cpu,uint64_t reg1,uintptr_t ra); +void ioinst_handle_rchp (S390CPU *cpu,uint64_t reg1,uintptr_t ra); +void ioinst_handle_sal (S390CPU *cpu,uint64_t reg1,uintptr_t ra);/* mem_helper.c */
target/s390x/ioinst.c +34 -33
256 lines not on this page · raw message
target/s390x/kvm.c +13 -13
@@ -1124,32 +1124,32 @@ static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)switch (ipa1) {case PRIV_B2_XSCH : -ioinst_handle_xsch (cpu, env->regs[1 ]); +ioinst_handle_xsch (cpu, env->regs[1 ],0 );break ;case PRIV_B2_CSCH : -ioinst_handle_csch (cpu, env->regs[1 ]); +ioinst_handle_csch (cpu, env->regs[1 ],0 );break ;case PRIV_B2_HSCH : -ioinst_handle_hsch (cpu, env->regs[1 ]); +ioinst_handle_hsch (cpu, env->regs[1 ],0 );break ;case PRIV_B2_MSCH : -ioinst_handle_msch (cpu, env->regs[1 ], run->s390_sieic.ipb); +ioinst_handle_msch (cpu, env->regs[1 ], run->s390_sieic.ipb,0 );break ;case PRIV_B2_SSCH : -ioinst_handle_ssch (cpu, env->regs[1 ], run->s390_sieic.ipb); +ioinst_handle_ssch (cpu, env->regs[1 ], run->s390_sieic.ipb,0 );break ;case PRIV_B2_STCRW : -ioinst_handle_stcrw (cpu, run->s390_sieic.ipb); +ioinst_handle_stcrw (cpu, run->s390_sieic.ipb,0 );break ;case PRIV_B2_STSCH : -ioinst_handle_stsch (cpu, env->regs[1 ], run->s390_sieic.ipb); +ioinst_handle_stsch (cpu, env->regs[1 ], run->s390_sieic.ipb,0 );break ;case PRIV_B2_TSCH :/* We should only get tsch via KVM_EXIT_S390_TSCH. */ fprintf (stderr,"Spurious tsch intercept\n" );break ;case PRIV_B2_CHSC : -ioinst_handle_chsc (cpu, run->s390_sieic.ipb); +ioinst_handle_chsc (cpu, run->s390_sieic.ipb,0 );break ;case PRIV_B2_TPI :/* This should have been handled by kvm already. */
@@ -1157,19 +1157,19 @@ static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)break ;case PRIV_B2_SCHM :ioinst_handle_schm (cpu, env->regs[1 ], env->regs[2 ], - run->s390_sieic.ipb); + run->s390_sieic.ipb,0 );break ;case PRIV_B2_RSCH : -ioinst_handle_rsch (cpu, env->regs[1 ]); +ioinst_handle_rsch (cpu, env->regs[1 ],0 );break ;case PRIV_B2_RCHP : -ioinst_handle_rchp (cpu, env->regs[1 ]); +ioinst_handle_rchp (cpu, env->regs[1 ],0 );break ;case PRIV_B2_STCPS :/* We do not provide this instruction, it is suppressed. */ break ;case PRIV_B2_SAL : -ioinst_handle_sal (cpu, env->regs[1 ]); +ioinst_handle_sal (cpu, env->regs[1 ],0 );break ;case PRIV_B2_SIGA :/* Not provided, set CC = 3 for subchannel not operational */
@@ -1673,7 +1673,7 @@ static int handle_tsch(S390CPU *cpu)cpu_synchronize_state (cs); - ret =ioinst_handle_tsch (cpu, cpu->env.regs[1 ], run->s390_tsch.ipb); + ret =ioinst_handle_tsch (cpu, cpu->env.regs[1 ], run->s390_tsch.ipb,0 );if (ret <0 ) {/* * Failure.
target/s390x/misc_helper.c +10 -10
@@ -323,7 +323,7 @@ void HELPER(xsch)(CPUS390XState *env, uint64_t r1) {S390CPU *cpu =s390_env_get_cpu (env);qemu_mutex_lock_iothread (); -ioinst_handle_xsch (cpu, r1); +ioinst_handle_xsch (cpu, r1,GETPC ());qemu_mutex_unlock_iothread (); }
@@ -331,7 +331,7 @@ void HELPER(csch)(CPUS390XState *env, uint64_t r1) {S390CPU *cpu =s390_env_get_cpu (env);qemu_mutex_lock_iothread (); -ioinst_handle_csch (cpu, r1); +ioinst_handle_csch (cpu, r1,GETPC ());qemu_mutex_unlock_iothread (); }
@@ -339,7 +339,7 @@ void HELPER(hsch)(CPUS390XState *env, uint64_t r1) {S390CPU *cpu =s390_env_get_cpu (env);qemu_mutex_lock_iothread (); -ioinst_handle_hsch (cpu, r1); +ioinst_handle_hsch (cpu, r1,GETPC ());qemu_mutex_unlock_iothread (); }
@@ -347,7 +347,7 @@ void HELPER(msch)(CPUS390XState *env, uint64_t r1, uint64_t inst) {S390CPU *cpu =s390_env_get_cpu (env);qemu_mutex_lock_iothread (); -ioinst_handle_msch (cpu, r1, inst >>16 ); +ioinst_handle_msch (cpu, r1, inst >>16 ,GETPC ());qemu_mutex_unlock_iothread (); }
@@ -355,7 +355,7 @@ void HELPER(rchp)(CPUS390XState *env, uint64_t r1) {S390CPU *cpu =s390_env_get_cpu (env);qemu_mutex_lock_iothread (); -ioinst_handle_rchp (cpu, r1); +ioinst_handle_rchp (cpu, r1,GETPC ());qemu_mutex_unlock_iothread (); }
@@ -363,7 +363,7 @@ void HELPER(rsch)(CPUS390XState *env, uint64_t r1) {S390CPU *cpu =s390_env_get_cpu (env);qemu_mutex_lock_iothread (); -ioinst_handle_rsch (cpu, r1); +ioinst_handle_rsch (cpu, r1,GETPC ());qemu_mutex_unlock_iothread (); }
@@ -371,7 +371,7 @@ void HELPER(ssch)(CPUS390XState *env, uint64_t r1, uint64_t inst) {S390CPU *cpu =s390_env_get_cpu (env);qemu_mutex_lock_iothread (); -ioinst_handle_ssch (cpu, r1, inst >>16 ); +ioinst_handle_ssch (cpu, r1, inst >>16 ,GETPC ());qemu_mutex_unlock_iothread (); }
@@ -379,7 +379,7 @@ void HELPER(stsch)(CPUS390XState *env, uint64_t r1, uint64_t inst) {S390CPU *cpu =s390_env_get_cpu (env);qemu_mutex_lock_iothread (); -ioinst_handle_stsch (cpu, r1, inst >>16 ); +ioinst_handle_stsch (cpu, r1, inst >>16 ,GETPC ());qemu_mutex_unlock_iothread (); }
@@ -387,7 +387,7 @@ void HELPER(tsch)(CPUS390XState *env, uint64_t r1, uint64_t inst) {S390CPU *cpu =s390_env_get_cpu (env);qemu_mutex_lock_iothread (); -ioinst_handle_tsch (cpu, r1, inst >>16 ); +ioinst_handle_tsch (cpu, r1, inst >>16 ,GETPC ());qemu_mutex_unlock_iothread (); }
@@ -395,7 +395,7 @@ void HELPER(chsc)(CPUS390XState *env, uint64_t inst) {S390CPU *cpu =s390_env_get_cpu (env);qemu_mutex_lock_iothread (); -ioinst_handle_chsc (cpu, inst >>16 ); +ioinst_handle_chsc (cpu, inst >>16 ,GETPC ());qemu_mutex_unlock_iothread (); }#endif
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2.12 04/15] s390x/ioinst: pass the retaddr to all IO instructions
Quoted 2 lines · On Tue, 28 Nov 2017 21:33:14 +0100 David Hildenbrand <[email protected]> wrote:
TCG needs the retaddr when injecting an interrupt. Let's just pass it along and use 0 for KVM. The value will be completely ignored for KVM.
Can we get a #define for that? Just to make it clear at a glance that we're passing an ignored value and not some magic thing.
Quoted 11 lines
Convert
program_interrupt()toprogram_interrupt_ra()directly, making use of the passed address.Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/internal.h | 29 +++++++++++--------- target/s390x/ioinst.c | 67 +++++++++++++++++++++++----------------------- target/s390x/kvm.c | 26 +++++++++--------- target/s390x/misc_helper.c | 20 +++++++------- 4 files changed, 73 insertions(+), 69 deletions(-)
Seems sane.
Re: [Qemu-devel] [PATCH v1 for-2.12 04/15] s390x/ioinst: pass the retaddr to all IO instructions
Quoted 8 lines · Cornelia Huck
Quoted 2 lines · On Tue, 28 Nov 2017 21:33:14 +0100 David Hildenbrand <[email protected]> wrote:
TCG needs the retaddr when injecting an interrupt. Let's just pass it along and use 0 for KVM. The value will be completely ignored for KVM.
Can we get a #define for that? Just to make it clear at a glance that we're passing an ignored value and not some magic thing.
If you can come up with a good name, I can't :)
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v1 for-2.12 04/15] s390x/ioinst: pass the retaddr to all IO instructions
Quoted 11 lines · On Wed, 29 Nov 2017 16:14:40 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 8 lines · Cornelia Huck
Quoted 2 lines · On Tue, 28 Nov 2017 21:33:14 +0100 David Hildenbrand <[email protected]> wrote:
TCG needs the retaddr when injecting an interrupt. Let's just pass it along and use 0 for KVM. The value will be completely ignored for KVM.
Can we get a #define for that? Just to make it clear at a glance that we're passing an ignored value and not some magic thing.
If you can come up with a good name, I can't :)
KVM_FAKE_RAKVM_RA_UNUSEDKVM_RA_IGNORED
?
Re: [Qemu-devel] [PATCH v1 for-2.12 04/15] s390x/ioinst: pass the retaddr to all IO instructions
Quoted 13 lines · Cornelia Huck
Quoted 10 lines · On Wed, 29 Nov 2017 16:14:40 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 8 lines · Cornelia Huck
Quoted 2 lines · On Tue, 28 Nov 2017 21:33:14 +0100 David Hildenbrand <[email protected]> wrote:
TCG needs the retaddr when injecting an interrupt. Let's just pass it along and use 0 for KVM. The value will be completely ignored for KVM.
Can we get a #define for that? Just to make it clear at a glance that we're passing an ignored value and not some magic thing.
Quoted 8 lines
Quoted 1 line
If you can come up with a good name, I can't :)
KVM_FAKE_RAKVM_RA_UNUSEDKVM_RA_IGNORED?
KVM_RA ? (to keep lines short?)
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v1 for-2.12 04/15] s390x/ioinst: pass the retaddr to all IO instructions
Quoted 16 lines · On Wed, 29 Nov 2017 16:38:03 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 15 lines · Cornelia Huck
Quoted 11 lines · On Wed, 29 Nov 2017 16:14:40 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 8 lines · Cornelia Huck
Quoted 2 lines · On Tue, 28 Nov 2017 21:33:14 +0100 David Hildenbrand <[email protected]> wrote:
TCG needs the retaddr when injecting an interrupt. Let's just pass it along and use 0 for KVM. The value will be completely ignored for KVM.
Can we get a #define for that? Just to make it clear at a glance that we're passing an ignored value and not some magic thing.
If you can come up with a good name, I can't :)
Quoted 9 lines
Quoted 6 lines
KVM_FAKE_RAKVM_RA_UNUSEDKVM_RA_IGNORED?
KVM_RA? (to keep lines short?)
That sounds too much like a magic value IMO (and not something that is ignored).
Re: [Qemu-devel] [PATCH v1 for-2.12 04/15] s390x/ioinst: pass the retaddr to all IO instructions
Quoted 23 lines · Cornelia Huck
Quoted 20 lines · On Wed, 29 Nov 2017 16:38:03 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 19 lines · Cornelia Huck
Quoted 11 lines · On Wed, 29 Nov 2017 16:14:40 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 8 lines · Cornelia Huck
Quoted 2 lines · On Tue, 28 Nov 2017 21:33:14 +0100 David Hildenbrand <[email protected]> wrote:
TCG needs the retaddr when injecting an interrupt. Let's just pass it along and use 0 for KVM. The value will be completely ignored for KVM.
Can we get a #define for that? Just to make it clear at a glance that we're passing an ignored value and not some magic thing.
If you can come up with a good name, I can't :)
KVM_FAKE_RAKVM_RA_UNUSEDKVM_RA_IGNORED
Quoted 9 lines
Quoted 5 lines
Quoted 2 lines
?
KVM_RA? (to keep lines short?)That sounds too much like a magic value IMO (and not something that is ignored).
Passing 0 will actually also ignore it for TCG. So it is actually a magic value :) (just that more gets ignore din case of KVM ...)
RA_NONE ?
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v1 for-2.12 04/15] s390x/ioinst: pass the retaddr to all IO instructions
Quoted 30 lines · On Wed, 29 Nov 2017 16:45:21 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 29 lines · Cornelia Huck
Quoted 25 lines · On Wed, 29 Nov 2017 16:38:03 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 13 lines · Cornelia Huck
Quoted 10 lines · On Wed, 29 Nov 2017 16:14:40 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 8 lines · Cornelia Huck
Quoted 2 lines · On Tue, 28 Nov 2017 21:33:14 +0100 David Hildenbrand <[email protected]> wrote:
TCG needs the retaddr when injecting an interrupt. Let's just pass it along and use 0 for KVM. The value will be completely ignored for KVM.
Can we get a #define for that? Just to make it clear at a glance that we're passing an ignored value and not some magic thing.
Quoted 8 lines
Quoted 1 line
If you can come up with a good name, I can't :)
KVM_FAKE_RAKVM_RA_UNUSEDKVM_RA_IGNORED?
KVM_RA? (to keep lines short?)
Quoted 10 lines
Quoted 3 lines
That sounds too much like a magic value IMO (and not something that is ignored).
Passing 0 will actually also ignore it for TCG. So it is actually a magic value :) (just that more gets ignore din case of KVM ...)
RA_NONE?
RA_IGNORED?
Re: [Qemu-devel] [PATCH v1 for-2.12 04/15] s390x/ioinst: pass the retaddr to all IO instructions
Quoted 37 lines · Cornelia Huck
Quoted 34 lines · On Wed, 29 Nov 2017 16:45:21 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 32 lines · Cornelia Huck
Quoted 16 lines · On Wed, 29 Nov 2017 16:38:03 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 15 lines · Cornelia Huck
Quoted 11 lines · On Wed, 29 Nov 2017 16:14:40 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 8 lines · Cornelia Huck
Quoted 2 lines · On Tue, 28 Nov 2017 21:33:14 +0100 David Hildenbrand <[email protected]> wrote:
TCG needs the retaddr when injecting an interrupt. Let's just pass it along and use 0 for KVM. The value will be completely ignored for KVM.
Can we get a #define for that? Just to make it clear at a glance that we're passing an ignored value and not some magic thing.
If you can come up with a good name, I can't :)
Quoted 9 lines
Quoted 6 lines
KVM_FAKE_RAKVM_RA_UNUSEDKVM_RA_IGNORED?
KVM_RA? (to keep lines short?)That sounds too much like a magic value IMO (and not something that is ignored).
Quoted 9 lines
Quoted 6 lines
Passing 0 will actually also ignore it for TCG. So it is actually a magic value :) (just that more gets ignore din case of KVM ...)
RA_NONE?
RA_IGNORED?
jap, will use that.
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v1 for-2.12 04/15] s390x/ioinst: pass the retaddr to all IO instructions
In reply to David HildenbrandQuoted 6 lines · David Hildenbrand
TCG needs the retaddr when injecting an interrupt. Let's just pass it along and use 0 for KVM. The value will be completely ignored for KVM.
Convert
program_interrupt()toprogram_interrupt_ra()directly, making use of the passed address.
Quoted 7 lines
Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/internal.h | 29 +++++++++++--------- target/s390x/ioinst.c | 67 +++++++++++++++++++++++----------------------- target/s390x/kvm.c | 26 +++++++++--------- target/s390x/misc_helper.c | 20 +++++++------- 4 files changed, 73 insertions(+), 69 deletions(-)
... with or without RA_IGNORED. ;-)
r~
PATCH 5/15 s390x/pci: pass the retaddr to all PCI instructions
In reply to David HildenbrandOnce we wire up TCG, we will need the retaddr to correctly inject program interrupts. As we want to get rid of the function program_interrupt(), convert PCI code too.
For KVM, we can simply pass a 0.
Convert program_interrupt() to program_interrupt_ra() directly, making use of the passed address.
--- hw/s390x/s390-pci-inst.c | 83 +++++++++++++++++++++++++----------------------- hw/s390x/s390-pci-inst.h | 16 ++++++---- target/s390x/kvm.c | 14 ++++---- 3 files changed, 59 insertions(+), 54 deletions(-)
hw/s390x/s390-pci-inst.c +43 -40
@@ -142,7 +142,7 @@ out:return rc; } -int clp_service_call (S390CPU *cpu,uint8_t r2) +int clp_service_call (S390CPU *cpu,uint8_t r2,uintptr_t ra) {ClpReqHdr *reqh;ClpRspHdr *resh;
@@ -158,7 +158,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2)cpu_synchronize_state (CPU (cpu));if (env->psw.mask &PSW_MASK_PSTATE ) { -program_interrupt (env,PGM_PRIVILEGED ,4 ); +program_interrupt_ra (env,PGM_PRIVILEGED ,4 , ra);return 0 ; }
@@ -168,7 +168,7 @@ int clp_service_call(S390CPU *cpu, uint8_t r2) reqh = (ClpReqHdr *)buffer; req_len =lduw_p (&reqh->len);if (req_len <16 || req_len >8184 || (req_len %8 !=0 )) { -program_interrupt (env,PGM_OPERAND ,4 ); +program_interrupt_ra (env,PGM_OPERAND ,4 , ra);return 0 ; }
@@ -179,11 +179,11 @@ int clp_service_call(S390CPU *cpu, uint8_t r2) resh = (ClpRspHdr *)(buffer + req_len); res_len =lduw_p (&resh->len);if (res_len <8 || res_len >8176 || (res_len %8 !=0 )) { -program_interrupt (env,PGM_OPERAND ,4 ); +program_interrupt_ra (env,PGM_OPERAND ,4 , ra);return 0 ; }if ((req_len + res_len) >8192 ) { -program_interrupt (env,PGM_OPERAND ,4 ); +program_interrupt_ra (env,PGM_OPERAND ,4 , ra);return 0 ; }
@@ -314,7 +314,7 @@ out:return 0 ; } -int pcilg_service_call (S390CPU *cpu,uint8_t r1,uint8_t r2) +int pcilg_service_call (S390CPU *cpu,uint8_t r1,uint8_t r2,uintptr_t ra) {CPUS390XState *env = &cpu->env;S390PCIBusDevice *pbdev;
@@ -329,12 +329,12 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)cpu_synchronize_state (CPU (cpu));if (env->psw.mask &PSW_MASK_PSTATE ) { -program_interrupt (env,PGM_PRIVILEGED ,4 ); +program_interrupt_ra (env,PGM_PRIVILEGED ,4 , ra);return 0 ; }if (r2 &0x1 ) { -program_interrupt (env,PGM_SPECIFICATION ,4 ); +program_interrupt_ra (env,PGM_SPECIFICATION ,4 , ra);return 0 ; }
@@ -367,19 +367,19 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)if (pcias <6 ) {if ((8 - (offset &0x7 )) < len) { -program_interrupt (env,PGM_OPERAND ,4 ); +program_interrupt_ra (env,PGM_OPERAND ,4 , ra);return 0 ; } mr = pbdev->pdev->io_regions[pcias].memory; result =memory_region_dispatch_read (mr, offset, &data, len,MEMTXATTRS_UNSPECIFIED );if (result !=MEMTX_OK ) { -program_interrupt (env,PGM_OPERAND ,4 ); +program_interrupt_ra (env,PGM_OPERAND ,4 , ra);return 0 ; } }else if (pcias ==15 ) {if ((4 - (offset &0x3 )) < len) { -program_interrupt (env,PGM_OPERAND ,4 ); +program_interrupt_ra (env,PGM_OPERAND ,4 , ra);return 0 ; } data = pci_host_config_read_common(
@@ -398,7 +398,7 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) data =bswap64 (data);break ;default : -program_interrupt (env,PGM_OPERAND ,4 ); +program_interrupt_ra (env,PGM_OPERAND ,4 , ra);return 0 ; } }else {
@@ -425,7 +425,7 @@ static int trap_msix(S390PCIBusDevice *pbdev, uint64_t offset, uint8_t pcias) } } -int pcistg_service_call (S390CPU *cpu,uint8_t r1,uint8_t r2) +int pcistg_service_call (S390CPU *cpu,uint8_t r1,uint8_t r2,uintptr_t ra) {CPUS390XState *env = &cpu->env;uint64_t offset, data;
@@ -439,12 +439,12 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)cpu_synchronize_state (CPU (cpu));if (env->psw.mask &PSW_MASK_PSTATE ) { -program_interrupt (env,PGM_PRIVILEGED ,4 ); +program_interrupt_ra (env,PGM_PRIVILEGED ,4 , ra);return 0 ; }if (r2 &0x1 ) { -program_interrupt (env,PGM_SPECIFICATION ,4 ); +program_interrupt_ra (env,PGM_SPECIFICATION ,4 , ra);return 0 ; }
@@ -478,7 +478,7 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) data = env->regs[r1];if (pcias <6 ) {if ((8 - (offset &0x7 )) < len) { -program_interrupt (env,PGM_OPERAND ,4 ); +program_interrupt_ra (env,PGM_OPERAND ,4 , ra);return 0 ; }
@@ -492,12 +492,12 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) result =memory_region_dispatch_write (mr, offset, data, len,MEMTXATTRS_UNSPECIFIED );if (result !=MEMTX_OK ) { -program_interrupt (env,PGM_OPERAND ,4 ); +program_interrupt_ra (env,PGM_OPERAND ,4 , ra);return 0 ; } }else if (pcias ==15 ) {if ((4 - (offset &0x3 )) < len) { -program_interrupt (env,PGM_OPERAND ,4 ); +program_interrupt_ra (env,PGM_OPERAND ,4 , ra);return 0 ; }switch (len) {
@@ -513,7 +513,7 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) data =bswap64 (data);break ;default : -program_interrupt (env,PGM_OPERAND ,4 ); +program_interrupt_ra (env,PGM_OPERAND ,4 , ra);return 0 ; }
@@ -531,7 +531,7 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)return 0 ; } -int rpcit_service_call (S390CPU *cpu,uint8_t r1,uint8_t r2) +int rpcit_service_call (S390CPU *cpu,uint8_t r1,uint8_t r2,uintptr_t ra) {CPUS390XState *env = &cpu->env;uint32_t fh;
@@ -545,12 +545,12 @@ int rpcit_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)cpu_synchronize_state (CPU (cpu));if (env->psw.mask &PSW_MASK_PSTATE ) { -program_interrupt (env,PGM_PRIVILEGED ,4 ); +program_interrupt_ra (env,PGM_PRIVILEGED ,4 , ra); goto out; }if (r2 &0x1 ) { -program_interrupt (env,PGM_SPECIFICATION ,4 ); +program_interrupt_ra (env,PGM_SPECIFICATION ,4 , ra); goto out; }
@@ -624,7 +624,7 @@ out: }int pcistb_service_call (S390CPU *cpu,uint8_t r1,uint8_t r3,uint64_t gaddr, -uint8_t ar) +uint8_t ar,uintptr_t ra) {CPUS390XState *env = &cpu->env;S390PCIBusDevice *pbdev;
@@ -637,7 +637,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,uint8_t buffer[128 ];if (env->psw.mask &PSW_MASK_PSTATE ) { -program_interrupt (env,PGM_PRIVILEGED ,6 ); +program_interrupt_ra (env,PGM_PRIVILEGED ,6 , ra);return 0 ; }
@@ -659,7 +659,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,case 128 :break ;default : -program_interrupt (env,PGM_SPECIFICATION ,6 ); +program_interrupt_ra (env,PGM_SPECIFICATION ,6 , ra);return 0 ; }
@@ -687,7 +687,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr, mr = pbdev->pdev->io_regions[pcias].memory;if (!memory_region_access_valid (mr, env->regs[r3], len, true)) { -program_interrupt (env,PGM_OPERAND ,6 ); +program_interrupt_ra (env,PGM_OPERAND ,6 , ra);return 0 ; }
@@ -700,7 +700,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,ldq_p (buffer + i *8 ),8 , MEMTXATTRS_UNSPECIFIED);if (result !=MEMTX_OK ) { -program_interrupt (env,PGM_OPERAND ,6 ); +program_interrupt_ra (env,PGM_OPERAND ,6 , ra);return 0 ; } }
@@ -767,7 +767,8 @@ int pci_dereg_irqs(S390PCIBusDevice *pbdev)return 0 ; } -static int reg_ioat (CPUS390XState *env,S390PCIIOMMU *iommu,ZpciFib fib) +static int reg_ioat (CPUS390XState *env,S390PCIIOMMU *iommu,ZpciFib fib, +uintptr_t ra) {uint64_t pba =ldq_p (&fib.pba);uint64_t pal =ldq_p (&fib.pal);
@@ -776,14 +777,14 @@ static int reg_ioat(CPUS390XState *env, S390PCIIOMMU *iommu, ZpciFib fib)uint8_t t = (g_iota >>11 ) &0x1 ;if (pba > pal || pba <ZPCI_SDMA_ADDR || pal >ZPCI_EDMA_ADDR ) { -program_interrupt (env,PGM_OPERAND ,6 ); +program_interrupt_ra (env,PGM_OPERAND ,6 , ra);return -EINVAL ; }/* currently we only support designation type 1 with translation */ if (!(dt ==ZPCI_IOTA_RTTO && t)) {error_report ("unsupported ioat dt %d t %d" , dt, t); -program_interrupt (env,PGM_OPERAND ,6 ); +program_interrupt_ra (env,PGM_OPERAND ,6 , ra);return -EINVAL ; }
@@ -804,7 +805,8 @@ void pci_dereg_ioat(S390PCIIOMMU *iommu) iommu->g_iota =0 ; } -int mpcifc_service_call (S390CPU *cpu,uint8_t r1,uint64_t fiba,uint8_t ar) +int mpcifc_service_call (S390CPU *cpu,uint8_t r1,uint64_t fiba,uint8_t ar, +uintptr_t ra) {CPUS390XState *env = &cpu->env;uint8_t oc, dmaas;
@@ -814,7 +816,7 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar)uint64_t cc =ZPCI_PCI_LS_OK ;if (env->psw.mask &PSW_MASK_PSTATE ) { -program_interrupt (env,PGM_PRIVILEGED ,6 ); +program_interrupt_ra (env,PGM_PRIVILEGED ,6 , ra);return 0 ; }
@@ -823,7 +825,7 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar) fh = env->regs[r1] >>32 ;if (fiba &0x7 ) { -program_interrupt (env,PGM_SPECIFICATION ,6 ); +program_interrupt_ra (env,PGM_SPECIFICATION ,6 , ra);return 0 ; }
@@ -850,7 +852,7 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar) }if (fib.fmt !=0 ) { -program_interrupt (env,PGM_OPERAND ,6 ); +program_interrupt_ra (env,PGM_OPERAND ,6 , ra);return 0 ; }
@@ -879,7 +881,7 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar) }else if (pbdev->iommu->enabled) { cc =ZPCI_PCI_LS_ERR ;s390_set_status_code (env, r1,ZPCI_MOD_ST_SEQUENCE ); - }else if (reg_ioat (env, pbdev->iommu, fib)) { + }else if (reg_ioat (env, pbdev->iommu, fib, ra)) { cc =ZPCI_PCI_LS_ERR ;s390_set_status_code (env, r1,ZPCI_MOD_ST_INSUF_RES ); }
@@ -904,7 +906,7 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar)s390_set_status_code (env, r1,ZPCI_MOD_ST_SEQUENCE ); }else {pci_dereg_ioat (pbdev->iommu); -if (reg_ioat (env, pbdev->iommu, fib)) { +if (reg_ioat (env, pbdev->iommu, fib, ra)) { cc =ZPCI_PCI_LS_ERR ;s390_set_status_code (env, r1,ZPCI_MOD_ST_INSUF_RES ); }
@@ -935,7 +937,7 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar) pbdev->fmb_addr =ldq_p (&fib.fmb_addr);break ;default : -program_interrupt (&cpu->env,PGM_OPERAND ,6 ); +program_interrupt_ra (&cpu->env,PGM_OPERAND ,6 , ra); cc =ZPCI_PCI_LS_ERR ; }
@@ -943,7 +945,8 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar)return 0 ; } -int stpcifc_service_call (S390CPU *cpu,uint8_t r1,uint64_t fiba,uint8_t ar) +int stpcifc_service_call (S390CPU *cpu,uint8_t r1,uint64_t fiba,uint8_t ar, +uintptr_t ra) {CPUS390XState *env = &cpu->env;uint8_t dmaas;
@@ -954,7 +957,7 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar)uint64_t cc =ZPCI_PCI_LS_OK ;if (env->psw.mask &PSW_MASK_PSTATE ) { -program_interrupt (env,PGM_PRIVILEGED ,6 ); +program_interrupt_ra (env,PGM_PRIVILEGED ,6 , ra);return 0 ; }
@@ -968,7 +971,7 @@ int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba, uint8_t ar) }if (fiba &0x7 ) { -program_interrupt (env,PGM_SPECIFICATION ,6 ); +program_interrupt_ra (env,PGM_SPECIFICATION ,6 , ra);return 0 ; }
hw/s390x/s390-pci-inst.h +9 -7
@@ -293,13 +293,15 @@ typedef struct ZpciFib {int pci_dereg_irqs (S390PCIBusDevice *pbdev);void pci_dereg_ioat (S390PCIIOMMU *iommu); -int clp_service_call (S390CPU *cpu,uint8_t r2); -int pcilg_service_call (S390CPU *cpu,uint8_t r1,uint8_t r2); -int pcistg_service_call (S390CPU *cpu,uint8_t r1,uint8_t r2); -int rpcit_service_call (S390CPU *cpu,uint8_t r1,uint8_t r2); +int clp_service_call (S390CPU *cpu,uint8_t r2,uintptr_t ra); +int pcilg_service_call (S390CPU *cpu,uint8_t r1,uint8_t r2,uintptr_t ra); +int pcistg_service_call (S390CPU *cpu,uint8_t r1,uint8_t r2,uintptr_t ra); +int rpcit_service_call (S390CPU *cpu,uint8_t r1,uint8_t r2,uintptr_t ra);int pcistb_service_call (S390CPU *cpu,uint8_t r1,uint8_t r3,uint64_t gaddr, -uint8_t ar); -int mpcifc_service_call (S390CPU *cpu,uint8_t r1,uint64_t fiba,uint8_t ar); -int stpcifc_service_call (S390CPU *cpu,uint8_t r1,uint64_t fiba,uint8_t ar); +uint8_t ar,uintptr_t ra); +int mpcifc_service_call (S390CPU *cpu,uint8_t r1,uint64_t fiba,uint8_t ar, +uintptr_t ra); +int stpcifc_service_call (S390CPU *cpu,uint8_t r1,uint64_t fiba,uint8_t ar, +uintptr_t ra);#endif
target/s390x/kvm.c +7 -7
@@ -1230,7 +1230,7 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run)uint8_t r2 = (run->s390_sieic.ipb &0x000f0000 ) >>16 ;if (s390_has_feat (S390_FEAT_ZPCI )) { -return clp_service_call (cpu, r2); +return clp_service_call (cpu, r2,0 ); }else {return -1 ; }
@@ -1242,7 +1242,7 @@ static int kvm_pcilg_service_call(S390CPU *cpu, struct kvm_run *run)uint8_t r2 = (run->s390_sieic.ipb &0x000f0000 ) >>16 ;if (s390_has_feat (S390_FEAT_ZPCI )) { -return pcilg_service_call (cpu, r1, r2); +return pcilg_service_call (cpu, r1, r2,0 ); }else {return -1 ; }
@@ -1254,7 +1254,7 @@ static int kvm_pcistg_service_call(S390CPU *cpu, struct kvm_run *run)uint8_t r2 = (run->s390_sieic.ipb &0x000f0000 ) >>16 ;if (s390_has_feat (S390_FEAT_ZPCI )) { -return pcistg_service_call (cpu, r1, r2); +return pcistg_service_call (cpu, r1, r2,0 ); }else {return -1 ; }
@@ -1270,7 +1270,7 @@ static int kvm_stpcifc_service_call(S390CPU *cpu, struct kvm_run *run)cpu_synchronize_state (CPU (cpu)); fiba =get_base_disp_rxy (cpu, run, &ar); -return stpcifc_service_call (cpu, r1, fiba, ar); +return stpcifc_service_call (cpu, r1, fiba, ar,0 ); }else {return -1 ; }
@@ -1302,7 +1302,7 @@ static int kvm_rpcit_service_call(S390CPU *cpu, struct kvm_run *run)uint8_t r2 = (run->s390_sieic.ipb &0x000f0000 ) >>16 ;if (s390_has_feat (S390_FEAT_ZPCI )) { -return rpcit_service_call (cpu, r1, r2); +return rpcit_service_call (cpu, r1, r2,0 ); }else {return -1 ; }
@@ -1319,7 +1319,7 @@ static int kvm_pcistb_service_call(S390CPU *cpu, struct kvm_run *run)cpu_synchronize_state (CPU (cpu)); gaddr =get_base_disp_rsy (cpu, run, &ar); -return pcistb_service_call (cpu, r1, r3, gaddr, ar); +return pcistb_service_call (cpu, r1, r3, gaddr, ar,0 ); }else {return -1 ; }
@@ -1335,7 +1335,7 @@ static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run)cpu_synchronize_state (CPU (cpu)); fiba =get_base_disp_rxy (cpu, run, &ar); -return mpcifc_service_call (cpu, r1, fiba, ar); +return mpcifc_service_call (cpu, r1, fiba, ar,0 ); }else {return -1 ; }
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2.12 05/15] s390x/pci: pass the retaddr to all PCI instructions
Quoted 9 lines · David Hildenbrand
Once we wire up TCG, we will need the retaddr to correctly inject program interrupts. As we want to get rid of the function
program_interrupt(), convert PCI code too.For KVM, we can simply pass a 0.
Convert
program_interrupt()toprogram_interrupt_ra()directly, making use of the passed address.
Quoted 6 lines
Signed-off-by David Hildenbrand <[email protected]>--- hw/s390x/s390-pci-inst.c | 83 +++++++++++++++++++++++++----------------------- hw/s390x/s390-pci-inst.h | 16 ++++++---- target/s390x/kvm.c | 14 ++++---- 3 files changed, 59 insertions(+), 54 deletions(-)
with or without RA_IGNORED.
r~
PATCH 6/15 s390x/diag: pass the retaddr into handle_diag_308()
In reply to David HildenbrandNeeded to later drop potential_page_fault() from the diag TCG translate function.
Convert program_interrupt() to program_interrupt_ra() directly, making use of the passed address.
--- target/s390x/diag.c | 14 +++++++------- target/s390x/internal.h | 3 ++- target/s390x/kvm.c | 2 +- target/s390x/misc_helper.c | 2 +- 4 files changed, 11 insertions(+), 10 deletions(-)
target/s390x/diag.c +7 -7
@@ -99,19 +99,19 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)#define DIAG_308_RC_NO_CONF 0x0102#define DIAG_308_RC_INVALID 0x0402 -void handle_diag_308 (CPUS390XState *env,uint64_t r1,uint64_t r3) +void handle_diag_308 (CPUS390XState *env,uint64_t r1,uint64_t r3,uintptr_t ra) {uint64_t addr = env->regs[r1];uint64_t subcode = env->regs[r3];IplParameterBlock *iplb;if (env->psw.mask &PSW_MASK_PSTATE ) { -program_interrupt (env,PGM_PRIVILEGED ,ILEN_AUTO ); +program_interrupt_ra (env,PGM_PRIVILEGED ,ILEN_AUTO , ra);return ; }if ((subcode & ~0x0ffffULL ) || (subcode >6 )) { -program_interrupt (env,PGM_SPECIFICATION ,ILEN_AUTO ); +program_interrupt_ra (env,PGM_SPECIFICATION ,ILEN_AUTO , ra);return ; }
@@ -136,12 +136,12 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3)break ;case 5 :if ((r1 &1 ) || (addr &0x0fffULL )) { -program_interrupt (env,PGM_SPECIFICATION ,ILEN_AUTO ); +program_interrupt_ra (env,PGM_SPECIFICATION ,ILEN_AUTO , ra);return ; }if (!address_space_access_valid (&address_space_memory, addr,sizeof (IplParameterBlock), false)) { -program_interrupt (env,PGM_ADDRESSING ,ILEN_AUTO ); +program_interrupt_ra (env,PGM_ADDRESSING ,ILEN_AUTO , ra);return ; } iplb =g_new0 (IplParameterBlock,1 );
@@ -165,12 +165,12 @@ out:return ;case 6 :if ((r1 &1 ) || (addr &0x0fffULL )) { -program_interrupt (env,PGM_SPECIFICATION ,ILEN_AUTO ); +program_interrupt_ra (env,PGM_SPECIFICATION ,ILEN_AUTO , ra);return ; }if (!address_space_access_valid (&address_space_memory, addr,sizeof (IplParameterBlock), true)) { -program_interrupt (env,PGM_ADDRESSING ,ILEN_AUTO ); +program_interrupt_ra (env,PGM_ADDRESSING ,ILEN_AUTO , ra);return ; } iplb =s390_ipl_get_iplb ();
target/s390x/internal.h +2 -1
@@ -411,7 +411,8 @@ int mmu_translate_real(CPUS390XState *env, target_ulong raddr, int rw,/* misc_helper.c */ int handle_diag_288 (CPUS390XState *env,uint64_t r1,uint64_t r3); -void handle_diag_308 (CPUS390XState *env,uint64_t r1,uint64_t r3); +void handle_diag_308 (CPUS390XState *env,uint64_t r1,uint64_t r3, +uintptr_t ra);/* translate.c */
target/s390x/kvm.c +1 -1
@@ -1451,7 +1451,7 @@ static void kvm_handle_diag_308(S390CPU *cpu, struct kvm_run *run)cpu_synchronize_state (CPU (cpu)); r1 = (run->s390_sieic.ipa &0x00f0 ) >>4 ; r3 = run->s390_sieic.ipa &0x000f ; -handle_diag_308 (&cpu->env, r1, r3); +handle_diag_308 (&cpu->env, r1, r3,0 ); }static int handle_sw_breakpoint (S390CPU *cpu,struct kvm_run *run)
target/s390x/misc_helper.c +1 -1
@@ -88,7 +88,7 @@ void HELPER(diag)(CPUS390XState *env, uint32_t r1, uint32_t r3, uint32_t num)case 0x308 :/* ipl */ qemu_mutex_lock_iothread (); -handle_diag_308 (env, r1, r3); +handle_diag_308 (env, r1, r3,GETPC ());qemu_mutex_unlock_iothread (); r =0 ;break ;
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2.12 06/15] s390x/diag: pass the retaddr into handle_diag_308()
Quoted 6 lines · David Hildenbrand
Needed to later drop
potential_page_fault()from the diag TCG translate function.Convert
program_interrupt()toprogram_interrupt_ra()directly, making use of the passed address.
Quoted 7 lines
Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/diag.c | 14 +++++++------- target/s390x/internal.h | 3 ++- target/s390x/kvm.c | 2 +- target/s390x/misc_helper.c | 2 +- 4 files changed, 11 insertions(+), 10 deletions(-)
Likewise wrt RA_IGNORED.
r~
PATCH 7/15 s390x: handle exceptions during s390_cpu_virt_mem_rw() correctly (TCG)
In reply to David Hildenbrands390_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.
--- 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(-)
hw/s390x/s390-pci-inst.c +7 -0
@@ -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 ; }
target/s390x/cpu.h +1 -0
@@ -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 */
target/s390x/ioinst.c +18 -3
@@ -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)
target/s390x/mmu_helper.c +18 -1
@@ -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
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2.12 07/15] s390x: handle exceptions during s390_cpu_virt_mem_rw() correctly (TCG)
Quoted 26 lines · On Tue, 28 Nov 2017 21:33:17 +0100 David Hildenbrand <[email protected]> wrote:
s390_cpu_virt_mem_rw()must always return, so callers can react on an exception (e.g. seeioinst_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 <[email protected]>--- 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(-)target/s390x/ioinst.c +0
-0
target/s390x/ioinst.c void ioinst_handle_stsch(S390CPU *cpu, uint64_t reg1, uint32_t ipb, · line 252
Quoted 5 lines
target/s390x/ioinst.c +2
-0@@ -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.
Quoted 5 lines
target/s390x/ioinst.c +0
-0}return ; }
target/s390x/ioinst.c void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra) · line 665
Quoted 9 lines
target/s390x/ioinst.c +3
-0@@ -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
Quoted 16 lines
target/s390x/ioinst.c +0
-0#define SCHM_REG1_RES (_reg) (_reg & 0x000000000ffffffc)#define SCHM_REG1_MBK (_reg) ((_reg & 0x00000000f0000000) >> 28)#define SCHM_REG1_UPD (_reg) ((_reg & 0x0000000000000002) >> 1)target/s390x/mmu_helper.c +1
-0@@ -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"
target/s390x/mmu_helper.c static int translate_pages(S390CPU *cpu, vaddr addr, int nr_pages, · line 462
Quoted 5 lines
target/s390x/mmu_helper.c +1
-1@@ -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?
Quoted 14 lines
target/s390x/mmu_helper.c +3
-0return -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)
target/s390x/mmu_helper.c int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf, · line 524
Quoted 7 lines
target/s390x/mmu_helper.c +4
-0@@ -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.
Quoted 12 lines
target/s390x/mmu_helper.c +9
-0+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
Re: [Qemu-devel] [PATCH v1 for-2.12 07/15] s390x: handle exceptions during s390_cpu_virt_mem_rw() correctly (TCG)
Quoted 16 lines
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.
Quoted 13 lines
} return; }target/s390x/ioinst.c +2
-0@@ -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); } }
Quoted 3 lines
Review void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, uintptr_t ra) · line 665
Quoted 1 line
(quoted diff) +1
-0+unrelated whitespace
ack, will drop.
Quoted 16 lines
Quoted 16 lines
target/s390x/ioinst.c +0
-0#define SCHM_REG1_RES (_reg) (_reg & 0x000000000ffffffc)#define SCHM_REG1_MBK (_reg) ((_reg & 0x00000000f0000000) >> 28)#define SCHM_REG1_UPD (_reg) ((_reg & 0x0000000000000002) >> 1)target/s390x/mmu_helper.c +1
-0@@ -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"
Quoted 8 lines
target/s390x/mmu_helper.c static int translate_pages(S390CPU *cpu, vaddr addr, int nr_pages, · line 462
Quoted 6 lines
target/s390x/mmu_helper.c +1
-1@@ -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 :)
Quoted 18 lines
Quoted 18 lines
target/s390x/mmu_helper.c +3
-0return -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; }
Quoted 6 lines
target/s390x/mmu_helper.c int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf, · line 524
Quoted 4 lines
target/s390x/mmu_helper.c +4
-0+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.
Quoted 14 lines
Quoted 13 lines
target/s390x/mmu_helper.c +9
-0+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
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v1 for-2.12 07/15] s390x: handle exceptions during s390_cpu_virt_mem_rw() correctly (TCG)
Quoted 21 lines · On Wed, 29 Nov 2017 16:54:30 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 12 lines
Quoted 12 lines
target/s390x/mmu_helper.c +1
-0@@ -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" Quoted 8 lines
target/s390x/mmu_helper.c static int translate_pages(S390CPU *cpu, vaddr addr, int nr_pages, · line 462
Quoted 6 lines
target/s390x/mmu_helper.c +1
-1@@ -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?
Quoted 4 lines
"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 :)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
target/s390x/mmu_helper.c +0
-0return -EFAULT ; } addr +=TARGET_PAGE_SIZE ;
Re: [Qemu-devel] [PATCH v1 for-2.12 07/15] s390x: handle exceptions during s390_cpu_virt_mem_rw() correctly (TCG)
Quoted 25 lines · Cornelia Huck
Quoted 21 lines · On Wed, 29 Nov 2017 16:54:30 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 20 lines
Quoted 13 lines
target/s390x/mmu_helper.c +1
-0@@ -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" target/s390x/mmu_helper.c static int translate_pages(S390CPU *cpu, vaddr addr, int nr_pages, · line 462
Quoted 5 lines
target/s390x/mmu_helper.c +1
-1@@ -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?
Quoted 6 lines
Quoted 4 lines
"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!
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v1 for-2.12 07/15] s390x: handle exceptions during s390_cpu_virt_mem_rw() correctly (TCG)
In reply to David Hildenbrandtarget/s390x/mmu_helper.c int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf, · line 530
Quoted 10 lines · David Hildenbrand
target/s390x/mmu_helper.c +10
-0+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~
PATCH 8/15 s390x/tcg: io instructions don't need potential_page_fault()
In reply to David HildenbrandAs we handle the retaddr in all cases properly now, we can drop it.
--- target/s390x/translate.c | 10 ---------- 1 file changed, 10 deletions(-)
target/s390x/translate.c +0 -10
@@ -4008,7 +4008,6 @@ static ExitStatus op_spx(DisasContext *s, DisasOps *o)static ExitStatus op_xsch (DisasContext *s,DisasOps *o) {check_privileged (s); -potential_page_fault (s);gen_helper_xsch (cpu_env, regs[1 ]);set_cc_static (s);return NO_EXIT ;
@@ -4017,7 +4016,6 @@ static ExitStatus op_xsch(DisasContext *s, DisasOps *o)static ExitStatus op_csch (DisasContext *s,DisasOps *o) {check_privileged (s); -potential_page_fault (s);gen_helper_csch (cpu_env, regs[1 ]);set_cc_static (s);return NO_EXIT ;
@@ -4026,7 +4024,6 @@ static ExitStatus op_csch(DisasContext *s, DisasOps *o)static ExitStatus op_hsch (DisasContext *s,DisasOps *o) {check_privileged (s); -potential_page_fault (s);gen_helper_hsch (cpu_env, regs[1 ]);set_cc_static (s);return NO_EXIT ;
@@ -4035,7 +4032,6 @@ static ExitStatus op_hsch(DisasContext *s, DisasOps *o)static ExitStatus op_msch (DisasContext *s,DisasOps *o) {check_privileged (s); -potential_page_fault (s);gen_helper_msch (cpu_env, regs[1 ], o->in2);set_cc_static (s);return NO_EXIT ;
@@ -4044,7 +4040,6 @@ static ExitStatus op_msch(DisasContext *s, DisasOps *o)static ExitStatus op_rchp (DisasContext *s,DisasOps *o) {check_privileged (s); -potential_page_fault (s);gen_helper_rchp (cpu_env, regs[1 ]);set_cc_static (s);return NO_EXIT ;
@@ -4053,7 +4048,6 @@ static ExitStatus op_rchp(DisasContext *s, DisasOps *o)static ExitStatus op_rsch (DisasContext *s,DisasOps *o) {check_privileged (s); -potential_page_fault (s);gen_helper_rsch (cpu_env, regs[1 ]);set_cc_static (s);return NO_EXIT ;
@@ -4062,7 +4056,6 @@ static ExitStatus op_rsch(DisasContext *s, DisasOps *o)static ExitStatus op_ssch (DisasContext *s,DisasOps *o) {check_privileged (s); -potential_page_fault (s);gen_helper_ssch (cpu_env, regs[1 ], o->in2);set_cc_static (s);return NO_EXIT ;
@@ -4071,7 +4064,6 @@ static ExitStatus op_ssch(DisasContext *s, DisasOps *o)static ExitStatus op_stsch (DisasContext *s,DisasOps *o) {check_privileged (s); -potential_page_fault (s);gen_helper_stsch (cpu_env, regs[1 ], o->in2);set_cc_static (s);return NO_EXIT ;
@@ -4080,7 +4072,6 @@ static ExitStatus op_stsch(DisasContext *s, DisasOps *o)static ExitStatus op_tsch (DisasContext *s,DisasOps *o) {check_privileged (s); -potential_page_fault (s);gen_helper_tsch (cpu_env, regs[1 ], o->in2);set_cc_static (s);return NO_EXIT ;
@@ -4089,7 +4080,6 @@ static ExitStatus op_tsch(DisasContext *s, DisasOps *o)static ExitStatus op_chsc (DisasContext *s,DisasOps *o) {check_privileged (s); -potential_page_fault (s);gen_helper_chsc (cpu_env, o->in2);set_cc_static (s);return NO_EXIT ;
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2.12 08/15] s390x/tcg: io instructions don't need potential_page_fault()
Quoted 6 lines · David Hildenbrand
As we handle the retaddr in all cases properly now, we can drop it.
Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/translate.c | 10 ---------- 1 file changed, 10 deletions(-)
r~
PATCH 9/15 s390x/tcg: use program_interrupt_ra() in SCLP Service Call
In reply to David HildenbrandNow we can drop potential_page_fault().
--- target/s390x/misc_helper.c | 3 +-- target/s390x/translate.c | 1 - 2 files changed, 1 insertion(+), 3 deletions(-)
target/s390x/misc_helper.c +1 -2
@@ -63,8 +63,7 @@ uint32_t HELPER(servc)(CPUS390XState *env, uint64_t r1, uint64_t r2)qemu_mutex_lock_iothread ();int r =sclp_service_call (env, r1, r2);if (r <0 ) { -program_interrupt (env, -r,4 ); - r =0 ; +program_interrupt_ra (env, -r,4 ,GETPC ()); }qemu_mutex_unlock_iothread ();return r;
target/s390x/translate.c +0 -1
@@ -3704,7 +3704,6 @@ static ExitStatus op_sqxb(DisasContext *s, DisasOps *o)static ExitStatus op_servc (DisasContext *s,DisasOps *o) {check_privileged (s); -potential_page_fault (s);gen_helper_servc (cc_op, cpu_env, o->in2, o->in1);set_cc_static (s);return NO_EXIT ;
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2.12 09/15] s390x/tcg: use program_interrupt_ra() in SCLP Service Call
target/s390x/misc_helper.c uint32_t HELPER(servc)(CPUS390XState *env, uint64_t r1, uint64_t r2) · line 69
Quoted 9 lines · David Hildenbrand
qemu_mutex_lock_iothread(); int r = sclp_service_call(env, r1, r2); if (r < 0) {target/s390x/misc_helper.c +1
-2-program_interrupt (env, -r,4 ); - r =0 ; +program_interrupt_ra (env, -r,4 ,GETPC ()); }qemu_mutex_unlock_iothread ();return r;
Incidentally, shouldn't the unlock happen before throwing the exception? We will unlock after a longjmp, but it does look odd to write it this way.
r~
Re: [Qemu-devel] [PATCH v1 for-2.12 09/15] s390x/tcg: use program_interrupt_ra() in SCLP Service Call
Quoted 11 lines · Richard Henderson
target/s390x/misc_helper.c uint32_t HELPER(servc)(CPUS390XState *env, uint64_t r1, uint64_t r2) · line 69
Quoted 9 lines · David Hildenbrand
qemu_mutex_lock_iothread(); int r = sclp_service_call(env, r1, r2); if (r < 0) {target/s390x/misc_helper.c +1
-2-program_interrupt (env, -r,4 ); - r =0 ; +program_interrupt_ra (env, -r,4 ,GETPC ()); }qemu_mutex_unlock_iothread ();return r;
Quoted 3 lines
Incidentally, shouldn't the unlock happen before throwing the exception? We will unlock after a longjmp, but it does look odd to write it this way.
Yes, but we have that all over the place, no? The unlock here could be moved up.
Quoted 3 lines
r~
Signature · 3 lines
Thanks, David / dhildenb
PATCH 10/15 s390x/tcg: use program_interrupt_ra() in DIAG
In reply to David HildenbrandNow we can drop the two save statements in the translate function.
--- target/s390x/misc_helper.c | 2 +- target/s390x/translate.c | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-)
target/s390x/misc_helper.c +1 -1
@@ -101,7 +101,7 @@ void HELPER(diag)(CPUS390XState *env, uint32_t r1, uint32_t r3, uint32_t num) }if (r) { -program_interrupt (env,PGM_SPECIFICATION ,ILEN_AUTO ); +program_interrupt_ra (env,PGM_SPECIFICATION ,ILEN_AUTO ,GETPC ()); } }
target/s390x/translate.c +0 -3
@@ -2124,9 +2124,6 @@ static ExitStatus op_diag(DisasContext *s, DisasOps *o)TCGv_i32 func_code =tcg_const_i32 (get_field (s->fields, i2));check_privileged (s); -update_psw_addr (s); -gen_op_calc_cc (s); -gen_helper_diag (cpu_env, r1, r3, func_code);tcg_temp_free_i32 (func_code);
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2.12 10/15] s390x/tcg: use program_interrupt_ra() in DIAG
Quoted 7 lines · David Hildenbrand
Now we can drop the two save statements in the translate function.
Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/misc_helper.c | 2 +- target/s390x/translate.c | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-)
r~
PATCH 11/15 s390x/tcg: use program_interrupt_ra() in per_check_exception()
In reply to David HildenbrandWe can now drop updating the cc.
--- target/s390x/misc_helper.c | 2 +- target/s390x/translate.c | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-)
target/s390x/misc_helper.c +1 -1
@@ -412,7 +412,7 @@ void HELPER(per_check_exception)(CPUS390XState *env)* of EXECUTE, while per_address contains the target of EXECUTE. */ ilen =get_ilen (cpu_ldub_code (env, env->per_address)); -program_interrupt (env,PGM_PER , ilen); +program_interrupt_ra (env,PGM_PER , ilen,GETPC ()); } }
target/s390x/translate.c +0 -3
@@ -5837,9 +5837,6 @@ static ExitStatus translate_one(CPUS390XState *env, DisasContext *s)tcg_gen_movi_i64 (psw_addr, s->next_pc); } -/* Save off cc. */ -update_cc_op (s); -/* Call the helper to check for a possible PER exception. */ gen_helper_per_check_exception (cpu_env); }
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2.12 11/15] s390x/tcg: use program_interrupt_ra() in per_check_exception()
Quoted 7 lines · David Hildenbrand
We can now drop updating the cc.
Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/misc_helper.c | 2 +- target/s390x/translate.c | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-)
r~
PATCH 12/15 s390x/tcg: use program_interrupt_ra() in SACF
In reply to David HildenbrandConvert this user, too.
--- target/s390x/cc_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
target/s390x/cc_helper.c +1 -1
@@ -564,7 +564,7 @@ void HELPER(sacf)(CPUS390XState *env, uint64_t a1)break ;default :HELPER_LOG ("unknown sacf mode: %" PRIx64"\n" , a1); -program_interrupt (env,PGM_SPECIFICATION ,2 ); +program_interrupt_ra (env,PGM_SPECIFICATION ,2 ,GETPC ());break ; } }
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2.12 12/15] s390x/tcg: use program_interrupt_ra() in SACF
Quoted 6 lines · David Hildenbrand
Convert this user, too.
Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/cc_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
r~
PATCH 13/15 s390x/tcg: use program_interrupt_ra() in STSI
In reply to David HildenbrandSTSI needs some more love, but let's do one step at a time. We can now drop potential_page_fault().
--- target/s390x/misc_helper.c | 2 +- target/s390x/translate.c | 1 - 2 files changed, 1 insertion(+), 2 deletions(-)
target/s390x/misc_helper.c +1 -1
@@ -184,7 +184,7 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0,if ((r0 &STSI_LEVEL_MASK ) <=STSI_LEVEL_3 && ((r0 &STSI_R0_RESERVED_MASK ) || (r1 &STSI_R1_RESERVED_MASK ))) {/* valid function code, invalid reserved bits */ -program_interrupt (env,PGM_SPECIFICATION ,4 ); +program_interrupt_ra (env,PGM_SPECIFICATION ,4 ,GETPC ()); } sel1 = r0 &STSI_R0_SEL1_MASK ;
target/s390x/translate.c +0 -1
@@ -3988,7 +3988,6 @@ static ExitStatus op_stpt(DisasContext *s, DisasOps *o)static ExitStatus op_stsi (DisasContext *s,DisasOps *o) {check_privileged (s); -potential_page_fault (s);gen_helper_stsi (cc_op, cpu_env, o->in2, regs[0 ], regs[1 ]);set_cc_static (s);return NO_EXIT ;
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2.12 13/15] s390x/tcg: use program_interrupt_ra() in STSI
Quoted 1 line · On Tue, 28 Nov 2017 21:33:23 +0100 David Hildenbrand <[email protected]> wrote:
STSI needs some more love, but let's do one step at a time.
Out of curiousity: What else do you want to do?
Quoted 7 lines
We can now drop
potential_page_fault().Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/misc_helper.c | 2 +- target/s390x/translate.c | 1 - 2 files changed, 1 insertion(+), 2 deletions(-)
Re: [Qemu-devel] [PATCH v1 for-2.12 13/15] s390x/tcg: use program_interrupt_ra() in STSI
Quoted 7 lines · Cornelia Huck
Quoted 1 line · On Tue, 28 Nov 2017 21:33:23 +0100 David Hildenbrand <[email protected]> wrote:
STSI needs some more love, but let's do one step at a time.
Out of curiousity: What else do you want to do?
Will post the patch as soon as this series has been picked up.
Here is a preview:
commit a1a86f4db56e307d22918117515184fde540c14b
Author: David Hildenbrand <[email protected]> Date: Wed Nov 22 18:18:52 2017 +0100
s390x/tcg: fix size + content of STSI blocks
All blocks are 4k in size, which is only true for one of them right now. Also some reserved fields were wrong, fix it and convert all reserved fields to speficy u8.
This also fixes the LPAR part output in /proc/sysinfo under LPAR.
commit 41192e74109532d4c106c7b1a6bcf7f989b2f4fd
Author: David Hildenbrand <[email protected]> Date: Wed Nov 22 18:21:14 2017 +0100
s390x/tcg: STSI overhaul
Current STSI implementation is a mess, so let's rewrite it.
Problems fixed by this patch:
1) The order of exceptions/when recognized is wrong.
2) We have to store to virtual address space, not absolute.
3) Alignment check of the block is missing.
3) The SMP information is not indicated.
While at it:
a) Make the code look nicer
- get rid of nesting levels
- use struct initialization instead of initializing to zero
- rename a misspelled field and rename function code defines
- use a union and have only one write statement
- use cpu_to_beX()
b) Indicate the VM name/extended name + UUID just like KVM does
c) Indicate that all LPAR CPUs we fake are dedicated
d) Add a comment why we fake being a KVM guest
e) Give our guest as default the name "TCGguest"
f) Fake the same CPU information we have in our Guest for all layers
While at it, get rid of "potential_page_fault()" by forwarding the retaddr properly.
The result is best verified by looking at "/proc/sysinfo" in the guest when specifying on the qemu command line
-uuid "74738ff5-5367-5958-9aee-98fffdcd1876" \
-name "extra long guest name"
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v1 for-2.12 13/15] s390x/tcg: use program_interrupt_ra() in STSI
In reply to David HildenbrandQuoted 8 lines · David Hildenbrand
STSI needs some more love, but let's do one step at a time. We can now drop
potential_page_fault().Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/misc_helper.c | 2 +- target/s390x/translate.c | 1 - 2 files changed, 1 insertion(+), 2 deletions(-)
r~
PATCH 14/15 s390x/tcg: drop program_interrupt()
In reply to David HildenbrandAll users are gone, we can finally drop it and make sure that all new new program interrupt injections are reminded of the retaddr - as they have to use program_interrupt_ra() now.
--- target/s390x/cpu.h | 1 - target/s390x/interrupt.c | 22 ++++++++-------------- 2 files changed, 8 insertions(+), 15 deletions(-)
target/s390x/cpu.h +0 -1
@@ -719,7 +719,6 @@ void s390_io_interrupt(uint16_t subchannel_id, uint16_t subchannel_nr, uint32_t io_int_parm, uint32_t io_int_word);/* automatically detect the instruction length */ #define ILEN_AUTO 0xff -void program_interrupt (CPUS390XState *env,uint32_t code,int ilen);void program_interrupt_ra (CPUS390XState *env,uint32_t code,int ilen,uintptr_t ra);/* service interrupts are floating therefore we must not pass an cpustate */
target/s390x/interrupt.c +8 -14
@@ -37,13 +37,20 @@ static void tcg_s390_program_interrupt(CPUS390XState *env, uint32_t code,#endif } -void program_interrupt (CPUS390XState *env,uint32_t code,int ilen) +void program_interrupt_ra (CPUS390XState *env,uint32_t code,int ilen, +uintptr_t ra) {S390CPU *cpu =s390_env_get_cpu (env);qemu_log_mask (CPU_LOG_INT ,"program interrupt at %#" PRIx64"\n" , env->psw.addr); +#ifdef CONFIG_TCG +if (tcg_enabled () && ra) { +cpu_restore_state (CPU (cpu), ra); + } +#endif +if (kvm_enabled ()) {kvm_s390_program_interrupt (cpu, code); }else if (tcg_enabled ()) {
@@ -53,19 +60,6 @@ void program_interrupt(CPUS390XState *env, uint32_t code, int ilen) } } -void program_interrupt_ra (CPUS390XState *env,uint32_t code,int ilen, -uintptr_t ra) -{ -S390CPU *cpu =s390_env_get_cpu (env); - -#ifdef CONFIG_TCG -if (tcg_enabled () && ra) { -cpu_restore_state (CPU (cpu), ra); - } -#endif -program_interrupt (env, code, ilen); -} -#if !defined(CONFIG_USER_ONLY )static void cpu_inject_service (S390CPU *cpu,uint32_t param) {
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2.12 14/15] s390x/tcg: drop program_interrupt()
Quoted 2 lines · On Tue, 28 Nov 2017 21:33:24 +0100 David Hildenbrand <[email protected]> wrote:
All users are gone, we can finally drop it and make sure that all new new program interrupt injections are reminded of the retaddr - as they
s/new new/new/
Quoted 8 lines
have to use
program_interrupt_ra()now.Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/cpu.h | 1 - target/s390x/interrupt.c | 22 ++++++++-------------- 2 files changed, 8 insertions(+), 15 deletions(-)
Re: [Qemu-devel] [PATCH v1 for-2.12 14/15] s390x/tcg: drop program_interrupt()
In reply to David HildenbrandQuoted 4 lines · David Hildenbrand
target/s390x/interrupt.c +2
-1-void program_interrupt (CPUS390XState *env,uint32_t code,int ilen) +void program_interrupt_ra (CPUS390XState *env,uint32_t code,int ilen, +uintptr_t ra) {
target/s390x/interrupt.c static void tcg_s390_program_interrupt(CPUS390XState *env, uint32_t code, · line 52
Quoted 10 lines
target/s390x/interrupt.c +5
-0S390CPU *cpu =s390_env_get_cpu (env);qemu_log_mask (CPU_LOG_INT ,"program interrupt at %#" PRIx64"\n" , env->psw.addr); +#ifdef CONFIG_TCG +if (tcg_enabled () && ra) { +cpu_restore_state (CPU (cpu), ra); + } +#endif
Please move this into tcg_s390_program_interrupt by using cpu_loop_exit_restore there.
r~
Re: [Qemu-devel] [PATCH v1 for-2.12 14/15] s390x/tcg: drop program_interrupt()
Quoted 10 lines · Richard Henderson
Quoted 9 lines · David Hildenbrand
target/s390x/interrupt.c +2
-1-void program_interrupt (CPUS390XState *env,uint32_t code,int ilen) +void program_interrupt_ra (CPUS390XState *env,uint32_t code,int ilen, +uintptr_t ra) {S390CPU *cpu =s390_env_get_cpu (env);qemu_log_mask (CPU_LOG_INT ,"program interrupt at %#" PRIx64"\n" , env->psw.addr);
Quoted 9 lines
target/s390x/interrupt.c static void tcg_s390_program_interrupt(CPUS390XState *env, uint32_t code, · line 52
Quoted 5 lines
target/s390x/interrupt.c +5
-0+#ifdef CONFIG_TCG +if (tcg_enabled () && ra) { +cpu_restore_state (CPU (cpu), ra); + } +#endif Please move this into
tcg_s390_program_interruptby usingcpu_loop_exit_restorethere.
Indeed, good idea, thanks!
Quoted 3 lines
r~
Signature · 3 lines
Thanks, David / dhildenb
PATCH 15/15 s390x/tcg: drop potential_page_fault()
In reply to David HildenbrandOnly one user left, get rid of it so we don't get any new users.
--- target/s390x/translate.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
target/s390x/translate.c +2 -7
@@ -240,12 +240,6 @@ static void update_cc_op(DisasContext *s) } } -static void potential_page_fault (DisasContext *s) -{ -update_psw_addr (s); -update_cc_op (s); -} -static inline uint64_t ld_code2 (CPUS390XState *env,uint64_t pc) {return (uint64_t )cpu_lduw_code (env, pc);
@@ -2939,7 +2933,8 @@ static ExitStatus op_lpd(DisasContext *s, DisasOps *o)/* In a parallel context, stop the world and single step. */ if (tb_cflags (s->tb) &CF_PARALLEL ) { -potential_page_fault (s); +update_psw_addr (s); +update_cc_op (s);gen_exception (EXCP_ATOMIC );return EXIT_NORETURN ; }
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2.12 15/15] s390x/tcg: drop potential_page_fault()
Quoted 6 lines · David Hildenbrand
Only one user left, get rid of it so we don't get any new users.
Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/translate.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
r~
Quoted 7 lines · David Hildenbrand
I have quite some patches on my queue for 2.12. (booting Fedora 26/27 guests, floating interrupts, machine checks, missing instructions ...)
So let's start slowly :) This series gets rid of
program_interrupt()andpotential_page_fault(). We now always properly restore the cpu state when injecting/delivering a program interrupt. So there is no need to update the state viapotential_page_fault()anymore.
I think this series is basically a very good idea! But... OK, this is kind of bike-shed-painting now, but since we're currently in hard freeze anyway and got plenty of time for discussion:
Something that bothers me a little bit is the name of the new function "program_interrupt_ra()" ... that would IMHO be OK if the old function "program_interrupt" would still stay, but since that is removed and the _ra function is the only generic way that is left to inject a program interrupt, could we maybe name the new function somewhat nicer right from the start? Something like "s390_program_interrupt" maybe (which is similar to tcg_s390_program_interrupt and kvm_s390_program_interrupt that we have already)?
Thomas
Quoted 9 lines · Thomas Huth
Quoted 7 lines · David Hildenbrand
I have quite some patches on my queue for 2.12. (booting Fedora 26/27 guests, floating interrupts, machine checks, missing instructions ...)
So let's start slowly :) This series gets rid of
program_interrupt()andpotential_page_fault(). We now always properly restore the cpu state when injecting/delivering a program interrupt. So there is no need to update the state viapotential_page_fault()anymore.
Quoted 11 lines
I think this series is basically a very good idea! But... OK, this is kind of bike-shed-painting now, but since we're currently in hard freeze anyway and got plenty of time for discussion:
Something that bothers me a little bit is the name of the new function "program_interrupt_ra()" ... that would IMHO be OK if the old function "program_interrupt" would still stay, but since that is removed and the_rafunction is the only generic way that is left to inject a program interrupt, could we maybe name the new function somewhat nicer right from the start? Something like "s390_program_interrupt" maybe (which is similar totcg_s390_program_interruptandkvm_s390_program_interruptthat we have already)?
Sure I can do that, other opinions?
Quoted 3 lines
Thomas
Signature · 3 lines
Thanks, David / dhildenb
Quoted 22 lines · On Wed, 29 Nov 2017 15:06:33 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 9 lines · Thomas Huth
Quoted 7 lines · David Hildenbrand
I have quite some patches on my queue for 2.12. (booting Fedora 26/27 guests, floating interrupts, machine checks, missing instructions ...)
So let's start slowly :) This series gets rid of
program_interrupt()andpotential_page_fault(). We now always properly restore the cpu state when injecting/delivering a program interrupt. So there is no need to update the state viapotential_page_fault()anymore.Quoted 11 lines
I think this series is basically a very good idea! But... OK, this is kind of bike-shed-painting now, but since we're currently in hard freeze anyway and got plenty of time for discussion:
Something that bothers me a little bit is the name of the new function "program_interrupt_ra()" ... that would IMHO be OK if the old function "program_interrupt" would still stay, but since that is removed and the_rafunction is the only generic way that is left to inject a program interrupt, could we maybe name the new function somewhat nicer right from the start? Something like "s390_program_interrupt" maybe (which is similar totcg_s390_program_interruptandkvm_s390_program_interruptthat we have already)?
Quoted 1 line
Sure I can do that, other opinions?
Fine with me as well.