Re: [PATCH v9 11/15] s390x: protvirt: Disable address checks for PV guest IO emulation

[Janosch Frank]

On 3/12/20 4:41 PM, Christian Borntraeger wrote:
> On 11.03.20 14:21, 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.
> 
> Maybe also mention that the data structures for  the I/O instrutions
> start at offset 0 of the SIDA?

You mean in the comment for get_address_from_regs() or in the commit
message? It would certainly make sense for get_address_from_regs().

>>
>> Signed-off-by: Janosch Frank <address@hidden>
>> Reviewed-by: Thomas Huth <address@hidden>
>> Reviewed-by: David Hildenbrand <address@hidden>
> 
> 
> Reviewed-by: Christian Borntraeger <address@hidden>
> 
> 
> 
>> ---
>>  target/s390x/ioinst.c | 33 ++++++++++++++++++++++++++-------
>>  1 file changed, 26 insertions(+), 7 deletions(-)
>>
>> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
>> index c437a1d8c6afed80..481d789de9e09a04 100644
>> --- a/target/s390x/ioinst.c
>> +++ b/target/s390x/ioinst.c
>> @@ -16,6 +16,23 @@
>>  #include "hw/s390x/ioinst.h"
>>  #include "trace.h"
>>  #include "hw/s390x/s390-pci-bus.h"
>> +#include "hw/s390x/pv.h"
>> +
>> +static uint64_t get_address_from_regs(CPUS390XState *env, uint32_t ipb,
>> +                                      uint8_t *ar)
>> +{
>> +    /*
>> +     * Addresses for protected guests are all offsets into the
>> +     * satellite block which holds the IO control structures. Those
>> +     * control structures are always aligned and accessible, so we can
>> +     * return 0 here which will pass the following address checks.
>> +     */
>> +    if (s390_is_pv()) {
>> +        *ar = 0;
>> +        return 0;
>> +    }
>> +    return decode_basedisp_s(env, ipb, ar);
>> +}
>>  
>>  int ioinst_disassemble_sch_ident(uint32_t value, int *m, int *cssid, int 
>> *ssid,
>>                                   int *schid)
>> @@ -114,7 +131,7 @@ void ioinst_handle_msch(S390CPU *cpu, uint64_t reg1, 
>> uint32_t ipb, uintptr_t ra)
>>      CPUS390XState *env = &cpu->env;
>>      uint8_t ar;
>>  
>> -    addr = decode_basedisp_s(env, ipb, &ar);
>> +    addr = get_address_from_regs(env, ipb, &ar);
>>      if (addr & 3) {
>>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>>          return;
>> @@ -171,7 +188,7 @@ void ioinst_handle_ssch(S390CPU *cpu, uint64_t reg1, 
>> uint32_t ipb, uintptr_t ra)
>>      CPUS390XState *env = &cpu->env;
>>      uint8_t ar;
>>  
>> -    addr = decode_basedisp_s(env, ipb, &ar);
>> +    addr = get_address_from_regs(env, ipb, &ar);
>>      if (addr & 3) {
>>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>>          return;
>> @@ -203,7 +220,7 @@ void ioinst_handle_stcrw(S390CPU *cpu, uint32_t ipb, 
>> uintptr_t ra)
>>      CPUS390XState *env = &cpu->env;
>>      uint8_t ar;
>>  
>> -    addr = decode_basedisp_s(env, ipb, &ar);
>> +    addr = get_address_from_regs(env, ipb, &ar);
>>      if (addr & 3) {
>>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>>          return;
>> @@ -234,7 +251,7 @@ void ioinst_handle_stsch(S390CPU *cpu, uint64_t reg1, 
>> uint32_t ipb,
>>      CPUS390XState *env = &cpu->env;
>>      uint8_t ar;
>>  
>> -    addr = decode_basedisp_s(env, ipb, &ar);
>> +    addr = get_address_from_regs(env, ipb, &ar);
>>      if (addr & 3) {
>>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>>          return;
>> @@ -303,7 +320,7 @@ int ioinst_handle_tsch(S390CPU *cpu, uint64_t reg1, 
>> uint32_t ipb, uintptr_t ra)
>>          return -EIO;
>>      }
>>      trace_ioinst_sch_id("tsch", cssid, ssid, schid);
>> -    addr = decode_basedisp_s(env, ipb, &ar);
>> +    addr = get_address_from_regs(env, ipb, &ar);
>>      if (addr & 3) {
>>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>>          return -EIO;
>> @@ -601,7 +618,7 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, 
>> uintptr_t ra)
>>  {
>>      ChscReq *req;
>>      ChscResp *res;
>> -    uint64_t addr;
>> +    uint64_t addr = 0;
>>      int reg;
>>      uint16_t len;
>>      uint16_t command;
>> @@ -610,7 +627,9 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, 
>> uintptr_t ra)
>>  
>>      trace_ioinst("chsc");
>>      reg = (ipb >> 20) & 0x00f;
>> -    addr = env->regs[reg];
>> +    if (!s390_is_pv()) {
>> +        addr = env->regs[reg];
>> +    }
>>      /* Page boundary? */
>>      if (addr & 0xfff) {
>>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>>

signature.asc
Description: OpenPGP digital signature