From: Harald Freudenberger <freude@linux.ibm.com>
To: Tony Krowiak <akrowiak@linux.ibm.com>,
linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
kvm@vger.kernel.org
Cc: jjherne@linux.ibm.com, borntraeger@de.ibm.com, cohuck@redhat.com,
mjrosato@linux.ibm.com, pasic@linux.ibm.com,
alex.williamson@redhat.com, kwankhede@nvidia.com,
fiuczy@linux.ibm.com
Subject: Re: [PATCH v17 11/15] s390/ap: driver callback to indicate resource in use
Date: Thu, 4 Nov 2021 12:27:17 +0100 [thread overview]
Message-ID: <15a87038-a88c-b29e-f7d7-760ca27c87cf@linux.ibm.com> (raw)
In-Reply-To: <20211021152332.70455-12-akrowiak@linux.ibm.com>
On 21.10.21 17:23, Tony Krowiak wrote:
> Introduces a new driver callback to prevent a root user from re-assigning
> the APQN of a queue that is in use by a non-default host device driver to
> a default host device driver and vice versa. The callback will be invoked
> whenever a change to the AP bus's sysfs apmask or aqmask attributes would
> result in one or more APQNs being re-assigned. If the callback responds
> in the affirmative for any driver queried, the change to the apmask or
> aqmask will be rejected with a device busy error.
>
> For this patch, only non-default drivers will be queried. Currently,
> there is only one non-default driver, the vfio_ap device driver. The
> vfio_ap device driver facilitates pass-through of an AP queue to a
> guest. The idea here is that a guest may be administered by a different
> sysadmin than the host and we don't want AP resources to unexpectedly
> disappear from a guest's AP configuration (i.e., adapters and domains
> assigned to the matrix mdev). This will enforce the proper procedure for
> removing AP resources intended for guest usage which is to
> first unassign them from the matrix mdev, then unbind them from the
> vfio_ap device driver.
>
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> Reviewed-by: Harald Freudenberger <freude@linux.ibm.com>
> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
> ---
> drivers/s390/crypto/ap_bus.c | 160 ++++++++++++++++++++++++++++++++---
> drivers/s390/crypto/ap_bus.h | 4 +
> 2 files changed, 154 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
> index d9b804943d19..15886610f61a 100644
> --- a/drivers/s390/crypto/ap_bus.c
> +++ b/drivers/s390/crypto/ap_bus.c
> @@ -36,6 +36,7 @@
> #include <linux/mod_devicetable.h>
> #include <linux/debugfs.h>
> #include <linux/ctype.h>
> +#include <linux/module.h>
>
> #include "ap_bus.h"
> #include "ap_debug.h"
> @@ -1060,6 +1061,23 @@ static int modify_bitmap(const char *str, unsigned long *bitmap, int bits)
> return 0;
> }
>
> +static int ap_parse_bitmap_str(const char *str, unsigned long *bitmap, int bits,
> + unsigned long *newmap)
> +{
> + unsigned long size;
> + int rc;
> +
> + size = BITS_TO_LONGS(bits) * sizeof(unsigned long);
> + if (*str == '+' || *str == '-') {
> + memcpy(newmap, bitmap, size);
> + rc = modify_bitmap(str, newmap, bits);
> + } else {
> + memset(newmap, 0, size);
> + rc = hex2bitmap(str, newmap, bits);
> + }
> + return rc;
> +}
> +
> int ap_parse_mask_str(const char *str,
> unsigned long *bitmap, int bits,
> struct mutex *lock)
> @@ -1079,14 +1097,7 @@ int ap_parse_mask_str(const char *str,
> kfree(newmap);
> return -ERESTARTSYS;
> }
> -
> - if (*str == '+' || *str == '-') {
> - memcpy(newmap, bitmap, size);
> - rc = modify_bitmap(str, newmap, bits);
> - } else {
> - memset(newmap, 0, size);
> - rc = hex2bitmap(str, newmap, bits);
> - }
> + rc = ap_parse_bitmap_str(str, bitmap, bits, newmap);
> if (rc == 0)
> memcpy(bitmap, newmap, size);
> mutex_unlock(lock);
> @@ -1278,12 +1289,76 @@ static ssize_t apmask_show(struct bus_type *bus, char *buf)
> return rc;
> }
>
> +static int __verify_card_reservations(struct device_driver *drv, void *data)
> +{
> + int rc = 0;
> + struct ap_driver *ap_drv = to_ap_drv(drv);
> + unsigned long *newapm = (unsigned long *)data;
> +
> + /*
> + * No need to verify whether the driver is using the queues if it is the
> + * default driver.
> + */
> + if (ap_drv->flags & AP_DRIVER_FLAG_DEFAULT)
> + return 0;
> +
> + /*
> + * increase the driver's module refcounter to be sure it is not
> + * going away when we invoke the callback function.
> + */
> + if (!try_module_get(drv->owner))
> + return 0;
> +
> + if (ap_drv->in_use) {
> + rc = ap_drv->in_use(newapm, ap_perms.aqm);
> + if (rc)
> + rc = -EBUSY;
> + }
> +
> + /* release the driver's module */
> + module_put(drv->owner);
> +
> + return rc;
> +}
> +
> +static int apmask_commit(unsigned long *newapm)
> +{
> + int rc;
> + unsigned long reserved[BITS_TO_LONGS(AP_DEVICES)];
> +
> + /*
> + * Check if any bits in the apmask have been set which will
> + * result in queues being removed from non-default drivers
> + */
> + if (bitmap_andnot(reserved, newapm, ap_perms.apm, AP_DEVICES)) {
> + rc = bus_for_each_drv(&ap_bus_type, NULL, reserved,
> + __verify_card_reservations);
> + if (rc)
> + return rc;
> + }
> +
> + memcpy(ap_perms.apm, newapm, APMASKSIZE);
> +
> + return 0;
> +}
> +
> static ssize_t apmask_store(struct bus_type *bus, const char *buf,
> size_t count)
> {
> int rc;
> + DECLARE_BITMAP(newapm, AP_DEVICES);
>
> - rc = ap_parse_mask_str(buf, ap_perms.apm, AP_DEVICES, &ap_perms_mutex);
> + if (mutex_lock_interruptible(&ap_perms_mutex))
> + return -ERESTARTSYS;
> +
> + rc = ap_parse_bitmap_str(buf, ap_perms.apm, AP_DEVICES, newapm);
> + if (rc)
> + goto done;
> +
> + rc = apmask_commit(newapm);
> +
> +done:
> + mutex_unlock(&ap_perms_mutex);
> if (rc)
> return rc;
>
> @@ -1309,12 +1384,77 @@ static ssize_t aqmask_show(struct bus_type *bus, char *buf)
> return rc;
> }
>
> +static int __verify_queue_reservations(struct device_driver *drv, void *data)
> +{
> + int rc = 0;
> + struct ap_driver *ap_drv = to_ap_drv(drv);
> + unsigned long *newaqm = (unsigned long *)data;
> +
> + /*
> + * If the reserved bits do not identify queues reserved for use by the
> + * non-default driver, there is no need to verify the driver is using
> + * the queues.
> + */
> + if (ap_drv->flags & AP_DRIVER_FLAG_DEFAULT)
> + return 0;
> +
> + /*
> + * increase the driver's module refcounter to be sure it is not
> + * going away when we invoke the callback function.
> + */
> + if (!try_module_get(drv->owner))
> + return 0;
> +
> + if (ap_drv->in_use) {
> + rc = ap_drv->in_use(ap_perms.apm, newaqm);
> + if (rc)
> + return -EBUSY;
> + }
> +
> + /* release the driver's module */
> + module_put(drv->owner);
> +
> + return rc;
> +}
> +
> +static int aqmask_commit(unsigned long *newaqm)
> +{
> + int rc;
> + unsigned long reserved[BITS_TO_LONGS(AP_DOMAINS)];
> +
> + /*
> + * Check if any bits in the aqmask have been set which will
> + * result in queues being removed from non-default drivers
> + */
> + if (bitmap_andnot(reserved, newaqm, ap_perms.aqm, AP_DOMAINS)) {
> + rc = bus_for_each_drv(&ap_bus_type, NULL, reserved,
> + __verify_queue_reservations);
> + if (rc)
> + return rc;
> + }
> +
> + memcpy(ap_perms.aqm, newaqm, AQMASKSIZE);
> +
> + return 0;
> +}
> +
> static ssize_t aqmask_store(struct bus_type *bus, const char *buf,
> size_t count)
> {
> int rc;
> + DECLARE_BITMAP(newaqm, AP_DOMAINS);
> +
> + if (mutex_lock_interruptible(&ap_perms_mutex))
> + return -ERESTARTSYS;
> +
> + rc = ap_parse_bitmap_str(buf, ap_perms.aqm, AP_DOMAINS, newaqm);
> + if (rc)
> + goto done;
> +
> + rc = aqmask_commit(newaqm);
>
> - rc = ap_parse_mask_str(buf, ap_perms.aqm, AP_DOMAINS, &ap_perms_mutex);
> +done:
> + mutex_unlock(&ap_perms_mutex);
> if (rc)
> return rc;
>
> diff --git a/drivers/s390/crypto/ap_bus.h b/drivers/s390/crypto/ap_bus.h
> index 95b577754b35..67c1bef60ad5 100644
> --- a/drivers/s390/crypto/ap_bus.h
> +++ b/drivers/s390/crypto/ap_bus.h
> @@ -142,6 +142,7 @@ struct ap_driver {
>
> int (*probe)(struct ap_device *);
> void (*remove)(struct ap_device *);
> + int (*in_use)(unsigned long *apm, unsigned long *aqm);
> };
>
> #define to_ap_drv(x) container_of((x), struct ap_driver, driver)
> @@ -289,6 +290,9 @@ void ap_queue_init_state(struct ap_queue *aq);
> struct ap_card *ap_card_create(int id, int queue_depth, int raw_type,
> int comp_type, unsigned int functions, int ml);
>
> +#define APMASKSIZE (BITS_TO_LONGS(AP_DEVICES) * sizeof(unsigned long))
> +#define AQMASKSIZE (BITS_TO_LONGS(AP_DOMAINS) * sizeof(unsigned long))
> +
> struct ap_perms {
> unsigned long ioctlm[BITS_TO_LONGS(AP_IOCTLS)];
> unsigned long apm[BITS_TO_LONGS(AP_DEVICES)];
reviewed again. I still don't like this as it introduces an unbalanced weighting for the
vfio dd but ... We could consider removing the
if (ap_drv->flags & AP_DRIVER_FLAG_DEFAULT)
return 0;
in function __verify_queue_reservations. It would still work as the 'default device
drivers' do not implement the in_use() callback and thus do not disagree about
the upcoming change.
next prev parent reply other threads:[~2021-11-04 11:27 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-21 15:23 [PATCH v17 00/15] s390/vfio-ap: dynamic configuration support Tony Krowiak
2021-10-21 15:23 ` [PATCH v17 01/15] s390/vfio-ap: Set pqap hook when vfio_ap module is loaded Tony Krowiak
2021-12-27 8:21 ` Halil Pasic
2022-01-11 21:13 ` Tony Krowiak
2022-01-04 16:22 ` Jason J. Herne
2022-01-11 17:29 ` Tony Krowiak
2021-10-21 15:23 ` [PATCH v17 02/15] s390/vfio-ap: use new AP bus interface to search for queue devices Tony Krowiak
2021-12-27 8:25 ` Halil Pasic
2022-01-11 17:32 ` Tony Krowiak
2021-10-21 15:23 ` [PATCH v17 03/15] s390/vfio-ap: move probe and remove callbacks to vfio_ap_ops.c Tony Krowiak
2021-10-21 15:23 ` [PATCH v17 04/15] s390/vfio-ap: manage link between queue struct and matrix mdev Tony Krowiak
2021-10-21 15:23 ` [PATCH v17 05/15] s390/vfio-ap: introduce shadow APCB Tony Krowiak
2021-10-21 15:23 ` [PATCH v17 06/15] s390/vfio-ap: refresh guest's APCB by filtering APQNs assigned to mdev Tony Krowiak
2021-12-27 8:53 ` Halil Pasic
2022-01-11 21:19 ` Tony Krowiak
2022-01-12 11:52 ` Halil Pasic
2022-01-15 0:31 ` Tony Krowiak
2021-10-21 15:23 ` [PATCH v17 07/15] s390/vfio-ap: allow assignment of unavailable AP queues to mdev device Tony Krowiak
2021-12-27 9:06 ` Halil Pasic
2021-10-21 15:23 ` [PATCH v17 08/15] s390/vfio-ap: keep track of active guests Tony Krowiak
2021-12-30 2:04 ` Halil Pasic
2022-01-11 21:27 ` Tony Krowiak
2021-12-30 3:33 ` Halil Pasic
2022-01-11 21:58 ` Tony Krowiak
2022-01-11 22:19 ` Tony Krowiak
2022-01-12 14:25 ` Halil Pasic
2022-01-15 0:29 ` Tony Krowiak
2021-10-21 15:23 ` [PATCH v17 09/15] s390/vfio-ap: allow hot plug/unplug of AP resources using mdev device Tony Krowiak
2022-01-09 21:36 ` Halil Pasic
2022-01-11 22:42 ` Tony Krowiak
2021-10-21 15:23 ` [PATCH v17 10/15] s390/vfio-ap: reset queues after adapter/domain unassignment Tony Krowiak
2021-10-21 15:23 ` [PATCH v17 11/15] s390/ap: driver callback to indicate resource in use Tony Krowiak
2021-11-04 11:27 ` Harald Freudenberger [this message]
2021-11-04 15:48 ` Tony Krowiak
2021-10-21 15:23 ` [PATCH v17 12/15] s390/vfio-ap: implement in-use callback for vfio_ap driver Tony Krowiak
2021-10-21 15:23 ` [PATCH v17 13/15] s390/vfio-ap: sysfs attribute to display the guest's matrix Tony Krowiak
2021-10-21 15:23 ` [PATCH v17 14/15] s390/ap: notify drivers on config changed and scan complete callbacks Tony Krowiak
2021-11-04 12:06 ` Harald Freudenberger
2021-11-04 15:50 ` Tony Krowiak
2021-11-05 8:23 ` Harald Freudenberger
2021-11-05 13:15 ` Harald Freudenberger
2021-11-08 14:27 ` Tony Krowiak
2021-11-08 14:26 ` Tony Krowiak
2022-02-04 10:43 ` Halil Pasic
2022-02-07 19:39 ` Tony Krowiak
2022-02-08 1:38 ` Halil Pasic
2022-02-08 3:27 ` Tony Krowiak
2021-10-21 15:23 ` [PATCH v17 15/15] s390/vfio-ap: update docs to include dynamic config support Tony Krowiak
2021-10-27 14:24 ` [PATCH v17 00/15] s390/vfio-ap: dynamic configuration support Tony Krowiak
2021-11-02 19:23 ` Tony Krowiak
2021-11-15 15:45 ` Tony Krowiak
2021-11-22 16:12 ` Tony Krowiak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=15a87038-a88c-b29e-f7d7-760ca27c87cf@linux.ibm.com \
--to=freude@linux.ibm.com \
--cc=akrowiak@linux.ibm.com \
--cc=alex.williamson@redhat.com \
--cc=borntraeger@de.ibm.com \
--cc=cohuck@redhat.com \
--cc=fiuczy@linux.ibm.com \
--cc=jjherne@linux.ibm.com \
--cc=kvm@vger.kernel.org \
--cc=kwankhede@nvidia.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-s390@vger.kernel.org \
--cc=mjrosato@linux.ibm.com \
--cc=pasic@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).