On Tue, 11 Jun 2019 18:19:44 +0200
Cornelia Huck <cohuck@xxxxxxxxxx> wrote:
> On Tue, 11 Jun 2019 16:27:21 +0200
> Halil Pasic <pasic@xxxxxxxxxxxxx> wrote:
>
> > On Tue, 11 Jun 2019 12:17:21 +0200
> > Cornelia Huck <cohuck@xxxxxxxxxx> wrote:
> >
> > > On Thu, 6 Jun 2019 13:51:23 +0200
> > > Halil Pasic <pasic@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > > > Reviewed-by: Sebastian Ott <sebott@xxxxxxxxxxxxx>
> > > > ---
> > > > 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);
}
