On 11/28/19 4:28 PM, Thomas Huth wrote:
> 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 <frankja@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?

I'd like to keep decode_basedisp_s() as is, but how about a static
function in ioinst.c called something like get_address_from_regs()?

It'll call decode_basedisp_s() or return 0.


> 
> Anyway,
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Thanks!