From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759955AbdKPPhi (ORCPT ); Thu, 16 Nov 2017 10:37:38 -0500 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:41766 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759907AbdKPPh0 (ORCPT ); Thu, 16 Nov 2017 10:37:26 -0500 Subject: Re: [RFC 06/19] s390/zcrypt: register matrix device with VFIO mediated device framework To: Cornelia Huck Cc: linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, freude@de.ibm.com, schwidefsky@de.ibm.com, heiko.carstens@de.ibm.com, borntraeger@de.ibm.com, kwankhede@nvidia.com, bjsdjshi@linux.vnet.ibm.com, pbonzini@redhat.com, alex.williamson@redhat.com, pmorel@linux.vnet.ibm.com, alifm@linux.vnet.ibm.com, mjrosato@linux.vnet.ibm.com, qemu-s390x@nongnu.org, jjherne@linux.vnet.ibm.com, thuth@redhat.com, pasic@linux.vnet.ibm.com References: <1507916344-3896-1-git-send-email-akrowiak@linux.vnet.ibm.com> <1507916344-3896-7-git-send-email-akrowiak@linux.vnet.ibm.com> <20171114141421.584811d9.cohuck@redhat.com> From: Tony Krowiak Date: Thu, 16 Nov 2017 10:37:15 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: <20171114141421.584811d9.cohuck@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-TM-AS-GCONF: 00 x-cbid: 17111615-0028-0000-0000-000008A95102 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00008074; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000240; SDB=6.00946748; UDB=6.00477924; IPR=6.00727039; BA=6.00005694; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00018046; XFM=3.00000015; UTC=2017-11-16 15:37:22 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17111615-0029-0000-0000-0000385B2565 Message-Id: <9504121b-c296-ba2d-c7d7-d95fa904b533@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-11-16_06:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1709140000 definitions=main-1711160210 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/14/2017 08:14 AM, Cornelia Huck wrote: > On Fri, 13 Oct 2017 13:38:51 -0400 > Tony Krowiak wrote: > >> Registers the matrix device created by the AP matrix bus with >> the VFIO mediated device framework. Registering the matrix >> device will create the sysfs structures needed to create >> mediated matrix devices that can be configured with a matrix >> of adapters, usage domains and control domains for a guest >> machine. >> >> Registering the matrix device with the VFIO mediated device >> framework will create the following sysfs structures: >> >> /sys/devices/ap_matrix >> ... [matrix] >> ...... [mdev_supported_types] >> ......... [ap_matrix-passthrough] >> ............ available_instances >> ............ create >> ............ device_api >> ............ [devices] >> ............ name >> >> To create a mediated device for the AP matrix device, write a UUID >> to the create file: >> >> uuidgen > create >> >> A symbolic link to the mediated device's directory will be created in the >> devices subdirectory named after the generated $uuid: >> >> /sys/devices/ap_matrix >> ... [matrix] >> ...... [mdev_supported_types] >> ......... [ap_matrix-passthrough] >> ............ [devices] >> ............... [$uuid] >> >> Signed-off-by: Tony Krowiak >> --- >> MAINTAINERS | 1 + >> drivers/s390/crypto/Makefile | 2 +- >> drivers/s390/crypto/ap_matrix_bus.c | 1 + >> drivers/s390/crypto/ap_matrix_bus.h | 4 + >> drivers/s390/crypto/vfio_ap_matrix_drv.c | 5 + >> drivers/s390/crypto/vfio_ap_matrix_ops.c | 172 ++++++++++++++++++++++++++ >> drivers/s390/crypto/vfio_ap_matrix_private.h | 3 + >> include/uapi/linux/vfio.h | 1 + >> 8 files changed, 188 insertions(+), 1 deletions(-) >> create mode 100644 drivers/s390/crypto/vfio_ap_matrix_ops.c >> >> diff --git a/drivers/s390/crypto/ap_matrix_bus.c b/drivers/s390/crypto/ap_matrix_bus.c >> index 66bfa54..418c23b 100644 >> --- a/drivers/s390/crypto/ap_matrix_bus.c >> +++ b/drivers/s390/crypto/ap_matrix_bus.c >> @@ -61,6 +61,7 @@ static int ap_matrix_dev_create(void) >> matrix->device.bus = &ap_matrix_bus_type; >> matrix->device.parent = ap_matrix_root_device; >> matrix->device.release = ap_matrix_dev_release; >> + INIT_LIST_HEAD(&matrix->queues); > Don't you also need to init the spin lock? Yes, but note that I am getting rid of the AP matrix bus - as we discussed earlier - so this code will be moved to the AP matrix driver. > >> >> ret = device_register(&matrix->device); >> if (ret) { >> diff --git a/drivers/s390/crypto/ap_matrix_bus.h b/drivers/s390/crypto/ap_matrix_bus.h >> index c2aff23..3eccc36 100644 >> --- a/drivers/s390/crypto/ap_matrix_bus.h >> +++ b/drivers/s390/crypto/ap_matrix_bus.h >> @@ -12,8 +12,12 @@ >> >> #include >> >> +#include "ap_bus.h" > Why do you need this include now? (IOW, does that need to go into > another patch?) It is not needed here, but as discussed earlier, the AP matrix bus is going away. > >> + >> struct ap_matrix { >> struct device device; >> + spinlock_t qlock; >> + struct list_head queues; >> }; >> >> struct ap_matrix *ap_matrix_get_device(void); >> diff --git a/drivers/s390/crypto/vfio_ap_matrix_ops.c b/drivers/s390/crypto/vfio_ap_matrix_ops.c >> new file mode 100644 >> index 0000000..7d01f18 >> --- /dev/null >> +++ b/drivers/s390/crypto/vfio_ap_matrix_ops.c >> @@ -0,0 +1,172 @@ >> +/* >> + * Adjunct processor matrix VFIO device driver callbacks. >> + * >> + * Copyright IBM Corp. 2017 >> + * Author(s): Tony Krowiak >> + * >> + */ >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include "vfio_ap_matrix_private.h" >> + >> +#define AP_MATRIX_MDEV_TYPE_HWVIRT "passthrough" >> +#define AP_MATRIX_MDEV_NAME_HWVIRT "AP Matrix Passthrough Device" >> + >> +#define AP_MATRIX_MAX_AVAILABLE_INSTANCES 255 >> + >> +struct ap_matrix_mdev { >> + struct mdev_device *mdev; >> + struct list_head node; >> +}; >> + >> +struct ap_matrix *matrix; >> +struct mutex mdev_devices_lock; >> +struct list_head mdev_devices; >> +int available_instances; > Having this as a global variable feels wrong. I see that vfio-ccw keeps > things like this in driver data attached to its parent. Can you do > something like that as well? I am moving all of these variables into struct ap_matrix. The matrix device will be stored as driver data for the device so it will be retrievable by the mdev functions. This will eliminate the need for these globals and also omit the need to store a reference to the matrix device in multiple places. > >> + >> +static struct ap_matrix_mdev *ap_matrix_mdev_find_by_uuid(uuid_le uuid) >> +{ >> + struct ap_matrix_mdev *matrix_mdev; >> + >> + list_for_each_entry(matrix_mdev, &mdev_devices, node) { >> + if (uuid_le_cmp(mdev_uuid(matrix_mdev->mdev), uuid) == 0) >> + return matrix_mdev; >> + } >> + >> + return NULL; >> +} > (...) > >> +int ap_matrix_mdev_register(struct ap_matrix *ap_matrix) >> +{ >> + int ret; >> + struct device *dev = &ap_matrix->device; >> + >> + ret = mdev_register_device(dev, &vfio_ap_matrix_ops); >> + if (ret) >> + return ret; >> + >> + matrix = ap_matrix; > I'd just grab the (one) matrix device here... I am changing this so the matrix device will be stored as driver data with the device: dev_set_drvdata(dev) > >> + mutex_init(&mdev_devices_lock); >> + INIT_LIST_HEAD(&mdev_devices); >> + available_instances = AP_MATRIX_MAX_AVAILABLE_INSTANCES; >> + >> + return 0; >> +} >> + >> +void ap_matrix_mdev_unregister(struct ap_matrix *ap_matrix) >> +{ >> + struct device *dev; >> + >> + if (ap_matrix == matrix) { > ...and lose this extra if. It is going bye bye. > >> + dev = &matrix->device; >> + available_instances--; >> + mdev_unregister_device(dev); >> + } >> +}