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

[Collin Walling]

On 11/13/20 3:06 AM, Thomas Huth wrote:
> On 12/11/2020 22.36, 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>
>> ---
>>
>> Changelog:
>>
>>         v2:
>>         - added Fixes tag
>>         - added CPU feat check in do_cpu function
> 
> Sorry, but this fails to build on x86 (i.e. without kvm):
> 
> ../../devel/qemu/target/s390x/cpu.c: In function ‘s390_do_cpu_set_diag318’:
> ../../devel/qemu/target/s390x/cpu.c:459:20: error: dereferencing pointer to
> incomplete type ‘struct kvm_run’
>          cs->kvm_run->s.regs.diag318 = diag318_info;
>                     ^~
> ../../devel/qemu/target/s390x/cpu.c:460:40: error: ‘KVM_SYNC_DIAG318’
> undeclared (first use in this function)
>          cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
>                                         ^~~~~~~~~~~~~~~~
> ../../devel/qemu/target/s390x/cpu.c:460:40: note: each undeclared identifier
> is reported only once for each function it appears in
> [878/1030] Compiling C object
> libqemu-s390x-softmmu.fa.p/target_s390x_misc_helper.c.o
> ninja: build stopped: subcommand failed.
> 
> (By the way, is Patchew down or now not compile-testing anymore?)
> 

Hmmm... okay, I guess that means I need to split this code into a
separate function defined in kvm.c, darn.

>> ---
>>  hw/s390x/s390-virtio-ccw.c |  5 +++++
>>  target/s390x/cpu.c         | 13 +++++++++++++
>>  target/s390x/cpu.h         |  1 +
>>  target/s390x/kvm.c         | 10 +++++-----
>>  4 files changed, 24 insertions(+), 5 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));
>> +        }
> 
> Just double-checking: Is it really ok to modify all CPUs here, while the
> code below only touches the ipl-CPU ?
I fail to see anything wrong with it. The diag318 data needs to be reset
for all CPUs during this event somehow.

If I understand the single ipl-CPU code correctly: IPL is kicked off by
only a single CPU, and the do_cpu_ipl prepares the single CPU for IPL
(sets IPL PSW, prepares IPLB, etc). There isn't a reason to signal all
CPUs for IPL.

Unless you think there might be something else at play here?

> 
>>          /* 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;
>>              }
> 
> Would it be really wrong to also clear the field in case of S390_RESET_PV?
> S390_RESET_PV is also a reset, so I'd assume we could (or even should) also
> clear the field in that case?
> If so, the new run_on_cpu() statements could maybe also be merged into a
> single one that is done before or after the "switch (reset_type)" statement?
> 

Thinking about it, your approach would work just fine. Diag318 is
disabled when in PV mode anyway.

>  Thomas
> 

Thanks!

-- 
Regards,
Collin

Stay safe and stay healthy