From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 8 May 2019 23:08:09 +0200 From: Halil Pasic Subject: Re: [PATCH 06/10] s390/cio: add basic protected virtualization support In-Reply-To: References: <20190426183245.37939-1-pasic@linux.ibm.com> <20190426183245.37939-7-pasic@linux.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-Id: <20190508230809.6b0faaf7.pasic@linux.ibm.com> Sender: kvm-owner@vger.kernel.org List-Archive: List-Post: To: Sebastian Ott Cc: kvm@vger.kernel.org, linux-s390@vger.kernel.org, Cornelia Huck , Martin Schwidefsky , virtualization@lists.linux-foundation.org, "Michael S. Tsirkin" , Christoph Hellwig , Thomas Huth , Christian Borntraeger , Viktor Mihajlovski , Vasily Gorbik , Janosch Frank , Claudio Imbrenda , Farhan Ali , Eric Farman List-ID: On Wed, 8 May 2019 15:46:42 +0200 (CEST) Sebastian Ott wrote: > > On Fri, 26 Apr 2019, Halil Pasic wrote: > > static struct ccw_device * io_subchannel_allocate_dev(struct subchannel *sch) > > { > [..] > > + cdev->private = kzalloc(sizeof(struct ccw_device_private), > > + GFP_KERNEL | GFP_DMA); > > Do we still need GFP_DMA here (since we now have cdev->private->dma_area)? > We probably do not. I kept it GFP_DMA to keep changes to the minimum. Should changing this in your opinion be a part of this patch? > > @@ -1062,6 +1082,14 @@ static int io_subchannel_probe(struct subchannel *sch) > > if (!io_priv) > > goto out_schedule; > > > > + io_priv->dma_area = dma_alloc_coherent(&sch->dev, > > + sizeof(*io_priv->dma_area), > > + &io_priv->dma_area_dma, GFP_KERNEL); > > This needs GFP_DMA. Christoph already answered this one. Thanks Christoph! > You use a genpool for ccw_private->dma and not for iopriv->dma - looks > kinda inconsistent. > Please have a look at patch #9. A virtio-ccw device uses the genpool of it's ccw device (the pool from which ccw_private->dma is allicated) for the ccw stuff it needs to do. AFAICT for a subchannel device and its API all the DMA memory we need is iopriv->dma. So my thought was constructing a genpool for that would be an overkill. Are you comfortable with this answer, or should we change something? Regards, Halil