On 20/11/2019 12.43, Janosch Frank wrote:
> IO instruction data is routed through SIDAD for protected guests, so
> adresses do not need to be checked, as this is kernel memory.
>
> Signed-off-by: Janosch Frank <fran...@linux.ibm.com>
> ---
> target/s390x/ioinst.c | 46 +++++++++++++++++++++++++++----------------
> 1 file changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index c437a1d8c6..d3bd422ddd 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -110,11 +110,13 @@ void ioinst_handle_msch(S390CPU *cpu, uint64_t reg1,
> uint32_t ipb, uintptr_t ra)
> int cssid, ssid, schid, m;
> SubchDev *sch;
> SCHIB schib;
> - uint64_t addr;
> + uint64_t addr = 0;
> CPUS390XState *env = &cpu->env;
> - uint8_t ar;
> + uint8_t ar = 0;
>
> - addr = decode_basedisp_s(env, ipb, &ar);
> + if (!env->pv) {
> + addr = decode_basedisp_s(env, ipb, &ar);
> + }
> if (addr & 3) {
> s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> return;
> @@ -167,11 +169,13 @@ void ioinst_handle_ssch(S390CPU *cpu, uint64_t reg1,
> uint32_t ipb, uintptr_t ra)
> int cssid, ssid, schid, m;
> SubchDev *sch;
> ORB orig_orb, orb;
> - uint64_t addr;
> + uint64_t addr = 0;
> CPUS390XState *env = &cpu->env;
> - uint8_t ar;
> + uint8_t ar = 0;
>
> - addr = decode_basedisp_s(env, ipb, &ar);
> + if (!env->pv) {
> + addr = decode_basedisp_s(env, ipb, &ar);
> + }
> if (addr & 3) {
> s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> return;
> @@ -198,12 +202,14 @@ 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)
> {
> CRW crw;
> - uint64_t addr;
> + uint64_t addr = 0;
> int cc;
> CPUS390XState *env = &cpu->env;
> - uint8_t ar;
> + uint8_t ar = 0;
>
> - addr = decode_basedisp_s(env, ipb, &ar);
> + if (!env->pv) {
> + addr = decode_basedisp_s(env, ipb, &ar);
> + }
> if (addr & 3) {
> s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> return;
> @@ -228,13 +234,15 @@ void ioinst_handle_stsch(S390CPU *cpu, uint64_t reg1,
> uint32_t ipb,
> {
> int cssid, ssid, schid, m;
> SubchDev *sch;
> - uint64_t addr;
> + uint64_t addr = 0;
> int cc;
> SCHIB schib;
> CPUS390XState *env = &cpu->env;
> - uint8_t ar;
> + uint8_t ar = 0;
>
> - addr = decode_basedisp_s(env, ipb, &ar);
> + if (!env->pv) {
> + addr = decode_basedisp_s(env, ipb, &ar);
> + }
> if (addr & 3) {
> s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> return;
> @@ -294,16 +302,18 @@ int ioinst_handle_tsch(S390CPU *cpu, uint64_t reg1,
> uint32_t ipb, uintptr_t ra)
> int cssid, ssid, schid, m;
> SubchDev *sch;
> IRB irb;
> - uint64_t addr;
> + uint64_t addr = 0;
> int cc, irb_len;
> - uint8_t ar;
> + uint8_t ar = 0;
>
> if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
> s390_program_interrupt(env, PGM_OPERAND, ra);
> return -EIO;
> }
> trace_ioinst_sch_id("tsch", cssid, ssid, schid);
> - addr = decode_basedisp_s(env, ipb, &ar);
> + if (!env->pv) {
> + addr = decode_basedisp_s(env, ipb, &ar);
> + }
> if (addr & 3) {
> s390_program_interrupt(env, PGM_SPECIFICATION, ra);
> return -EIO;
Would it make sense to hide all these changes in decode_basedisp_s()
instead? ... so that decode_basedisp_s() returns 0 if env->pv == true ?
... or are there still cases where we need real values from
decode_basedisp_s() in case of env->pv==true?
Anyway,
Reviewed-by: Thomas Huth <th...@redhat.com>
