Re: [PATCH v3] s390/kvm: fix diag318 propagation and reset functionality

[Janosch Frank]

On 11/13/20 11:10 PM, Collin Walling wrote:
> The Control Program Name Code (CPNC) portion of the diag318
> info must be set within the SIE block of each VCPU in the
> configuration. The handler will iterate through each VCPU
> and dirty the diag318_info reg to be synced with KVM on a
> subsequent sync_regs call.
> 
> Additionally, the diag318 info resets must be handled via
> userspace. As such, QEMU will reset this value for each
> VCPU during a modified clear, load normal, and load clear
> reset event.
> 
> Fixes: fabdada9357b ("s390: guest support for diagnose 0x318")
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---

Please push to devel so we get some coverage.

I've run the KVM unit tests and started a normal PV guest for a simple
smoke test.
Reviewed-by: Janosch Frank <frankja@de.ibm.com>

> 
> Changelog:
> 
>         v3:
>         - moved loop outside of switch block
>         - added kvm_s390_set_diag318 function, called by
>             do_cpu_function (this is so other archs do
>             not complain)
> 
>         v2:
>         - added Fixes tag
>         - added CPU feat check in do_cpu function
> 
> ---
>  hw/s390x/s390-virtio-ccw.c |  4 ++++
>  target/s390x/cpu.c         |  7 +++++++
>  target/s390x/cpu.h         |  1 +
>  target/s390x/kvm-stub.c    |  4 ++++
>  target/s390x/kvm.c         | 22 +++++++++++++++++-----
>  target/s390x/kvm_s390x.h   |  1 +
>  6 files changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 22222c4fd5..4e140bbead 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -486,6 +486,10 @@ static void s390_machine_reset(MachineState *machine)
>      default:
>          g_assert_not_reached();
>      }
> +
> +    CPU_FOREACH(t) {
> +        run_on_cpu(t, s390_do_cpu_set_diag318, RUN_ON_CPU_HOST_ULONG(0));
> +    }
>      s390_ipl_clear_reset_request();
>  }
>  
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 66f5942b53..7b66718c44 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -447,6 +447,13 @@ void s390_enable_css_support(S390CPU *cpu)
>          kvm_s390_enable_css_support(cpu);
>      }
>  }
> +
> +void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg)
> +{
> +    if (kvm_enabled()) {
> +        kvm_s390_set_diag318(cs, arg.host_ulong);
> +    }
> +}
>  #endif
>  
>  static gchar *s390_gdb_arch_name(CPUState *cs)
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index f875ebf0f4..60d434d5ed 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -769,6 +769,7 @@ int s390_set_memory_limit(uint64_t new_limit, uint64_t 
> *hw_limit);
>  void s390_set_max_pagesize(uint64_t pagesize, Error **errp);
>  void s390_cmma_reset(void);
>  void s390_enable_css_support(S390CPU *cpu);
> +void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg);
>  int s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch_id,
>                                  int vq, bool assign);
>  #ifndef CONFIG_USER_ONLY
> diff --git a/target/s390x/kvm-stub.c b/target/s390x/kvm-stub.c
> index aa185017a2..9970b5a8c7 100644
> --- a/target/s390x/kvm-stub.c
> +++ b/target/s390x/kvm-stub.c
> @@ -120,3 +120,7 @@ void kvm_s390_stop_interrupt(S390CPU *cpu)
>  void kvm_s390_restart_interrupt(S390CPU *cpu)
>  {
>  }
> +
> +void kvm_s390_set_diag318(CPUState *cs, uint64_t diag318_info)
> +{
> +}
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index baa070fdf7..b8385e6b95 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1611,10 +1611,23 @@ static int handle_sw_breakpoint(S390CPU *cpu, struct 
> kvm_run *run)
>      return -ENOENT;
>  }
>  
> +void kvm_s390_set_diag318(CPUState *cs, uint64_t diag318_info)
> +{
> +    CPUS390XState *env = &S390_CPU(cs)->env;
> +
> +    /* Feat bit is set only if KVM supports sync for diag318 */
> +    if (s390_has_feat(S390_FEAT_DIAG_318)) {
> +        env->diag318_info = diag318_info;
> +        cs->kvm_run->s.regs.diag318 = diag318_info;
> +        cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
> +    }
> +}
> +
>  static void handle_diag_318(S390CPU *cpu, struct kvm_run *run)
>  {
>      uint64_t reg = (run->s390_sieic.ipa & 0x00f0) >> 4;
>      uint64_t diag318_info = run->s.regs.gprs[reg];
> +    CPUState *t;
>  
>      /*
>       * DIAG 318 can only be enabled with KVM support. As such, let's
> @@ -1622,13 +1635,12 @@ static void handle_diag_318(S390CPU *cpu, struct 
> kvm_run *run)
>       */
>      if (!s390_has_feat(S390_FEAT_DIAG_318)) {
>          kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
> +        return;
>      }
>  
> -    cpu->env.diag318_info = diag318_info;
> -
> -    if (can_sync_regs(CPU(cpu), KVM_SYNC_DIAG318)) {
> -        run->s.regs.diag318 = diag318_info;
> -        run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
> +    CPU_FOREACH(t) {
> +        run_on_cpu(t, s390_do_cpu_set_diag318,
> +                   RUN_ON_CPU_HOST_ULONG(diag318_info));
>      }
>  }
>  
> diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
> index 6ab17c81b7..25bbe98b25 100644
> --- a/target/s390x/kvm_s390x.h
> +++ b/target/s390x/kvm_s390x.h
> @@ -45,5 +45,6 @@ void kvm_s390_set_max_pagesize(uint64_t pagesize, Error 
> **errp);
>  void kvm_s390_crypto_reset(void);
>  void kvm_s390_restart_interrupt(S390CPU *cpu);
>  void kvm_s390_stop_interrupt(S390CPU *cpu);
> +void kvm_s390_set_diag318(CPUState *cs, uint64_t diag318_info);
>  
>  #endif /* KVM_S390X_H */
>

OpenPGP_0xE354E6B8E238B9F8.asc
Description: application/pgp-keys

OpenPGP_signature
Description: OpenPGP digital signature