Re: [PATCH v3 3/8] s390/cio: add basic protected virtualization support

Michael Mueller
2019-06-03T12:45:03Z

On 03.06.19 14:06, Cornelia Huck wrote:
> On Wed, 29 May 2019 14:26:52 +0200
> Michael Mueller <mimu@linux.ibm.com> wrote:
>
>> From: Halil Pasic <pasic@linux.ibm.com>
>>
>> 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 <pasic@linux.ibm.com>
>> Reviewed-by: Sebastian Ott <sebott@linux.ibm.com>
>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>> ---
>> 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);
>
> <pre-existing, not introduced by this patch>
> 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. ;)

> </pre-existing, not introduced by this 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 <cohuck@redhat.com>
>

Thanks,
Michael