Re: [PATCH v4] KVM: s390: fix gisa destroy operation might lead to cpu stalls

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Michael Mueller (2023-09-04 16:11:26)
> 
> 
> On 04.09.23 09:05, Nico Boehr wrote:
> > Quoting Michael Mueller (2023-09-01 12:58:23)
> > [...]
> >> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> >> index 9bd0a873f3b1..96450e5c4b6f 100644
> >> --- a/arch/s390/kvm/interrupt.c
> >> +++ b/arch/s390/kvm/interrupt.c
> > [...]
> >>   static inline void gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
> >>   {
> >>          set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
> >> @@ -3202,11 +3197,12 @@ void kvm_s390_gisa_destroy(struct kvm *kvm)
> >>   
> >>          if (!gi->origin)
> >>                  return;
> >> -       if (gi->alert.mask)
> >> -               KVM_EVENT(3, "vm 0x%pK has unexpected iam 0x%02x",
> >> -                         kvm, gi->alert.mask);
> >> -       while (gisa_in_alert_list(gi->origin))
> >> -               cpu_relax();
> >> +       WARN(gi->alert.mask != 0x00,
> >> +            "unexpected non zero alert.mask 0x%02x",
> >> +            gi->alert.mask);
> >> +       gi->alert.mask = 0x00;
> >> +       if (gisa_set_iam(gi->origin, gi->alert.mask))
> >> +               process_gib_alert_list();
> > 
> > I am not an expert for the GISA, so excuse my possibly stupid question:
> > process_gib_alert_list() starts the timer. So can gisa_vcpu_kicker()
> > already be running before we reach hrtimer_cancel() below? Is this fine?
> 
> You are right, It cannnot be running in that situation because 
> gisa_vcpu_kicker() has returned with HRTIMER_NORESTART and no vcpus are 
> defined anymore.
> 
> There is another case when the gisa specific timer is started not only 
> in the process_gib_alert() case but also when a vcpu of the guest owning 
> the gisa is put into wait state, see kvm_s390_handle_wait(). Thus yes, 
> it could be running already in that situation. I remember having seen 
> this situation when I write the gisa/gib code. But that's case in 
> process_gib_alert_list()
> 
> It does not hurt to have the hrtimer_cancel() here but I don't want to 
> add a change to this patch. Eventually a new one.

Ah OK, thanks for the explanation. hrtimer_cancel() also waits until the
timer function has completed. LGTM then.

Reviewed-by: Nico Boehr <nrb@xxxxxxxxxxxxx>




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux