Re: [PATCH] crypto: virtio-crypto: call finalize with bh disabled

[Halil Pasic <pasic@linux.ibm.com>]

On Wed, 27 Sep 2023 14:12:19 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, Sep 27 2023, Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Wed, 27 Sep 2023 12:08:43 +0200
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >  
> >> > On the other hand virtio_airq_handler() calls vring_interrupt() with
> >> > interrupts enabled. (While vring_interrupt() is called in a (read)
> >> > critical section in virtio_airq_handler() we use read_lock() and
> >> > not read_lock_irqsave() to grab the lock. Whether that is correct in
> >> > it self (i.e. disregarding the crypto problem) or not I'm not sure right
> >> > now. Will think some more about it tomorrow.) If the way to go forward
> >> > is disabling interrupts in virtio-ccw before vring_interrupt() is
> >> > called, I would be glad to spin a patch for that.    
> >> 
> >> virtio_airq_handler() is supposed to be an interrupt handler for an
> >> adapter interrupt -- as such I would expect it to always run with
> >> interrupts disabled (and I'd expect vring_interrupt() to be called
> >> with interrupts disabled as well; if that's not the case, I think it
> >> would need to run asynchronously.) At least that was my understanding at
> >> the time I wrote the code.  
> >
> > Thanks Connie! I don't quite understand what do you mean by "run with
> > interrupts disabled" in this context.
> >
> > Do you mean that if I were to add the following warning:
> >
> > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> > index ac67576301bf..2a9c73f5964f 100644
> > --- a/drivers/s390/virtio/virtio_ccw.c
> > +++ b/drivers/s390/virtio/virtio_ccw.c
> > @@ -211,6 +211,8 @@ static void virtio_airq_handler(struct airq_struct *airq,
> >         struct airq_info *info = container_of(airq, struct airq_info, airq);
> >         unsigned long ai;
> >  
> > +       WARN_ONCE(in_irq(), "irqs are ought to be disabled but are not\n");
> > +
> >         inc_irq_stat(IRQIO_VAI);
> >
> > it would/should never trigger, or do you mean something different?
> >
> > If yes, does that mean that you would expect the common airq code (i.e. something
> > like do_airq_interrupt()) to disable interrupts, or call virtio_airq_handler()?
> > asynchronously sort of as a bottom half (my understanding of bottom halves is currently
> > not complete).
> >
> > If no what do you actually mean?  
> 
> My understanding (at the time) was that we're coming from the low-level
> interrupt handler (which disables interrupts via the NEW PSW);
> interrupts will be re-enabled once the basic processing is done. This
> might no longer be the case, but I currently don't have the time to dig
> into the code -- it has been some time.
> 

It disables IO interrupts. I happen to have the PSW :) But AFAIU we may
still get machine check type interrupts.

So I'm leaning towards: the code is actually safe, but I will double
check again. But then what we do on s390x probably does not fit well
with Linux abstractions. AFAIU in Linux we don't have the granularity
"this lock is used in irq context but only IO irq context, so we don't
care that we may get interrupted by a non-IO irq"...

This complication is why I asked what do you mean by "run with
interrupts disabled", because this code does run with "IO interrupts
locally disabled, but not with *all* interrupts disabled".

I fully understand that you are bandwith limited. I'm adding
Peter and Vineeth, since this also concerns CIO. The easy fix for
this warning is to disable interrupts locally I guess.

In any case I will take care of this one way or another.

Thanks Conny! 

Regards,
Halil



_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization