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

[Thomas Huth]

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?)

> ---
>  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 ?

>          /* 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?

 Thomas