Re: [PATCH v9 12/15] s390x: protvirt: Move IO control structures over SIDA

[David Hildenbrand]

On 11.03.20 14:21, Janosch Frank wrote:
> For protected guests, we need to put the IO emulation results into the
> SIDA, so SIE will write them into the guest at the next entry.
> 
> Signed-off-by: Janosch Frank <address@hidden>
> ---
>  target/s390x/ioinst.c | 87 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 61 insertions(+), 26 deletions(-)
> 
> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index 481d789de9e09a04..61095bdc9ffef436 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -136,9 +136,13 @@ void ioinst_handle_msch(S390CPU *cpu, uint64_t reg1, 
> uint32_t ipb, uintptr_t ra)
>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>          return;
>      }
> -    if (s390_cpu_virt_mem_read(cpu, addr, ar, &schib, sizeof(schib))) {
> -        s390_cpu_virt_mem_handle_exc(cpu, ra);
> -        return;
> +    if (s390_is_pv()) {
> +        s390_cpu_pv_mem_read(cpu, addr, &schib, sizeof(schib));
> +    } else {

} else if ( ) {?

> +        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) ||
>          !ioinst_schib_valid(&schib)) {
> @@ -193,9 +197,13 @@ void ioinst_handle_ssch(S390CPU *cpu, uint64_t reg1, 
> uint32_t ipb, uintptr_t ra)
>          s390_program_interrupt(env, PGM_SPECIFICATION, ra);
>          return;
>      }
> -    if (s390_cpu_virt_mem_read(cpu, addr, ar, &orig_orb, sizeof(orb))) {
> -        s390_cpu_virt_mem_handle_exc(cpu, ra);
> -        return;
> +    if (s390_is_pv()) {
> +        s390_cpu_pv_mem_read(cpu, addr, &orig_orb, sizeof(orb));
> +    } else {

dito

> +        if (s390_cpu_virt_mem_read(cpu, addr, ar, &orig_orb, sizeof(orb))) {
> +            s390_cpu_virt_mem_handle_exc(cpu, ra);
> +            return;
> +        }
>      }

[...]
>  
> @@ -258,6 +271,9 @@ void ioinst_handle_stsch(S390CPU *cpu, uint64_t reg1, 
> uint32_t ipb,
>      }
>  
>      if (ioinst_disassemble_sch_ident(reg1, &m, &cssid, &ssid, &schid)) {
> +        if (s390_is_pv()) {
> +            return;
> +        }

I'd prefer a comment here, explaining what the UV checks and why we
don't have to do anything.

>          /*
>           * As operand exceptions have a lower priority than access 
> exceptions,
>           * we check whether the memory area is writeable (injecting the
> @@ -290,14 +306,19 @@ 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;
> +        if (s390_is_pv()) {
> +            s390_cpu_pv_mem_write(cpu, addr, &schib, sizeof(schib));
> +        } else {
> +            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) {
> +        if (!s390_is_pv() &&
> +            s390_cpu_virt_mem_check_write(cpu, addr, ar, sizeof(schib)) != 
> 0) {
>              s390_cpu_virt_mem_handle_exc(cpu, ra);
>              return;
>          }
> @@ -334,15 +355,20 @@ 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;
> +        if (s390_is_pv()) {
> +            s390_cpu_pv_mem_write(cpu, addr, &irb, irb_len);
> +        } else {
> +            if (s390_cpu_virt_mem_write(cpu, addr, ar, &irb, irb_len) != 0) {

dito

> +                s390_cpu_virt_mem_handle_exc(cpu, ra);
> +                return -EFAULT;
> +            }
>          }
>          css_do_tsch_update_subch(sch);
>      } else {
>          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) {
> +        if (!s390_is_pv() &&
> +            s390_cpu_virt_mem_check_write(cpu, addr, ar, irb_len) != 0) {
>              s390_cpu_virt_mem_handle_exc(cpu, ra);
>              return -EFAULT;
>          }
> @@ -640,9 +666,13 @@ void ioinst_handle_chsc(S390CPU *cpu, uint32_t ipb, 
> uintptr_t ra)
>       * present CHSC sub-handlers ... if we ever need more, we should take
>       * 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;
> +    if (s390_is_pv()) {
> +        s390_cpu_pv_mem_read(cpu, addr, buf, sizeof(ChscReq));
> +    } else {

dito.

> +        if (s390_cpu_virt_mem_read(cpu, addr, reg, buf, sizeof(ChscReq))) {
> +            s390_cpu_virt_mem_handle_exc(cpu, ra);
> +            return;
> +        }
>      }

LGTM

Reviewed-by: David Hildenbrand <address@hidden>

-- 
Thanks,

David / dhildenb