Re: [PATCH] s390x: kvm: adjust diag318 resets to retain data

[Christian Borntraeger]

Am 08.11.21 um 18:02 schrieb Janosch Frank:
> On 11/5/21 23:46, Collin Walling wrote:
> > The CPNC portion of the diag 318 data is erroneously reset during an
> > initial CPU reset caused by SIGP. Let's go ahead and relocate the
> > diag318_info field within the CPUS390XState struct such that it is
> > only zeroed during a clear reset. This way, the CPNC will be retained
> > for each VCPU in the configuration after the diag 318 instruction
> > has been invoked by the kernel.
> >
> > Additionally, the diag 318 data reset is handled via the CPU reset
> > code. The set_diag318 code can be merged into the handler function
> > and the helper functions can consequently be removed.
> >
> > Signed-off-by: Collin Walling <walling@linux.ibm.com>
>
> Fixes tag?
>
> > ---
> >   hw/s390x/s390-virtio-ccw.c   |  3 ---
> >   target/s390x/cpu-sysemu.c    |  7 -------
> >   target/s390x/cpu.h           |  5 ++---
> >   target/s390x/kvm/kvm.c       | 19 +++++--------------
> >   target/s390x/kvm/kvm_s390x.h |  1 -
> >   5 files changed, 7 insertions(+), 28 deletions(-)
> >
> > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > index 653587ea62..51dcb83b0c 100644
> > --- a/hw/s390x/s390-virtio-ccw.c
> > +++ b/hw/s390x/s390-virtio-ccw.c
> > @@ -489,9 +489,6 @@ static void s390_machine_reset(MachineState *machine)
> >           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-sysemu.c b/target/s390x/cpu-sysemu.c
> > index 5471e01ee8..6d9f6d4402 100644
> > --- a/target/s390x/cpu-sysemu.c
> > +++ b/target/s390x/cpu-sysemu.c
> > @@ -299,10 +299,3 @@ 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);
> > -    }
> > -}
> > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> > index 3153d053e9..1b94b91d87 100644
> > --- a/target/s390x/cpu.h
> > +++ b/target/s390x/cpu.h
> > @@ -63,6 +63,8 @@ struct CPUS390XState {
> >       uint64_t etoken;       /* etoken */
> >       uint64_t etoken_extension; /* etoken extension */
> > +    uint64_t diag318_info;
>
> Before we brought this upstream I had a conversation with the architect because 
> I was confused about this myself. He said: SIGP does not affect 318 data but 
> all 308 subcode resets do (0,1,3,4).
>
> Hence I'd much rather move this out of the automatic reset areas and clear it 
> by hand for diag308 resets. And then add a big comment with a warning to never 
> move this into the automatic clearing areas.

the automatic cleaning areas are also used for the big hammer system_reset in 
QEMU which acts like a power cycle. And those fields are initialized always on 
such events.
 So you cannot avoid clearing this for the big hammer things anyway.
> > +
> >       /* Fields up to this point are not cleared by initial CPU reset */
> >       struct {} start_initial_reset_fields;
> > @@ -118,8 +120,6 @@ struct CPUS390XState {
> >       uint16_t external_call_addr;
> >       DECLARE_BITMAP(emergency_signals, S390_MAX_CPUS);
> > -    uint64_t diag318_info;
> > -
> >   #if !defined(CONFIG_USER_ONLY)
> >       uint64_t tlb_fill_tec;   /* translation exception code during tlb_fill */
> >       int tlb_fill_exc;        /* exception number seen during tlb_fill */
> > @@ -780,7 +780,6 @@ 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/kvm.c b/target/s390x/kvm/kvm.c
> > index 5b1fdb55c4..ed9c477b6f 100644
> > --- a/target/s390x/kvm/kvm.c
> > +++ b/target/s390x/kvm/kvm.c
> > @@ -1576,18 +1576,6 @@ 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;
> > @@ -1604,8 +1592,11 @@ static void handle_diag_318(S390CPU *cpu, struct kvm_run 
> > *run)
> >       }
> >       CPU_FOREACH(t) {
> > -        run_on_cpu(t, s390_do_cpu_set_diag318,
> > -                   RUN_ON_CPU_HOST_ULONG(diag318_info));
> > +        CPUS390XState *env = &S390_CPU(t)->env;
> > +
> > +        env->diag318_info = diag318_info;
> > +        t->kvm_run->s.regs.diag318 = diag318_info;
> > +        t->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
> >       }
> >   }
> > diff --git a/target/s390x/kvm/kvm_s390x.h b/target/s390x/kvm/kvm_s390x.h
> > index 05a5e1e6f4..8c244ee84d 100644
> > --- a/target/s390x/kvm/kvm_s390x.h
> > +++ b/target/s390x/kvm/kvm_s390x.h
> > @@ -44,6 +44,5 @@ 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 */