From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Reply-To: mimu@linux.ibm.com Subject: Re: [PATCH v3 3/8] s390/cio: add basic protected virtualization support References: <20190529122657.166148-1-mimu@linux.ibm.com> <20190529122657.166148-4-mimu@linux.ibm.com> <20190603140649.7d5ebc3e.cohuck@redhat.com> From: Michael Mueller Date: Mon, 3 Jun 2019 14:45:03 +0200 MIME-Version: 1.0 In-Reply-To: <20190603140649.7d5ebc3e.cohuck@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Message-Id: <18348fed-07d1-a11f-215c-f09ac94e9fbf@linux.ibm.com> Sender: kvm-owner@vger.kernel.org List-Archive: List-Post: To: Cornelia Huck Cc: KVM Mailing List , Linux-S390 Mailing List , Sebastian Ott , Heiko Carstens , Halil Pasic , 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 , Pierre Morel List-ID: On 03.06.19 14:06, Cornelia Huck wrote: > On Wed, 29 May 2019 14:26:52 +0200 > Michael Mueller wrote: > >> From: Halil Pasic >> >> As virtio-ccw devices are channel devices, we need to use the dma area >> for any communication with the hypervisor. > > "we need to use the dma area within the common I/O layer for any > communication with the hypervisor. Note that we do not need to use that > area for control blocks directly referenced by instructions, e.g. the > orb." using this now > > ...although I'm still not particularly confident about the actual > distinction here? I'm trusting you that you actually have tested it, > though :) > >> >> It handles neither QDIO in the common code, nor any device type specific >> stuff (like channel programs constructed by the DASD driver). >> >> An interesting side effect is that virtio structures are now going to >> get allocated in 31 bit addressable storage. >> >> Signed-off-by: Halil Pasic >> Reviewed-by: Sebastian Ott >> Signed-off-by: Michael Mueller >> --- >> arch/s390/include/asm/ccwdev.h | 4 +++ >> drivers/s390/cio/ccwreq.c | 9 +++--- >> drivers/s390/cio/device.c | 67 +++++++++++++++++++++++++++++++++------- >> drivers/s390/cio/device_fsm.c | 49 +++++++++++++++++------------ >> drivers/s390/cio/device_id.c | 20 ++++++------ >> drivers/s390/cio/device_ops.c | 21 +++++++++++-- >> drivers/s390/cio/device_pgid.c | 22 +++++++------ >> drivers/s390/cio/device_status.c | 24 +++++++------- >> drivers/s390/cio/io_sch.h | 20 +++++++++--- >> drivers/s390/virtio/virtio_ccw.c | 10 ------ >> 10 files changed, 163 insertions(+), 83 deletions(-) > > (...) > >> @@ -1593,20 +1625,31 @@ struct ccw_device * __init ccw_device_create_console(struct ccw_driver *drv) >> return ERR_CAST(sch); >> >> io_priv = kzalloc(sizeof(*io_priv), GFP_KERNEL | GFP_DMA); >> - if (!io_priv) { >> - put_device(&sch->dev); >> - return ERR_PTR(-ENOMEM); >> - } >> + if (!io_priv) >> + goto err_priv; >> + io_priv->dma_area = dma_alloc_coherent(&sch->dev, >> + sizeof(*io_priv->dma_area), >> + &io_priv->dma_area_dma, GFP_KERNEL); >> + if (!io_priv->dma_area) >> + goto err_dma_area; >> set_io_private(sch, io_priv); >> cdev = io_subchannel_create_ccwdev(sch); >> if (IS_ERR(cdev)) { >> put_device(&sch->dev); >> + dma_free_coherent(&sch->dev, sizeof(*io_priv->dma_area), >> + io_priv->dma_area, io_priv->dma_area_dma); >> kfree(io_priv); > > > Shouldn't that branch do set_io_private(sch, NULL)? Not sure if any > code would make use of it, but it's probably better to clean out > references to freed objects. Added behind kfree(). I hope nobody asks for a separate patch. ;) > > >> return cdev; >> } >> cdev->drv = drv; >> ccw_device_set_int_class(cdev); >> return cdev; >> + >> +err_dma_area: >> + kfree(io_priv); >> +err_priv: >> + put_device(&sch->dev); >> + return ERR_PTR(-ENOMEM); >> } >> >> void __init ccw_device_destroy_console(struct ccw_device *cdev) > > With the reservations above, > Reviewed-by: Cornelia Huck > Thanks, Michael From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Mueller Subject: Re: [PATCH v3 3/8] s390/cio: add basic protected virtualization support Date: Mon, 3 Jun 2019 14:45:03 +0200 Message-ID: <18348fed-07d1-a11f-215c-f09ac94e9fbf@linux.ibm.com> References: <20190529122657.166148-1-mimu@linux.ibm.com> <20190529122657.166148-4-mimu@linux.ibm.com> <20190603140649.7d5ebc3e.cohuck@redhat.com> Reply-To: mimu@linux.ibm.com Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20190603140649.7d5ebc3e.cohuck@redhat.com> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: Cornelia Huck Cc: Vasily Gorbik , Linux-S390 Mailing List , Thomas Huth , Claudio Imbrenda , KVM Mailing List , Sebastian Ott , "Michael S . Tsirkin" , Pierre Morel , Farhan Ali , Heiko Carstens , Eric Farman , virtualization@lists.linux-foundation.org, Halil Pasic , Christian Borntraeger , Christoph Hellwig , Viktor Mihajlovski , Janosch Frank List-Id: virtualization@lists.linuxfoundation.org On 03.06.19 14:06, Cornelia Huck wrote: > On Wed, 29 May 2019 14:26:52 +0200 > Michael Mueller wrote: > >> From: Halil Pasic >> >> As virtio-ccw devices are channel devices, we need to use the dma area >> for any communication with the hypervisor. > > "we need to use the dma area within the common I/O layer for any > communication with the hypervisor. Note that we do not need to use that > area for control blocks directly referenced by instructions, e.g. the > orb." using this now > > ...although I'm still not particularly confident about the actual > distinction here? I'm trusting you that you actually have tested it, > though :) > >> >> It handles neither QDIO in the common code, nor any device type specific >> stuff (like channel programs constructed by the DASD driver). >> >> An interesting side effect is that virtio structures are now going to >> get allocated in 31 bit addressable storage. >> >> Signed-off-by: Halil Pasic >> Reviewed-by: Sebastian Ott >> Signed-off-by: Michael Mueller >> --- >> arch/s390/include/asm/ccwdev.h | 4 +++ >> drivers/s390/cio/ccwreq.c | 9 +++--- >> drivers/s390/cio/device.c | 67 +++++++++++++++++++++++++++++++++------- >> drivers/s390/cio/device_fsm.c | 49 +++++++++++++++++------------ >> drivers/s390/cio/device_id.c | 20 ++++++------ >> drivers/s390/cio/device_ops.c | 21 +++++++++++-- >> drivers/s390/cio/device_pgid.c | 22 +++++++------ >> drivers/s390/cio/device_status.c | 24 +++++++------- >> drivers/s390/cio/io_sch.h | 20 +++++++++--- >> drivers/s390/virtio/virtio_ccw.c | 10 ------ >> 10 files changed, 163 insertions(+), 83 deletions(-) > > (...) > >> @@ -1593,20 +1625,31 @@ struct ccw_device * __init ccw_device_create_console(struct ccw_driver *drv) >> return ERR_CAST(sch); >> >> io_priv = kzalloc(sizeof(*io_priv), GFP_KERNEL | GFP_DMA); >> - if (!io_priv) { >> - put_device(&sch->dev); >> - return ERR_PTR(-ENOMEM); >> - } >> + if (!io_priv) >> + goto err_priv; >> + io_priv->dma_area = dma_alloc_coherent(&sch->dev, >> + sizeof(*io_priv->dma_area), >> + &io_priv->dma_area_dma, GFP_KERNEL); >> + if (!io_priv->dma_area) >> + goto err_dma_area; >> set_io_private(sch, io_priv); >> cdev = io_subchannel_create_ccwdev(sch); >> if (IS_ERR(cdev)) { >> put_device(&sch->dev); >> + dma_free_coherent(&sch->dev, sizeof(*io_priv->dma_area), >> + io_priv->dma_area, io_priv->dma_area_dma); >> kfree(io_priv); > > > Shouldn't that branch do set_io_private(sch, NULL)? Not sure if any > code would make use of it, but it's probably better to clean out > references to freed objects. Added behind kfree(). I hope nobody asks for a separate patch. ;) > > >> return cdev; >> } >> cdev->drv = drv; >> ccw_device_set_int_class(cdev); >> return cdev; >> + >> +err_dma_area: >> + kfree(io_priv); >> +err_priv: >> + put_device(&sch->dev); >> + return ERR_PTR(-ENOMEM); >> } >> >> void __init ccw_device_destroy_console(struct ccw_device *cdev) > > With the reservations above, > Reviewed-by: Cornelia Huck > Thanks, Michael