[v4,4/8] s390/airq: use DMA memory for adapter interrupts
diff mbox series

Message ID 20190606115127.55519-5-pasic@linux.ibm.com
State New
Headers show
Series
  • s390: virtio: support protected virtualization
Related show

Commit Message

Halil Pasic June 6, 2019, 11:51 a.m. UTC
Protected virtualization guests have to use shared pages for airq
notifier bit vectors, because hypervisor needs to write these bits.

Let us make sure we allocate DMA memory for the notifier bit vectors by
replacing the kmem_cache with a dma_cache and kalloc() with
cio_dma_zalloc().

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Reviewed-by: Sebastian Ott <sebott@linux.ibm.com>
---
 arch/s390/include/asm/airq.h |  2 ++
 drivers/s390/cio/airq.c      | 32 ++++++++++++++++++++------------
 drivers/s390/cio/cio.h       |  2 ++
 drivers/s390/cio/css.c       |  1 +
 4 files changed, 25 insertions(+), 12 deletions(-)

Comments

Cornelia Huck June 11, 2019, 10:17 a.m. UTC | #1
On Thu,  6 Jun 2019 13:51:23 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> Protected virtualization guests have to use shared pages for airq
> notifier bit vectors, because hypervisor needs to write these bits.
> 
> Let us make sure we allocate DMA memory for the notifier bit vectors by
> replacing the kmem_cache with a dma_cache and kalloc() with
> cio_dma_zalloc().
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Reviewed-by: Sebastian Ott <sebott@linux.ibm.com>
> ---
>  arch/s390/include/asm/airq.h |  2 ++
>  drivers/s390/cio/airq.c      | 32 ++++++++++++++++++++------------
>  drivers/s390/cio/cio.h       |  2 ++
>  drivers/s390/cio/css.c       |  1 +
>  4 files changed, 25 insertions(+), 12 deletions(-)
> 

(...)

> @@ -295,12 +303,12 @@ unsigned long airq_iv_scan(struct airq_iv *iv, unsigned long start,
>  }
>  EXPORT_SYMBOL(airq_iv_scan);
>  
> -static int __init airq_init(void)
> +int __init airq_init(void)
>  {
> -	airq_iv_cache = kmem_cache_create("airq_iv_cache", cache_line_size(),
> -					  cache_line_size(), 0, NULL);
> +	airq_iv_cache = dma_pool_create("airq_iv_cache", cio_get_dma_css_dev(),
> +					cache_line_size(),
> +					cache_line_size(), PAGE_SIZE);
>  	if (!airq_iv_cache)
>  		return -ENOMEM;

Sorry about not noticing that in the last iteration; but you may return
an error here if airq_iv_cache could not be allocated...

>  	return 0;
>  }
> -subsys_initcall(airq_init);

(...)

> diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c
> index 6fc91d534af1..7901c8ed3597 100644
> --- a/drivers/s390/cio/css.c
> +++ b/drivers/s390/cio/css.c
> @@ -1182,6 +1182,7 @@ static int __init css_bus_init(void)
>  	ret = cio_dma_pool_init();
>  	if (ret)
>  		goto out_unregister_pmn;
> +	airq_init();

...but don't check the return code here. Probably a pathological case,
but shouldn't you handle that error as well?

>  	css_init_done = 1;
>  
>  	/* Enable default isc for I/O subchannels. */
Halil Pasic June 11, 2019, 2:27 p.m. UTC | #2
On Tue, 11 Jun 2019 12:17:21 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Thu,  6 Jun 2019 13:51:23 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > Protected virtualization guests have to use shared pages for airq
> > notifier bit vectors, because hypervisor needs to write these bits.
> > 
> > Let us make sure we allocate DMA memory for the notifier bit vectors by
> > replacing the kmem_cache with a dma_cache and kalloc() with
> > cio_dma_zalloc().
> > 
> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > Reviewed-by: Sebastian Ott <sebott@linux.ibm.com>
> > ---
> >  arch/s390/include/asm/airq.h |  2 ++
> >  drivers/s390/cio/airq.c      | 32 ++++++++++++++++++++------------
> >  drivers/s390/cio/cio.h       |  2 ++
> >  drivers/s390/cio/css.c       |  1 +
> >  4 files changed, 25 insertions(+), 12 deletions(-)
> > 
> 
> (...)
> 
> > @@ -295,12 +303,12 @@ unsigned long airq_iv_scan(struct airq_iv *iv, unsigned long start,
> >  }
> >  EXPORT_SYMBOL(airq_iv_scan);
> >  
> > -static int __init airq_init(void)
> > +int __init airq_init(void)
> >  {
> > -	airq_iv_cache = ) "airq_iv_cache", cache_line_size(),
> > -					  cache_line_size(), 0, NULL);
> > +	airq_iv_cache = dma_pool_create("airq_iv_cache", cio_get_dma_css_dev(),
> > +					cache_line_size(),
> > +					cache_line_size(), PAGE_SIZE);
> >  	if (!airq_iv_cache)
> >  		return -ENOMEM;
> 
> Sorry about not noticing that in the last iteration; but you may return
> an error here if airq_iv_cache could not be allocated...
> 
> >  	return 0;
> >  }
> > -subsys_initcall(airq_init);
> 
> (...)
> 
> > diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c
> > index 6fc91d534af1..7901c8ed3597 100644
> > --- a/drivers/s390/cio/css.c
> > +++ b/drivers/s390/cio/css.c
> > @@ -1182,6 +1182,7 @@ static int __init css_bus_init(void)
> >  	ret = cio_dma_pool_init();
> >  	if (ret)
> >  		goto out_unregister_pmn;
> > +	airq_init();
> 
> ...but don't check the return code here. Probably a pathological case,
> but shouldn't you handle that error as well?
> 

Tricky business... The problem is that the airq stuff ain't 'private' to
the CIO subsystem (e.g. zPCI). I'm afraid failing to init css won't
really prevent all usages.

My first thought was, that this is more or less analogous to what we
had before. Namely kmem_cache_create() and dma_pool_create() should fail
under similar circumstances, and the return value of airq_init() was
ignored in do_initcall_level(). So I was like ignoring it seems to be
consistent with previous state.

But, ouch, there is a big difference! While kmem_cache_zalloc() seems
to tolerate the first argument (pointer to kmem_cache) being NULL the
dma_pool_zalloc() does not.

IMHO the cleanest thing to do at this stage is to check if the
airq_iv_cache is NULL and fail the allocation if it is (to preserve
previous behavior).

I would prefer having a separate discussion on eventually changing
the behavior (e.g. fail css initialization).

Connie, would that work with you? Thanks for spotting this!

Regards,
Halil

> >  	css_init_done = 1;
> >  
> >  	/* Enable default isc for I/O subchannels. */
>
Cornelia Huck June 11, 2019, 4:19 p.m. UTC | #3
On Tue, 11 Jun 2019 16:27:21 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Tue, 11 Jun 2019 12:17:21 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Thu,  6 Jun 2019 13:51:23 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> >   
> > > Protected virtualization guests have to use shared pages for airq
> > > notifier bit vectors, because hypervisor needs to write these bits.
> > > 
> > > Let us make sure we allocate DMA memory for the notifier bit vectors by
> > > replacing the kmem_cache with a dma_cache and kalloc() with
> > > cio_dma_zalloc().
> > > 
> > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > > Reviewed-by: Sebastian Ott <sebott@linux.ibm.com>
> > > ---
> > >  arch/s390/include/asm/airq.h |  2 ++
> > >  drivers/s390/cio/airq.c      | 32 ++++++++++++++++++++------------
> > >  drivers/s390/cio/cio.h       |  2 ++
> > >  drivers/s390/cio/css.c       |  1 +
> > >  4 files changed, 25 insertions(+), 12 deletions(-)
> > >   
> > 
> > (...)
> >   
> > > @@ -295,12 +303,12 @@ unsigned long airq_iv_scan(struct airq_iv *iv, unsigned long start,
> > >  }
> > >  EXPORT_SYMBOL(airq_iv_scan);
> > >  
> > > -static int __init airq_init(void)
> > > +int __init airq_init(void)
> > >  {
> > > -	airq_iv_cache = ) "airq_iv_cache", cache_line_size(),
> > > -					  cache_line_size(), 0, NULL);
> > > +	airq_iv_cache = dma_pool_create("airq_iv_cache", cio_get_dma_css_dev(),
> > > +					cache_line_size(),
> > > +					cache_line_size(), PAGE_SIZE);
> > >  	if (!airq_iv_cache)
> > >  		return -ENOMEM;  
> > 
> > Sorry about not noticing that in the last iteration; but you may return
> > an error here if airq_iv_cache could not be allocated...
> >   
> > >  	return 0;
> > >  }
> > > -subsys_initcall(airq_init);  
> > 
> > (...)
> >   
> > > diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c
> > > index 6fc91d534af1..7901c8ed3597 100644
> > > --- a/drivers/s390/cio/css.c
> > > +++ b/drivers/s390/cio/css.c
> > > @@ -1182,6 +1182,7 @@ static int __init css_bus_init(void)
> > >  	ret = cio_dma_pool_init();
> > >  	if (ret)
> > >  		goto out_unregister_pmn;
> > > +	airq_init();  
> > 
> > ...but don't check the return code here. Probably a pathological case,
> > but shouldn't you handle that error as well?
> >   
> 
> Tricky business... The problem is that the airq stuff ain't 'private' to
> the CIO subsystem (e.g. zPCI). I'm afraid failing to init css won't
> really prevent all usages.

Architecture-wise, there's an unfortunate tie-in of some things like
zPCI with the channel subsystem (most of that seems to come in via chsc
and machine checks; but as you say, airq as well). I'd basically
consider css to be a base system for virtually any I/O on s390...

> 
> My first thought was, that this is more or less analogous to what we
> had before. Namely kmem_cache_create() and dma_pool_create() should fail
> under similar circumstances, and the return value of airq_init() was
> ignored in do_initcall_level(). So I was like ignoring it seems to be
> consistent with previous state.
> 
> But, ouch, there is a big difference! While kmem_cache_zalloc() seems
> to tolerate the first argument (pointer to kmem_cache) being NULL the
> dma_pool_zalloc() does not.

Yeah. While previously continuing with a failed allocation simply was
not very workable, now we actually would end up with crashes :(

> 
> IMHO the cleanest thing to do at this stage is to check if the
> airq_iv_cache is NULL and fail the allocation if it is (to preserve
> previous behavior).

That's probably the least invasive fix for now. Did you check whether
any of the other dma pools this series introduces have a similar
problem due to init not failing?

> 
> I would prefer having a separate discussion on eventually changing
> the behavior (e.g. fail css initialization).

I did a quick check of the common I/O layer code and one place that
looks dangerous is the chsc initialization (where we get two pages that
are later accessed unconditionally by the code).

All of this is related to not being able to fulfill some basic memory
availability requirements early during boot and then discovering that
pulling the emergency break did not actually stop the train. I'd vote
for calling panic() if the common I/O layer cannot perform its setup;
but as this is really a pathological case I also think we should solve
that independently of this patch series.

> 
> Connie, would that work with you? Thanks for spotting this!

Yeah, let's give your approach a try.
Halil Pasic June 12, 2019, 12:32 a.m. UTC | #4
On Tue, 11 Jun 2019 18:19:44 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 11 Jun 2019 16:27:21 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Tue, 11 Jun 2019 12:17:21 +0200
> > Cornelia Huck <cohuck@redhat.com> wrote:
> > 
> > > On Thu,  6 Jun 2019 13:51:23 +0200
> > > Halil Pasic <pasic@linux.ibm.com> wrote:
> > >   
> > > > Protected virtualization guests have to use shared pages for airq
> > > > notifier bit vectors, because hypervisor needs to write these bits.
> > > > 
> > > > Let us make sure we allocate DMA memory for the notifier bit vectors by
> > > > replacing the kmem_cache with a dma_cache and kalloc() with
> > > > cio_dma_zalloc().
> > > > 
> > > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > > > Reviewed-by: Sebastian Ott <sebott@linux.ibm.com>
> > > > ---
> > > >  arch/s390/include/asm/airq.h |  2 ++
> > > >  drivers/s390/cio/airq.c      | 32 ++++++++++++++++++++------------
> > > >  drivers/s390/cio/cio.h       |  2 ++
> > > >  drivers/s390/cio/css.c       |  1 +
> > > >  4 files changed, 25 insertions(+), 12 deletions(-)
> > > >   
> > > 
> > > (...)
> > >   
> > > > @@ -295,12 +303,12 @@ unsigned long airq_iv_scan(struct airq_iv *iv, unsigned long start,
> > > >  }
> > > >  EXPORT_SYMBOL(airq_iv_scan);
> > > >  
> > > > -static int __init airq_init(void)
> > > > +int __init airq_init(void)
> > > >  {
> > > > -	airq_iv_cache = ) "airq_iv_cache", cache_line_size(),
> > > > -					  cache_line_size(), 0, NULL);
> > > > +	airq_iv_cache = dma_pool_create("airq_iv_cache", cio_get_dma_css_dev(),
> > > > +					cache_line_size(),
> > > > +					cache_line_size(), PAGE_SIZE);
> > > >  	if (!airq_iv_cache)
> > > >  		return -ENOMEM;  
> > > 
> > > Sorry about not noticing that in the last iteration; but you may return
> > > an error here if airq_iv_cache could not be allocated...
> > >   
> > > >  	return 0;
> > > >  }
> > > > -subsys_initcall(airq_init);  
> > > 
> > > (...)
> > >   
> > > > diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c
> > > > index 6fc91d534af1..7901c8ed3597 100644
> > > > --- a/drivers/s390/cio/css.c
> > > > +++ b/drivers/s390/cio/css.c
> > > > @@ -1182,6 +1182,7 @@ static int __init css_bus_init(void)
> > > >  	ret = cio_dma_pool_init();
> > > >  	if (ret)
> > > >  		goto out_unregister_pmn;
> > > > +	airq_init();  
> > > 
> > > ...but don't check the return code here. Probably a pathological case,
> > > but shouldn't you handle that error as well?
> > >   
> > 
> > Tricky business... The problem is that the airq stuff ain't 'private' to
> > the CIO subsystem (e.g. zPCI). I'm afraid failing to init css won't
> > really prevent all usages.
> 
> Architecture-wise, there's an unfortunate tie-in of some things like
> zPCI with the channel subsystem (most of that seems to come in via chsc
> and machine checks; but as you say, airq as well). I'd basically
> consider css to be a base system for virtually any I/O on s390...
> 

I tend to agree.

> > 
> > My first thought was, that this is more or less analogous to what we
> > had before. Namely kmem_cache_create() and dma_pool_create() should fail
> > under similar circumstances, and the return value of airq_init() was
> > ignored in do_initcall_level(). So I was like ignoring it seems to be
> > consistent with previous state.
> > 
> > But, ouch, there is a big difference! While kmem_cache_zalloc() seems
> > to tolerate the first argument (pointer to kmem_cache) being NULL the
> > dma_pool_zalloc() does not.
> 
> Yeah. While previously continuing with a failed allocation simply was
> not very workable, now we actually would end up with crashes :(
> 

Nod.

> > 
> > IMHO the cleanest thing to do at this stage is to check if the
> > airq_iv_cache is NULL and fail the allocation if it is (to preserve
> > previous behavior).
> 
> That's probably the least invasive fix for now. Did you check whether
> any of the other dma pools this series introduces have a similar
> problem due to init not failing?
>

Good question!

I did a quick check. virtio_ccw_init() should be OK, because we don't
register the driver if allocation fails, so the thing is going to end
up dysfunctional as expected.

If however cio_dma_pool_init() fails, then we end up with the same
problem with airqs, just on the !AIRQ_IV_CACHELINE code path. It can be
fixed analogously: make cio_dma_zalloc() fail all allocation if
cio_dma_pool_init() failed before.

The rest should be OK.

> > 
> > I would prefer having a separate discussion on eventually changing
> > the behavior (e.g. fail css initialization).
> 
> I did a quick check of the common I/O layer code and one place that
> looks dangerous is the chsc initialization (where we get two pages that
> are later accessed unconditionally by the code).
> 
> All of this is related to not being able to fulfill some basic memory
> availability requirements early during boot and then discovering that
> pulling the emergency break did not actually stop the train. I'd vote
> for calling panic() if the common I/O layer cannot perform its setup;
> but as this is really a pathological case I also think we should solve
> that independently of this patch series.
>

panic() sounds very reasonable to me. As an user I would like to see a
message that tells me, I'm trying to boot with insufficient RAM. Is there
such a message somewhere?
 
> > 
> > Connie, would that work with you? Thanks for spotting this!
> 
> Yeah, let's give your approach a try.
> 

OK. I intend to send out v5 with these changes tomorrow in the
afternoon:
 
diff --git a/drivers/s390/cio/airq.c b/drivers/s390/cio/airq.c
index 89d26e43004d..427b2e24a8ce 100644
--- a/drivers/s390/cio/airq.c
+++ b/drivers/s390/cio/airq.c
@@ -142,7 +142,8 @@ struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags)
        size = iv_size(bits);
 
        if (flags & AIRQ_IV_CACHELINE) {
-               if ((cache_line_size() * BITS_PER_BYTE) < bits)
+               if ((cache_line_size() * BITS_PER_BYTE) < bits
+                               || !airq_iv_cache)
                        goto out_free;
 
                iv->vector = dma_pool_zalloc(airq_iv_cache, GFP_KERNEL,
@@ -186,7 +187,7 @@ struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags)
        kfree(iv->ptr);
        kfree(iv->bitlock);
        kfree(iv->avail);
-       if (iv->flags & AIRQ_IV_CACHELINE)
+       if (iv->flags & AIRQ_IV_CACHELINE && iv->vector)
                dma_pool_free(airq_iv_cache, iv->vector, iv->vector_dma);
        else
                cio_dma_free(iv->vector, size);
diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c
index 7901c8ed3597..d709bd8545f2 100644
--- a/drivers/s390/cio/css.c
+++ b/drivers/s390/cio/css.c
@@ -1128,6 +1128,8 @@ void cio_gp_dma_free(struct gen_pool *gp_dma, void *cpu_addr, size_t size)
  */
 void *cio_dma_zalloc(size_t size)
 {
+       if (!cio_dma_pool)
+               return NULL;
        return cio_gp_dma_zalloc(cio_dma_pool, cio_get_dma_css_dev(), size);
 }
Cornelia Huck June 12, 2019, 6:21 a.m. UTC | #5
On Wed, 12 Jun 2019 02:32:31 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Tue, 11 Jun 2019 18:19:44 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Tue, 11 Jun 2019 16:27:21 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:

> > > IMHO the cleanest thing to do at this stage is to check if the
> > > airq_iv_cache is NULL and fail the allocation if it is (to preserve
> > > previous behavior).  
> > 
> > That's probably the least invasive fix for now. Did you check whether
> > any of the other dma pools this series introduces have a similar
> > problem due to init not failing?
> >  
> 
> Good question!
> 
> I did a quick check. virtio_ccw_init() should be OK, because we don't
> register the driver if allocation fails, so the thing is going to end
> up dysfunctional as expected.
> 
> If however cio_dma_pool_init() fails, then we end up with the same
> problem with airqs, just on the !AIRQ_IV_CACHELINE code path. It can be
> fixed analogously: make cio_dma_zalloc() fail all allocation if
> cio_dma_pool_init() failed before.

Ok, makes sense.

> 
> The rest should be OK.
> 
> > > 
> > > I would prefer having a separate discussion on eventually changing
> > > the behavior (e.g. fail css initialization).  
> > 
> > I did a quick check of the common I/O layer code and one place that
> > looks dangerous is the chsc initialization (where we get two pages that
> > are later accessed unconditionally by the code).
> > 
> > All of this is related to not being able to fulfill some basic memory
> > availability requirements early during boot and then discovering that
> > pulling the emergency break did not actually stop the train. I'd vote
> > for calling panic() if the common I/O layer cannot perform its setup;
> > but as this is really a pathological case I also think we should solve
> > that independently of this patch series.
> >  
> 
> panic() sounds very reasonable to me. As an user I would like to see a
> message that tells me, I'm trying to boot with insufficient RAM. Is there
> such a message somewhere?

You could add it in the panic() message :) I would not spend overly
much time on this, though, as this really sounds like someone is trying
to run on a system that is way too tiny memory-wise for doing anything
useful.

>  
> > > 
> > > Connie, would that work with you? Thanks for spotting this!  
> > 
> > Yeah, let's give your approach a try.
> >   
> 
> OK. I intend to send out v5 with these changes tomorrow in the
> afternoon:
>  
> diff --git a/drivers/s390/cio/airq.c b/drivers/s390/cio/airq.c
> index 89d26e43004d..427b2e24a8ce 100644
> --- a/drivers/s390/cio/airq.c
> +++ b/drivers/s390/cio/airq.c
> @@ -142,7 +142,8 @@ struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags)
>         size = iv_size(bits);
>  
>         if (flags & AIRQ_IV_CACHELINE) {
> -               if ((cache_line_size() * BITS_PER_BYTE) < bits)
> +               if ((cache_line_size() * BITS_PER_BYTE) < bits
> +                               || !airq_iv_cache)

It's perhaps a bit more readable if you keep checking for airq_iv_cache
on a separate if statement, but that's a matter of taste, I guess.

>                         goto out_free;
>  
>                 iv->vector = dma_pool_zalloc(airq_iv_cache, GFP_KERNEL,
> @@ -186,7 +187,7 @@ struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags)
>         kfree(iv->ptr);
>         kfree(iv->bitlock);
>         kfree(iv->avail);
> -       if (iv->flags & AIRQ_IV_CACHELINE)
> +       if (iv->flags & AIRQ_IV_CACHELINE && iv->vector)
>                 dma_pool_free(airq_iv_cache, iv->vector, iv->vector_dma);
>         else
>                 cio_dma_free(iv->vector, size);
> diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c
> index 7901c8ed3597..d709bd8545f2 100644
> --- a/drivers/s390/cio/css.c
> +++ b/drivers/s390/cio/css.c
> @@ -1128,6 +1128,8 @@ void cio_gp_dma_free(struct gen_pool *gp_dma, void *cpu_addr, size_t size)
>   */
>  void *cio_dma_zalloc(size_t size)
>  {
> +       if (!cio_dma_pool)
> +               return NULL;
>         return cio_gp_dma_zalloc(cio_dma_pool, cio_get_dma_css_dev(), size);
>  }
> 

Just looked at patch 2 again, will comment there.
Halil Pasic June 12, 2019, 1:33 p.m. UTC | #6
On Wed, 12 Jun 2019 08:21:27 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 12 Jun 2019 02:32:31 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Tue, 11 Jun 2019 18:19:44 +0200
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> > > On Tue, 11 Jun 2019 16:27:21 +0200
> > > Halil Pasic <pasic@linux.ibm.com> wrote:  
> 
> > > > IMHO the cleanest thing to do at this stage is to check if the
> > > > airq_iv_cache is NULL and fail the allocation if it is (to preserve
> > > > previous behavior).    
> > > 
> > > That's probably the least invasive fix for now. Did you check whether
> > > any of the other dma pools this series introduces have a similar
> > > problem due to init not failing?
> > >    
> > 
> > Good question!
> > 
> > I did a quick check. virtio_ccw_init() should be OK, because we don't
> > register the driver if allocation fails, so the thing is going to end
> > up dysfunctional as expected.
> > 
> > If however cio_dma_pool_init() fails, then we end up with the same
> > problem with airqs, just on the !AIRQ_IV_CACHELINE code path. It can be
> > fixed analogously: make cio_dma_zalloc() fail all allocation if
> > cio_dma_pool_init() failed before.  
> 
> Ok, makes sense.

v5 is out with the fixes. I have no ack/r-b from you for patch 4. Would
you like to give some, or should I proceed without?

Regards,
Halil
Cornelia Huck June 12, 2019, 1:46 p.m. UTC | #7
On Wed, 12 Jun 2019 15:33:24 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Wed, 12 Jun 2019 08:21:27 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Wed, 12 Jun 2019 02:32:31 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> >   
> > > On Tue, 11 Jun 2019 18:19:44 +0200
> > > Cornelia Huck <cohuck@redhat.com> wrote:
> > >     
> > > > On Tue, 11 Jun 2019 16:27:21 +0200
> > > > Halil Pasic <pasic@linux.ibm.com> wrote:    
> >   
> > > > > IMHO the cleanest thing to do at this stage is to check if the
> > > > > airq_iv_cache is NULL and fail the allocation if it is (to preserve
> > > > > previous behavior).      
> > > > 
> > > > That's probably the least invasive fix for now. Did you check whether
> > > > any of the other dma pools this series introduces have a similar
> > > > problem due to init not failing?
> > > >      
> > > 
> > > Good question!
> > > 
> > > I did a quick check. virtio_ccw_init() should be OK, because we don't
> > > register the driver if allocation fails, so the thing is going to end
> > > up dysfunctional as expected.
> > > 
> > > If however cio_dma_pool_init() fails, then we end up with the same
> > > problem with airqs, just on the !AIRQ_IV_CACHELINE code path. It can be
> > > fixed analogously: make cio_dma_zalloc() fail all allocation if
> > > cio_dma_pool_init() failed before.    
> > 
> > Ok, makes sense.  
> 
> v5 is out with the fixes. I have no ack/r-b from you for patch 4. Would
> you like to give some, or should I proceed without?

Please give this some more time, I will look at this later.

Patch
diff mbox series

diff --git a/arch/s390/include/asm/airq.h b/arch/s390/include/asm/airq.h
index c10d2ee2dfda..01936fdfaddb 100644
--- a/arch/s390/include/asm/airq.h
+++ b/arch/s390/include/asm/airq.h
@@ -11,6 +11,7 @@ 
 #define _ASM_S390_AIRQ_H
 
 #include <linux/bit_spinlock.h>
+#include <linux/dma-mapping.h>
 
 struct airq_struct {
 	struct hlist_node list;		/* Handler queueing. */
@@ -29,6 +30,7 @@  void unregister_adapter_interrupt(struct airq_struct *airq);
 /* Adapter interrupt bit vector */
 struct airq_iv {
 	unsigned long *vector;	/* Adapter interrupt bit vector */
+	dma_addr_t vector_dma; /* Adapter interrupt bit vector dma */
 	unsigned long *avail;	/* Allocation bit mask for the bit vector */
 	unsigned long *bitlock;	/* Lock bit mask for the bit vector */
 	unsigned long *ptr;	/* Pointer associated with each bit */
diff --git a/drivers/s390/cio/airq.c b/drivers/s390/cio/airq.c
index 4534afc63591..89d26e43004d 100644
--- a/drivers/s390/cio/airq.c
+++ b/drivers/s390/cio/airq.c
@@ -16,9 +16,11 @@ 
 #include <linux/mutex.h>
 #include <linux/rculist.h>
 #include <linux/slab.h>
+#include <linux/dmapool.h>
 
 #include <asm/airq.h>
 #include <asm/isc.h>
+#include <asm/cio.h>
 
 #include "cio.h"
 #include "cio_debug.h"
@@ -27,7 +29,7 @@ 
 static DEFINE_SPINLOCK(airq_lists_lock);
 static struct hlist_head airq_lists[MAX_ISC+1];
 
-static struct kmem_cache *airq_iv_cache;
+static struct dma_pool *airq_iv_cache;
 
 /**
  * register_adapter_interrupt() - register adapter interrupt handler
@@ -115,6 +117,11 @@  void __init init_airq_interrupts(void)
 	setup_irq(THIN_INTERRUPT, &airq_interrupt);
 }
 
+static inline unsigned long iv_size(unsigned long bits)
+{
+	return BITS_TO_LONGS(bits) * sizeof(unsigned long);
+}
+
 /**
  * airq_iv_create - create an interrupt vector
  * @bits: number of bits in the interrupt vector
@@ -132,17 +139,18 @@  struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags)
 		goto out;
 	iv->bits = bits;
 	iv->flags = flags;
-	size = BITS_TO_LONGS(bits) * sizeof(unsigned long);
+	size = iv_size(bits);
 
 	if (flags & AIRQ_IV_CACHELINE) {
 		if ((cache_line_size() * BITS_PER_BYTE) < bits)
 			goto out_free;
 
-		iv->vector = kmem_cache_zalloc(airq_iv_cache, GFP_KERNEL);
+		iv->vector = dma_pool_zalloc(airq_iv_cache, GFP_KERNEL,
+					     &iv->vector_dma);
 		if (!iv->vector)
 			goto out_free;
 	} else {
-		iv->vector = kzalloc(size, GFP_KERNEL);
+		iv->vector = cio_dma_zalloc(size);
 		if (!iv->vector)
 			goto out_free;
 	}
@@ -179,9 +187,9 @@  struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags)
 	kfree(iv->bitlock);
 	kfree(iv->avail);
 	if (iv->flags & AIRQ_IV_CACHELINE)
-		kmem_cache_free(airq_iv_cache, iv->vector);
+		dma_pool_free(airq_iv_cache, iv->vector, iv->vector_dma);
 	else
-		kfree(iv->vector);
+		cio_dma_free(iv->vector, size);
 	kfree(iv);
 out:
 	return NULL;
@@ -198,9 +206,9 @@  void airq_iv_release(struct airq_iv *iv)
 	kfree(iv->ptr);
 	kfree(iv->bitlock);
 	if (iv->flags & AIRQ_IV_CACHELINE)
-		kmem_cache_free(airq_iv_cache, iv->vector);
+		dma_pool_free(airq_iv_cache, iv->vector, iv->vector_dma);
 	else
-		kfree(iv->vector);
+		cio_dma_free(iv->vector, iv_size(iv->bits));
 	kfree(iv->avail);
 	kfree(iv);
 }
@@ -295,12 +303,12 @@  unsigned long airq_iv_scan(struct airq_iv *iv, unsigned long start,
 }
 EXPORT_SYMBOL(airq_iv_scan);
 
-static int __init airq_init(void)
+int __init airq_init(void)
 {
-	airq_iv_cache = kmem_cache_create("airq_iv_cache", cache_line_size(),
-					  cache_line_size(), 0, NULL);
+	airq_iv_cache = dma_pool_create("airq_iv_cache", cio_get_dma_css_dev(),
+					cache_line_size(),
+					cache_line_size(), PAGE_SIZE);
 	if (!airq_iv_cache)
 		return -ENOMEM;
 	return 0;
 }
-subsys_initcall(airq_init);
diff --git a/drivers/s390/cio/cio.h b/drivers/s390/cio/cio.h
index 06a91743335a..4d6c7d16416e 100644
--- a/drivers/s390/cio/cio.h
+++ b/drivers/s390/cio/cio.h
@@ -135,6 +135,8 @@  extern int cio_commit_config(struct subchannel *sch);
 int cio_tm_start_key(struct subchannel *sch, struct tcw *tcw, u8 lpm, u8 key);
 int cio_tm_intrg(struct subchannel *sch);
 
+extern int __init airq_init(void);
+
 /* Use with care. */
 #ifdef CONFIG_CCW_CONSOLE
 extern struct subchannel *cio_probe_console(void);
diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c
index 6fc91d534af1..7901c8ed3597 100644
--- a/drivers/s390/cio/css.c
+++ b/drivers/s390/cio/css.c
@@ -1182,6 +1182,7 @@  static int __init css_bus_init(void)
 	ret = cio_dma_pool_init();
 	if (ret)
 		goto out_unregister_pmn;
+	airq_init();
 	css_init_done = 1;
 
 	/* Enable default isc for I/O subchannels. */