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

[Collin Walling]

On 11/10/20 5:51 AM, Cornelia Huck wrote:
> On Wed,  4 Nov 2020 13:12:44 -0500
> Collin Walling <walling@linux.ibm.com> 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")
> 
> (I guess?)
> 
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>  hw/s390x/s390-virtio-ccw.c |  5 +++++
>>  target/s390x/cpu.c         | 10 ++++++++++
>>  target/s390x/cpu.h         |  1 +
>>  target/s390x/kvm.c         | 10 ++++++----
>>  4 files changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 22222c4fd5..a8cebf9bff 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -427,6 +427,9 @@ static void s390_machine_reset(MachineState *machine)
>>          qemu_devices_reset();
>>          s390_crypto_reset();
>>  
>> +        CPU_FOREACH(t) {
>> +            run_on_cpu(t, s390_do_cpu_set_diag318, 
>> RUN_ON_CPU_HOST_ULONG(0));
>> +        }
>>          /* configure and start the ipl CPU only */
>>          run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
>>          break;
>> @@ -440,6 +443,7 @@ static void s390_machine_reset(MachineState *machine)
>>          s390_pv_prepare_reset(ms);
>>          CPU_FOREACH(t) {
>>              run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
>> +            run_on_cpu(t, s390_do_cpu_set_diag318, 
>> RUN_ON_CPU_HOST_ULONG(0));
>>          }
>>          run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>>          break;
>> @@ -451,6 +455,7 @@ static void s390_machine_reset(MachineState *machine)
>>          subsystem_reset();
>>          s390_pv_prepare_reset(ms);
>>          CPU_FOREACH(t) {
>> +            run_on_cpu(t, s390_do_cpu_set_diag318, 
>> RUN_ON_CPU_HOST_ULONG(0));
>>              if (t == cs) {
>>                  continue;
>>              }
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index 66f5942b53..cb0548b4d8 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -447,6 +447,16 @@ 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)
>> +{
>> +    CPUS390XState *env = &S390_CPU(cs)->env;
>> +    uint64_t diag318_info = arg.host_ulong;
> 
> Should you check with can_sync_regs() here? Or in the callers?
> 
> Do we need this even without the cpu feature bit set?
> 

Hmm yeah it may be best to check for both the feature bit and sync
capability here.

>> +
>> +    env->diag318_info = diag318_info;
>> +    cs->kvm_run->s.regs.diag318 = diag318_info;
>> +    cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
>> +}
>>  #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.c b/target/s390x/kvm.c
>> index baa070fdf7..4b2aad009c 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -1615,6 +1615,7 @@ 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 +1623,14 @@ 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;
> 
> Seems like this return already fixes a bug on its own?

Yes. I can split this into a separate patch.

> 
>>      }
>>  
>> -    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));
>> +        }
>>      }
>>  }
>>  
> 
> 


-- 
Regards,
Collin

Stay safe and stay healthy