This patch series extends the crypto adapter pass-through support to
provide safeguards against inadvertent sharing of AP resources between
guests and/or the host, and to implement more of the s390 AP
architecture related to provisioning and dynamic configuration of
AP resources.
Tony Krowiak (7):
s390: zcrypt: driver callback to indicate resource in use
s390: vfio-ap: implement in-use callback for vfio_ap driver
s390: vfio-ap: allow assignment of unavailable AP resources to mdev
device
s390: vfio-ap: allow hot plug/unplug of AP resources using mdev device
s390: vfio-ap: wait for queue empty on queue reset
s390: vfio-ap: handle dynamic config/deconfig of AP adapter
s390: vfio-ap: update documentation
Documentation/s390/vfio-ap.txt | 147 +++++++++++----
arch/s390/include/asm/kvm_host.h | 2 +
arch/s390/kvm/kvm-s390.c | 37 ++++
drivers/s390/crypto/ap_bus.c | 138 ++++++++++++++-
drivers/s390/crypto/ap_bus.h | 3 +
drivers/s390/crypto/vfio_ap_drv.c | 39 +++-
drivers/s390/crypto/vfio_ap_ops.c | 324 +++++++++++++++-------------------
drivers/s390/crypto/vfio_ap_private.h | 2 +
8 files changed, 471 insertions(+), 221 deletions(-)
--
2.7.4
Introduces a new driver callback to prevent a root user from unbinding
an AP queue from its device driver if the queue is in use. This prevents
a root user from inadvertently taking a queue away from a guest and
giving it to the host, or vice versa. The callback will be invoked
whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
result in one or more AP queues being removed from its driver. If the
callback responds in the affirmative for any driver queried, the change
to the apmask or aqmask will be rejected with a device in use 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 manages AP queues passed through to one or more
guests and we don't want to unexpectedly take AP resources away from
guests which are most likely independently administered.
Signed-off-by: Tony Krowiak <[email protected]>
---
drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
drivers/s390/crypto/ap_bus.h | 3 +
2 files changed, 135 insertions(+), 6 deletions(-)
diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
index 1546389d71db..66a5a9d9fae6 100644
--- a/drivers/s390/crypto/ap_bus.c
+++ b/drivers/s390/crypto/ap_bus.c
@@ -35,6 +35,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"
@@ -980,9 +981,11 @@ int ap_parse_mask_str(const char *str,
newmap = kmalloc(size, GFP_KERNEL);
if (!newmap)
return -ENOMEM;
- if (mutex_lock_interruptible(lock)) {
- kfree(newmap);
- return -ERESTARTSYS;
+ if (lock) {
+ if (mutex_lock_interruptible(lock)) {
+ kfree(newmap);
+ return -ERESTARTSYS;
+ }
}
if (*str == '+' || *str == '-') {
@@ -994,7 +997,10 @@ int ap_parse_mask_str(const char *str,
}
if (rc == 0)
memcpy(bitmap, newmap, size);
- mutex_unlock(lock);
+
+ if (lock)
+ mutex_unlock(lock);
+
kfree(newmap);
return rc;
}
@@ -1181,12 +1187,72 @@ 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;
+
+ /*
+ * If the reserved bits do not identify cards 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;
+
+ /* Pin the driver's module code */
+ if (!try_module_get(drv->owner))
+ return 0;
+
+ if (ap_drv->in_use)
+ if (ap_drv->in_use(newapm, ap_perms.aqm))
+ rc = -EADDRINUSE;
+
+ 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;
+ unsigned long newapm[BITS_TO_LONGS(AP_DEVICES)];
+
+ memcpy(newapm, ap_perms.apm, APMASKSIZE);
+
+ rc = ap_parse_mask_str(buf, newapm, AP_DEVICES, NULL);
+ if (rc)
+ return rc;
+
+ if (mutex_lock_interruptible(&ap_perms_mutex))
+ return -ERESTARTSYS;
+
+ rc = apmask_commit(newapm);
+ mutex_unlock(&ap_perms_mutex);
- rc = ap_parse_mask_str(buf, ap_perms.apm, AP_DEVICES, &ap_perms_mutex);
if (rc)
return rc;
@@ -1212,12 +1278,72 @@ 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;
+
+ /* Pin the driver's module code */
+ if (!try_module_get(drv->owner))
+ return 0;
+
+ if (ap_drv->in_use)
+ if (ap_drv->in_use(ap_perms.apm, newaqm))
+ rc = -EADDRINUSE;
+
+ 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, APMASKSIZE);
+
+ return 0;
+}
+
static ssize_t aqmask_store(struct bus_type *bus, const char *buf,
size_t count)
{
int rc;
+ unsigned long newaqm[BITS_TO_LONGS(AP_DOMAINS)];
+
+ memcpy(newaqm, ap_perms.aqm, AQMASKSIZE);
+
+ rc = ap_parse_mask_str(buf, newaqm, AP_DOMAINS, NULL);
+ if (rc)
+ return rc;
+
+ if (mutex_lock_interruptible(&ap_perms_mutex))
+ return -ERESTARTSYS;
+
+ rc = aqmask_commit(newaqm);
+ mutex_unlock(&ap_perms_mutex);
- rc = ap_parse_mask_str(buf, ap_perms.aqm, AP_DOMAINS, &ap_perms_mutex);
if (rc)
return rc;
diff --git a/drivers/s390/crypto/ap_bus.h b/drivers/s390/crypto/ap_bus.h
index 15a98a673c5c..b00952d5fdbb 100644
--- a/drivers/s390/crypto/ap_bus.h
+++ b/drivers/s390/crypto/ap_bus.h
@@ -137,6 +137,7 @@ struct ap_driver {
void (*remove)(struct ap_device *);
void (*suspend)(struct ap_device *);
void (*resume)(struct ap_device *);
+ bool (*in_use)(unsigned long *apm, unsigned long *aqm);
};
#define to_ap_drv(x) container_of((x), struct ap_driver, driver)
@@ -262,6 +263,8 @@ void ap_queue_reinit_state(struct ap_queue *aq);
struct ap_card *ap_card_create(int id, int queue_depth, int raw_device_type,
int comp_device_type, unsigned int functions);
+#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)];
--
2.7.4
Refactors the AP queue reset function to wait until the queue is empty
after the PQAP(ZAPQ) instruction is executed to zero out the queue as
required by the AP architecture.
Signed-off-by: Tony Krowiak <[email protected]>
---
drivers/s390/crypto/vfio_ap_ops.c | 35 ++++++++++++++++++++++++++++++++---
1 file changed, 32 insertions(+), 3 deletions(-)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index cda1d216ee38..ade2c150fe6b 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -664,15 +664,44 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
return NOTIFY_OK;
}
-static int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
- unsigned int retry)
+static void vfio_ap_mdev_wait_for_qempty(unsigned long apid, unsigned long apqi)
{
struct ap_queue_status status;
+ ap_qid_t qid = AP_MKQID(apid, apqi);
+ int retry = 5;
+
+ do {
+ status = ap_tapq(qid, NULL);
+ switch (status.response_code) {
+ case AP_RESPONSE_NORMAL:
+ if (status.queue_empty)
+ return;
+ msleep(20);
+ break;
+ case AP_RESPONSE_RESET_IN_PROGRESS:
+ case AP_RESPONSE_BUSY:
+ msleep(20);
+ break;
+ default:
+ pr_warn("%s: tapq err %02x: 0x%04x may not be empty\n",
+ __func__, status.response_code, q->apqn);
+ return;
+ }
+ } while (--retry);
+}
+
+int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi)
+{
+ struct ap_queue_status status;
+ int retry = 5;
do {
status = ap_zapq(AP_MKQID(apid, apqi));
switch (status.response_code) {
case AP_RESPONSE_NORMAL:
+ vfio_ap_mdev_wait_for_qempty(apid, apqi);
+ return 0;
+ case AP_RESPONSE_DECONFIGURED:
return 0;
case AP_RESPONSE_RESET_IN_PROGRESS:
case AP_RESPONSE_BUSY:
@@ -698,7 +727,7 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
matrix_mdev->matrix.apm_max + 1) {
for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
matrix_mdev->matrix.aqm_max + 1) {
- ret = vfio_ap_mdev_reset_queue(apid, apqi, 1);
+ ret = vfio_ap_mdev_reset_queue(apid, apqi);
/*
* Regardless whether a queue turns out to be busy, or
* is not operational, we need to continue resetting
--
2.7.4
Updates to the documentation to reflect changes introduced by
this patch series. This patch also clarifies the section on
configuring the apmask and aqmask.
Signed-off-by: Tony Krowiak <[email protected]>
---
Documentation/s390/vfio-ap.txt | 147 +++++++++++++++++++++++++++++++----------
1 file changed, 113 insertions(+), 34 deletions(-)
diff --git a/Documentation/s390/vfio-ap.txt b/Documentation/s390/vfio-ap.txt
index 65167cfe4485..aa9ff0b4f9b1 100644
--- a/Documentation/s390/vfio-ap.txt
+++ b/Documentation/s390/vfio-ap.txt
@@ -438,7 +438,33 @@ guest use.
Example:
=======
Let's now provide an example to illustrate how KVM guests may be given
-access to AP facilities. For this example, we will show how to configure
+access to AP facilities. Let's assume that the following AP devices are
+configured for the host:
+
+/sys/bus/ap/devices
+... 04.0004
+... 04.0005
+... 04.0006
+... 04.0047
+... 04.00ab
+... 04.00ff
+... 05.0004
+... 05.0005
+... 05.0006
+... 05.0047
+... 05.00ab
+... 05.00ff
+... 06.0004
+... 06.0005
+... 06.0006
+... 06.0047
+... 06.00ab
+... 06.00ff
+... card04
+... card05
+... card06
+
+For this example, we will show how to configure
three guests such that executing the lszcrypt command on the guests would
look like this:
@@ -461,7 +487,7 @@ CARD.DOMAIN TYPE MODE
05.0047 CEX5A Accelerator
05.00ff CEX5A Accelerator
-Guest2
+Guest3
------
CARD.DOMAIN TYPE MODE
------------------------------
@@ -538,7 +564,7 @@ These are the steps:
The APQN of each AP queue device assigned to the linux host is checked by the
AP bus against the set of APQNs derived from the cross product of APIDs
and APQIs marked as usable only by the default AP queue device drivers. If a
- match is detected, only the default AP queue device drivers will be probed;
+ match is detected, only the default AP queue device drivers will be probed;
otherwise, the vfio_ap device driver will be probed.
By default, the two masks are set to reserve all APQNs for use by the default
@@ -599,32 +625,72 @@ These are the steps:
default drivers pool: adapter 0-15, domain 1
alternate drivers pool: adapter 16-255, domains 0, 2-255
+ Note:
+
+ * Clearing a bit from the apmask renders all queues associated with
+ the corresponding adapter inaccessible to the host.
+
+ * Clearing a bit from the aqmask renders that queue inaccessible on all
+ adapters reserved for the host.
+
+ * When either mask is changed, the AP bus will reprobe all of the AP devices
+ to ensure each adapter card and queue is bound to the appropriate device
+ driver as specified by the apmask and aqmask. If the change will result in
+ unbinding an AP queue with an APQN that is assigned to an mdev device,
+ the change will be rejected with an EADDRINUSE error.
+
Securing the APQNs for our example:
----------------------------------
To secure the AP queues 05.0004, 05.0047, 05.00ab, 05.00ff, 06.0004, 06.0047,
- 06.00ab, and 06.00ff for use by the vfio_ap device driver, the corresponding
- APQNs can either be removed from the default masks:
+ 06.00ab, and 06.00ff for use by the vfio_ap device driver, either mask can
+ be edited.
+
+ To reserve all queues for adapters 05 and 06:
echo -5,-6 > /sys/bus/ap/apmask
+ or
+ echo 0xf9ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff \
+ > apmask
- echo -4,-0x47,-0xab,-0xff > /sys/bus/ap/aqmask
+ This would result in binding all available queues for adapters 05 and 06 to
+ the vfio_ap device driver:
- Or the masks can be set as follows:
+ /sys/bus/ap
+ ... [drivers]
+ ...... [vfio_ap]
+ ......... [05.0004]
+ ......... [05.0005]
+ ......... [05.0006]
+ ......... [05.0047]
+ ......... [05.00ab]
+ ......... [05.00ff]
+ ......... [06.0004]
+ ......... [06.0005]
+ ......... [06.0006]
+ ......... [06.0047]
+ ......... [06.00ab]
+ ......... [06.00ff]
- echo 0xf9ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff \
- > apmask
+ To reserve only the queues (thus allowing the adapters to be shared by the
+ host and guests):
+ echo -4,-0x47,-0xab,-0xff > /sys/bus/ap/aqmask
+ or
echo 0xf7fffffffffffffffeffffffffffffffffffffffffeffffffffffffffffffffe \
> aqmask
- This will result in AP queues 05.0004, 05.0047, 05.00ab, 05.00ff, 06.0004,
- 06.0047, 06.00ab, and 06.00ff getting bound to the vfio_ap device driver. The
- sysfs directory for the vfio_ap device driver will now contain symbolic links
- to the AP queue devices bound to it:
+ Or the masks can be set as follows:
+
+ This would result in binding only queues 4, 71 (0x47), 171 (0xab), and
+ 255 (0xff) all available adapters to the vfio_ap device driver:
/sys/bus/ap
... [drivers]
...... [vfio_ap]
+ ......... [04.0004]
+ ......... [04.0047]
+ ......... [04.00ab]
+ ......... [04.00ff]
......... [05.0004]
......... [05.0047]
......... [05.00ab]
@@ -709,6 +775,16 @@ These are the steps:
4. The administrator now needs to configure the matrixes for the mediated
devices $uuid1 (for Guest1), $uuid2 (for Guest2) and $uuid3 (for Guest3).
+ The matrix is configured by assigning adapters, domains, and control domains
+ to the mediated matrix device using its sysfs assignment interfaces.
+
+ For example, to assign adapters 4, 5, 6, 22 and 23:
+
+ echo 4 > assign_adapter
+ echo 5 > assign_adapter
+ echo 6 > assign_adapter
+ echo 22 > assign_adapter
+ echo 23 > assign_adapter
This is how the matrix is configured for Guest1:
@@ -748,11 +824,9 @@ These are the steps:
an error (ENODEV).
* All APQNs that can be derived from the adapter ID and the IDs of
- the previously assigned domains must be bound to the vfio_ap device
- driver. If no domains have yet been assigned, then there must be at least
- one APQN with the specified APID bound to the vfio_ap driver. If no such
- APQNs are bound to the driver, the operation will terminate with an
- error (EADDRNOTAVAIL).
+ the previously assigned domains must be available for use by the vfio_ap
+ device driver as specified by the bus's apmask and aqmask sysfs interfaces;
+ otherwise, the operation will terminate with an error (EADDRNOTAVAIL).
No APQN that can be derived from the adapter ID and the IDs of the
previously assigned domains can be assigned to another mediated matrix
@@ -767,11 +841,9 @@ These are the steps:
an error (ENODEV).
* All APQNs that can be derived from the domain ID and the IDs of
- the previously assigned adapters must be bound to the vfio_ap device
- driver. If no domains have yet been assigned, then there must be at least
- one APQN with the specified APQI bound to the vfio_ap driver. If no such
- APQNs are bound to the driver, the operation will terminate with an
- error (EADDRNOTAVAIL).
+ the previously assigned adapters must be available for use by the vfio_ap
+ device driver as specified by the bus's apmask and aqmask sysfs interfaces;
+ otherwise, the operation will terminate with an error (EADDRNOTAVAIL).
No APQN that can be derived from the domain ID and the IDs of the
previously assigned adapters can be assigned to another mediated matrix
@@ -824,14 +896,21 @@ Using our example again, to remove the mediated matrix device $uuid1:
Limitations
===========
-* The KVM/kernel interfaces do not provide a way to prevent restoring an APQN
- to the default drivers pool of a queue that is still assigned to a mediated
- device in use by a guest. It is incumbent upon the administrator to
- ensure there is no mediated device in use by a guest to which the APQN is
- assigned lest the host be given access to the private data of the AP queue
- device such as a private key configured specifically for the guest.
-
-* Dynamically modifying the AP matrix for a running guest (which would amount to
- hot(un)plug of AP devices for the guest) is currently not supported
-
-* Live guest migration is not supported for guests using AP devices.
+* Live guest migration is not supported for guests using AP devices. The
+ AP devices being used by the guest must be unplugged before live migration
+ can be initiated. Hot plug/unplug of adapters, domains and control domains can
+ be done using the mediated matrix device sysfs assignment interfaces. To
+ unplug an adapter from a running guest, simply unassign it. For example, if
+ a guest is using adapters 0 through 15, they can be unplugged as follows:
+
+ echo 0 > unassign_adapter
+ echo 1 > unassign_adapter
+ echo 2 > unassign_adapter
+ ...
+ echo 15 > unassign_adapter
+
+ Note that if you unassign an adapter, all of the associated domains will also
+ become inaccessible on the guest, so it is only necessary to unassign
+ the adapters before live migration. After the live migration is complete,
+ the AP resources will have to be re-assigned to the mediated matrix device
+ on the target system.
--
2.7.4
Once an APQN is assigned to an mdev device it will remained assigned until
it is explicitly unassigned from the mdev device. The associated AP queue
devices, however, can come and go due to failures or deliberate actions by
a sysadmin. For example, a sysadmin can dynamically remove an AP adapter
card using the SE or by executing an SCLP command. This patch refactors
the probe and remove callbacks of the vfio_ap driver to handle dynamic
changes as follows:
* Probe callback changes:
If the APQN of the queue being probed is assigned to an mdev device, the
mdev device is in use by a guest, and the APQN is not set in the guest's
CRYCB, the CRYCB will be dynamically updated to give the guest access to
the queue.
* Remove callback changes:
If the APQN of the queue being removed is assigned to an mdev device,
the mdev
device is in use by a guest, and the APQN is set in the guest's CRYCB,
the CRYCB will be dynamically updated to remove the guest's access to
the adapter card associated with the queue. Keep in mind, the
architecture does not provide a way to remove access to a single queue
unless only one queue is in the guest's configuration, so it was decided
that it makes more sense to unplug the adapter from the guest.
The APQN of the queue being removed will remain assigned to the mdev
device should the queue be dynamically returned to the configuration.
The queue will also be reset prior to returning control to the caller
(a.k.a., the AP bus).
Signed-off-by: Tony Krowiak <[email protected]>
---
arch/s390/include/asm/kvm_host.h | 2 ++
arch/s390/kvm/kvm-s390.c | 37 +++++++++++++++++++
drivers/s390/crypto/vfio_ap_drv.c | 16 +++++++--
drivers/s390/crypto/vfio_ap_ops.c | 67 +++++++++++++++++++++++++++++++++--
drivers/s390/crypto/vfio_ap_private.h | 2 ++
5 files changed, 120 insertions(+), 4 deletions(-)
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index c47e22bba87f..0ce5d9b0df59 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -895,6 +895,8 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
void kvm_arch_crypto_clear_masks(struct kvm *kvm);
void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
unsigned long *aqm, unsigned long *adm);
+int kvm_arch_crypto_test_masks(struct kvm *kvm, unsigned long *apm,
+ unsigned long *aqm, unsigned long *adm);
extern int sie64a(struct kvm_s390_sie_block *, u64 *);
extern char sie_exit;
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 4638303ba6a8..5f423cdd29ba 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2217,6 +2217,43 @@ static void kvm_s390_set_crycb_format(struct kvm *kvm)
kvm->arch.crypto.crycbd |= CRYCB_FORMAT1;
}
+int kvm_arch_crypto_test_masks(struct kvm *kvm, unsigned long *apm,
+ unsigned long *aqm, unsigned long *adm)
+{
+ int ret;
+ struct kvm_s390_crypto_cb *crycb = kvm->arch.crypto.crycb;
+
+ switch (kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
+ case CRYCB_FORMAT2: /* APCB1 use 256 bits */
+ ret = bitmap_equal(apm, (unsigned long *)crycb->apcb1.apm, 256);
+ VM_EVENT(kvm, 3, "TEST CRYCB: apm %016lx %016lx %016lx %016lx",
+ apm[0], apm[1], apm[2], apm[3]);
+ ret &= bitmap_equal(aqm,
+ (unsigned long *)crycb->apcb1.aqm, 256);
+ VM_EVENT(kvm, 3, "TEST CRYCB: aqm %016lx %016lx %016lx %016lx",
+ aqm[0], aqm[1], aqm[2], aqm[3]);
+ ret &= bitmap_equal(adm,
+ (unsigned long *)crycb->apcb1.adm, 256);
+ VM_EVENT(kvm, 3, "TEST CRYCB: adm %016lx %016lx %016lx %016lx",
+ adm[0], adm[1], adm[2], adm[3]);
+ break;
+ case CRYCB_FORMAT1:
+ case CRYCB_FORMAT0: /* Fall through both use APCB0 */
+ ret = bitmap_equal(apm, (unsigned long *)crycb->apcb1.apm, 64);
+ ret &= bitmap_equal(aqm, (unsigned long *)crycb->apcb1.aqm, 16);
+ ret &= bitmap_equal(adm, (unsigned long *)crycb->apcb1.adm, 16);
+ VM_EVENT(kvm, 3, "TEST CRYCB: apm %016lx aqm %04x adm %04x",
+ apm[0], *((unsigned short *)aqm),
+ *((unsigned short *)adm));
+ break;
+ default: /* Can not happen */
+ ret = 0;
+ break;
+ }
+ return ret;
+}
+EXPORT_SYMBOL_GPL(kvm_arch_crypto_test_masks);
+
void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
unsigned long *aqm, unsigned long *adm)
{
diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index f340a28c1d65..2a79d27d9730 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -42,12 +42,24 @@ MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
{
- return 0;
+ struct ap_queue *apq = to_ap_queue(&apdev->device);
+ unsigned long apid = AP_QID_CARD(apq->qid);
+ unsigned long apqi = AP_QID_QUEUE(apq->qid);
+
+ mutex_lock(&matrix_dev->lock);
+ vfio_ap_mdev_probe_queue(apid, apqi);
+ mutex_unlock(&matrix_dev->lock);
}
static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
{
- /* Nothing to do yet */
+ struct ap_queue *apq = to_ap_queue(&apdev->device);
+ unsigned long apid = AP_QID_CARD(apq->qid);
+ unsigned long apqi = AP_QID_QUEUE(apq->qid);
+
+ mutex_lock(&matrix_dev->lock);
+ vfio_ap_mdev_remove_queue(apid, apqi);
+ mutex_unlock(&matrix_dev->lock);
}
static void vfio_ap_matrix_dev_release(struct device *dev)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index ade2c150fe6b..8a70707bf870 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -683,8 +683,8 @@ static void vfio_ap_mdev_wait_for_qempty(unsigned long apid, unsigned long apqi)
msleep(20);
break;
default:
- pr_warn("%s: tapq err %02x: 0x%04x may not be empty\n",
- __func__, status.response_code, q->apqn);
+ pr_warn("%s: tapq err %02x: %02lx%04lx may not be empty\n",
+ __func__, status.response_code, apid, apqi);
return;
}
} while (--retry);
@@ -840,3 +840,66 @@ void vfio_ap_mdev_unregister(void)
{
mdev_unregister_device(&matrix_dev->device);
}
+
+static struct ap_matrix_mdev *vfio_ap_mdev_find_matrix_mdev(unsigned long apid,
+ unsigned long apqi)
+{
+ struct ap_matrix_mdev *matrix_mdev;
+
+ list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
+ if (test_bit_inv(apid, matrix_mdev->matrix.apm) &&
+ test_bit_inv(apqi, matrix_mdev->matrix.aqm))
+ return matrix_mdev;
+ }
+
+ return NULL;
+}
+
+void vfio_ap_mdev_remove_queue(unsigned long apid, unsigned long apqi)
+{
+ struct ap_matrix_mdev *matrix_mdev;
+
+ matrix_mdev = vfio_ap_mdev_find_matrix_mdev(apid, apqi);
+
+ /*
+ * If the queue is assigned to the mdev device and the mdev device
+ * is in use by a guest
+ */
+ if (matrix_mdev && matrix_mdev->kvm) {
+ /*
+ * If the queue is plugged into the guest, unplug the adapter
+ * but keep the adapter assigned to the mdev device
+ */
+ if (!kvm_arch_crypto_test_masks(matrix_mdev->kvm,
+ matrix_mdev->matrix.apm,
+ matrix_mdev->matrix.aqm,
+ matrix_mdev->matrix.adm)) {
+ clear_bit_inv(apid, matrix_mdev->matrix.apm);
+ vfio_ap_mdev_update_crycb(matrix_mdev);
+ set_bit_inv(apid, matrix_mdev->matrix.apm);
+ }
+ }
+
+ vfio_ap_mdev_reset_queue(apid, apqi);
+}
+
+void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi)
+{
+ struct ap_matrix_mdev *matrix_mdev;
+
+ matrix_mdev = vfio_ap_mdev_find_matrix_mdev(apid, apqi);
+
+ /*
+ * If the queue is assigned to the mdev device and the mdev device
+ * is in use by a guest
+ */
+ if (matrix_mdev && matrix_mdev->kvm) {
+ /* If the queue is not plugged into the guest, plug it in */
+ if (!kvm_arch_crypto_test_masks(matrix_mdev->kvm,
+ matrix_mdev->matrix.apm,
+ matrix_mdev->matrix.aqm,
+ matrix_mdev->matrix.adm)) {
+ vfio_ap_mdev_update_crycb(matrix_mdev);
+ }
+ }
+}
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 76b7f98e47e9..acdd5bfabaaf 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -85,5 +85,7 @@ struct ap_matrix_mdev {
extern int vfio_ap_mdev_register(void);
extern void vfio_ap_mdev_unregister(void);
+void vfio_ap_mdev_remove_queue(unsigned long apid, unsigned long apqi);
+void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi);
#endif /* _VFIO_AP_PRIVATE_H_ */
--
2.7.4
The AP architecture does not preclude assignment of AP resources that are
not yet in the AP configuration (i.e., not available or not online).
Let's go ahead and implement this facet of the AP architecture for linux
guests.
Access to AP resources is controlled by bit masks in a guest's SIE
state description (i.e., the control block containing the state of a
guest). When an AP instruction is executed on the guest, the firmware
combines the masks from the guest's SIE state description with the masks
from the host by doing a logical bitwise AND of the masks to create what
are defined as effective masks. The effective masks determine which AP
resources can be accessed by the guest.
Effective masking allows the assignment of AP resources to a guest even
if the underlying device is not in the AP configuration. If the device
subsequently becomes configured, the guest will automatically have access
to the associated AP resources (e.g., AP queues).
The current implementation does not allow assignment of AP resources to
an mdev device if the AP queues identified by the assignment are not
bound to the vfio_ap device driver. This patch allows assignment of AP
resources to the mdev device even if the AP queues are not bound to the
vfio_ap device driver, as long as the AP queues are not reserved by the
AP BUS for use by the zcrypt device drivers. If the queues are subsequently
bound to the vfio_ap device driver while a running guest is using the mdev
device, the guest will automatically gain access to the queues. Effective
masking prevents access to queues until such time as they become available.
Signed-off-by: Tony Krowiak <[email protected]>
---
drivers/s390/crypto/vfio_ap_ops.c | 160 +++++---------------------------------
1 file changed, 18 insertions(+), 142 deletions(-)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 900b9cf20ca5..cb3e4f7671be 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -113,122 +113,6 @@ static struct attribute_group *vfio_ap_mdev_type_groups[] = {
NULL,
};
-struct vfio_ap_queue_reserved {
- unsigned long *apid;
- unsigned long *apqi;
- bool reserved;
-};
-
-/**
- * vfio_ap_has_queue
- *
- * @dev: an AP queue device
- * @data: a struct vfio_ap_queue_reserved reference
- *
- * Flags whether the AP queue device (@dev) has a queue ID containing the APQN,
- * apid or apqi specified in @data:
- *
- * - If @data contains both an apid and apqi value, then @data will be flagged
- * as reserved if the APID and APQI fields for the AP queue device matches
- *
- * - If @data contains only an apid value, @data will be flagged as
- * reserved if the APID field in the AP queue device matches
- *
- * - If @data contains only an apqi value, @data will be flagged as
- * reserved if the APQI field in the AP queue device matches
- *
- * Returns 0 to indicate the input to function succeeded. Returns -EINVAL if
- * @data does not contain either an apid or apqi.
- */
-static int vfio_ap_has_queue(struct device *dev, void *data)
-{
- struct vfio_ap_queue_reserved *qres = data;
- struct ap_queue *ap_queue = to_ap_queue(dev);
- ap_qid_t qid;
- unsigned long id;
-
- if (qres->apid && qres->apqi) {
- qid = AP_MKQID(*qres->apid, *qres->apqi);
- if (qid == ap_queue->qid)
- qres->reserved = true;
- } else if (qres->apid && !qres->apqi) {
- id = AP_QID_CARD(ap_queue->qid);
- if (id == *qres->apid)
- qres->reserved = true;
- } else if (!qres->apid && qres->apqi) {
- id = AP_QID_QUEUE(ap_queue->qid);
- if (id == *qres->apqi)
- qres->reserved = true;
- } else {
- return -EINVAL;
- }
-
- return 0;
-}
-
-/**
- * vfio_ap_verify_queue_reserved
- *
- * @matrix_dev: a mediated matrix device
- * @apid: an AP adapter ID
- * @apqi: an AP queue index
- *
- * Verifies that the AP queue with @apid/@apqi is reserved by the VFIO AP device
- * driver according to the following rules:
- *
- * - If both @apid and @apqi are not NULL, then there must be an AP queue
- * device bound to the vfio_ap driver with the APQN identified by @apid and
- * @apqi
- *
- * - If only @apid is not NULL, then there must be an AP queue device bound
- * to the vfio_ap driver with an APQN containing @apid
- *
- * - If only @apqi is not NULL, then there must be an AP queue device bound
- * to the vfio_ap driver with an APQN containing @apqi
- *
- * Returns 0 if the AP queue is reserved; otherwise, returns -EADDRNOTAVAIL.
- */
-static int vfio_ap_verify_queue_reserved(unsigned long *apid,
- unsigned long *apqi)
-{
- int ret;
- struct vfio_ap_queue_reserved qres;
-
- qres.apid = apid;
- qres.apqi = apqi;
- qres.reserved = false;
-
- ret = driver_for_each_device(&matrix_dev->vfio_ap_drv->driver, NULL,
- &qres, vfio_ap_has_queue);
- if (ret)
- return ret;
-
- if (qres.reserved)
- return 0;
-
- return -EADDRNOTAVAIL;
-}
-
-static int
-vfio_ap_mdev_verify_queues_reserved_for_apid(struct ap_matrix_mdev *matrix_mdev,
- unsigned long apid)
-{
- int ret;
- unsigned long apqi;
- unsigned long nbits = matrix_mdev->matrix.aqm_max + 1;
-
- if (find_first_bit_inv(matrix_mdev->matrix.aqm, nbits) >= nbits)
- return vfio_ap_verify_queue_reserved(&apid, NULL);
-
- for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, nbits) {
- ret = vfio_ap_verify_queue_reserved(&apid, &apqi);
- if (ret)
- return ret;
- }
-
- return 0;
-}
-
/**
* vfio_ap_mdev_verify_no_sharing
*
@@ -336,14 +220,22 @@ static ssize_t assign_adapter_store(struct device *dev,
set_bit_inv(apid, matrix_mdev->matrix.apm);
+ ret = ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
+ matrix_mdev->matrix.aqm);
+
+ /* If any APQN is reserved for used by the default drivers */
+ ret = (ret == 1) ? -EADDRNOTAVAIL : ret;
+ if (ret)
+ goto error;
+
ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
if (ret)
- goto share_err;
+ goto error;
ret = count;
goto done;
-share_err:
+error:
clear_bit_inv(apid, matrix_mdev->matrix.apm);
done:
mutex_unlock(&matrix_dev->lock);
@@ -397,26 +289,6 @@ static ssize_t unassign_adapter_store(struct device *dev,
}
static DEVICE_ATTR_WO(unassign_adapter);
-static int
-vfio_ap_mdev_verify_queues_reserved_for_apqi(struct ap_matrix_mdev *matrix_mdev,
- unsigned long apqi)
-{
- int ret;
- unsigned long apid;
- unsigned long nbits = matrix_mdev->matrix.apm_max + 1;
-
- if (find_first_bit_inv(matrix_mdev->matrix.apm, nbits) >= nbits)
- return vfio_ap_verify_queue_reserved(NULL, &apqi);
-
- for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, nbits) {
- ret = vfio_ap_verify_queue_reserved(&apid, &apqi);
- if (ret)
- return ret;
- }
-
- return 0;
-}
-
/**
* assign_domain_store
*
@@ -471,12 +343,16 @@ static ssize_t assign_domain_store(struct device *dev,
mutex_lock(&matrix_dev->lock);
- ret = vfio_ap_mdev_verify_queues_reserved_for_apqi(matrix_mdev, apqi);
- if (ret)
- goto done;
-
set_bit_inv(apqi, matrix_mdev->matrix.aqm);
+ ret = ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
+ matrix_mdev->matrix.aqm);
+
+ /* If any APQN is owned by the default drivers */
+ ret = (ret == 1) ? -EADDRNOTAVAIL : ret;
+ if (ret)
+ goto error;
+
ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
if (ret)
goto share_err;
--
2.7.4
Let's implement the callback to indicate when an APQN
is in use by the vfio_ap device driver. The callback is
invoked whenever a change to the apmask or aqmask may
result in one or APQNs being removed from the driver. The
vfio_ap device driver will indicate a resource is in use
if any of the removed APQNs are assigned to any of the matrix
mdev devices.
Signed-off-by: Tony Krowiak <[email protected]>
---
drivers/s390/crypto/vfio_ap_drv.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index e9824c35c34f..f340a28c1d65 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -139,6 +139,28 @@ static void vfio_ap_matrix_dev_destroy(void)
root_device_unregister(root_device);
}
+static bool vfio_ap_resource_in_use(unsigned long *apm, unsigned long *aqm)
+{
+ bool in_use = false;
+ struct ap_matrix_mdev *matrix_mdev;
+
+ mutex_lock(&matrix_dev->lock);
+
+ list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
+ if (bitmap_intersects(matrix_mdev->matrix.apm,
+ apm, AP_DEVICES) &&
+ bitmap_intersects(matrix_mdev->matrix.aqm,
+ aqm, AP_DOMAINS)) {
+ in_use = true;
+ break;
+ }
+ }
+
+ mutex_unlock(&matrix_dev->lock);
+
+ return in_use;
+}
+
static int __init vfio_ap_init(void)
{
int ret;
@@ -154,6 +176,7 @@ static int __init vfio_ap_init(void)
memset(&vfio_ap_drv, 0, sizeof(vfio_ap_drv));
vfio_ap_drv.probe = vfio_ap_queue_dev_probe;
vfio_ap_drv.remove = vfio_ap_queue_dev_remove;
+ vfio_ap_drv.in_use = vfio_ap_resource_in_use;
vfio_ap_drv.ids = ap_queue_ids;
ret = ap_driver_register(&vfio_ap_drv, THIS_MODULE, VFIO_AP_DRV_NAME);
--
2.7.4
Let's allow adapters, domains and control domains to be assigned to or
unassigned from an AP matrix mdev device while it is in use by a guest.
When an adapter, domain or control domain is assigned to or unassigned
from an mdev device while a guest is using it, the guest's CRYCB will be
updated thus giving access to the resource assigned, or taking access away
from the resource unassigned for the guest.
Signed-off-by: Tony Krowiak <[email protected]>
---
drivers/s390/crypto/vfio_ap_ops.c | 68 +++++++++++++++++++--------------------
1 file changed, 33 insertions(+), 35 deletions(-)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index cb3e4f7671be..cda1d216ee38 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -155,6 +155,24 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
return 0;
}
+/*
+ * vfio_ap_mdev_update_crycb
+ *
+ * @matrix_mdev: the mediated matrix device
+ *
+ * Updates the AP matrix in the guest's CRYCB from the masks configured for the
+ * mediated matrix device via its sysfs interfaces.
+ */
+static void vfio_ap_mdev_update_crycb(struct ap_matrix_mdev *matrix_mdev)
+{
+ if (matrix_mdev->kvm) {
+ kvm_arch_crypto_set_masks(matrix_mdev->kvm,
+ matrix_mdev->matrix.apm,
+ matrix_mdev->matrix.aqm,
+ matrix_mdev->matrix.adm);
+ }
+}
+
/**
* assign_adapter_store
*
@@ -196,10 +214,6 @@ static ssize_t assign_adapter_store(struct device *dev,
struct mdev_device *mdev = mdev_from_dev(dev);
struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
- /* If the guest is running, disallow assignment of adapter */
- if (matrix_mdev->kvm)
- return -EBUSY;
-
ret = kstrtoul(buf, 0, &apid);
if (ret)
return ret;
@@ -214,16 +228,12 @@ static ssize_t assign_adapter_store(struct device *dev,
*/
mutex_lock(&matrix_dev->lock);
- ret = vfio_ap_mdev_verify_queues_reserved_for_apid(matrix_mdev, apid);
- if (ret)
- goto done;
-
set_bit_inv(apid, matrix_mdev->matrix.apm);
ret = ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
matrix_mdev->matrix.aqm);
- /* If any APQN is reserved for used by the default drivers */
+ /* If any APQN is owned by the default drivers */
ret = (ret == 1) ? -EADDRNOTAVAIL : ret;
if (ret)
goto error;
@@ -232,6 +242,7 @@ static ssize_t assign_adapter_store(struct device *dev,
if (ret)
goto error;
+ vfio_ap_mdev_update_crycb(matrix_mdev);
ret = count;
goto done;
@@ -270,10 +281,6 @@ static ssize_t unassign_adapter_store(struct device *dev,
struct mdev_device *mdev = mdev_from_dev(dev);
struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
- /* If the guest is running, disallow un-assignment of adapter */
- if (matrix_mdev->kvm)
- return -EBUSY;
-
ret = kstrtoul(buf, 0, &apid);
if (ret)
return ret;
@@ -283,6 +290,7 @@ static ssize_t unassign_adapter_store(struct device *dev,
mutex_lock(&matrix_dev->lock);
clear_bit_inv((unsigned long)apid, matrix_mdev->matrix.apm);
+ vfio_ap_mdev_update_crycb(matrix_mdev);
mutex_unlock(&matrix_dev->lock);
return count;
@@ -331,10 +339,6 @@ static ssize_t assign_domain_store(struct device *dev,
struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
unsigned long max_apqi = matrix_mdev->matrix.aqm_max;
- /* If the guest is running, disallow assignment of domain */
- if (matrix_mdev->kvm)
- return -EBUSY;
-
ret = kstrtoul(buf, 0, &apqi);
if (ret)
return ret;
@@ -355,12 +359,13 @@ static ssize_t assign_domain_store(struct device *dev,
ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
if (ret)
- goto share_err;
+ goto error;
+ vfio_ap_mdev_update_crycb(matrix_mdev);
ret = count;
goto done;
-share_err:
+error:
clear_bit_inv(apqi, matrix_mdev->matrix.aqm);
done:
mutex_unlock(&matrix_dev->lock);
@@ -396,10 +401,6 @@ static ssize_t unassign_domain_store(struct device *dev,
struct mdev_device *mdev = mdev_from_dev(dev);
struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
- /* If the guest is running, disallow un-assignment of domain */
- if (matrix_mdev->kvm)
- return -EBUSY;
-
ret = kstrtoul(buf, 0, &apqi);
if (ret)
return ret;
@@ -409,6 +410,7 @@ static ssize_t unassign_domain_store(struct device *dev,
mutex_lock(&matrix_dev->lock);
clear_bit_inv((unsigned long)apqi, matrix_mdev->matrix.aqm);
+ vfio_ap_mdev_update_crycb(matrix_mdev);
mutex_unlock(&matrix_dev->lock);
return count;
@@ -440,10 +442,6 @@ static ssize_t assign_control_domain_store(struct device *dev,
struct mdev_device *mdev = mdev_from_dev(dev);
struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
- /* If the guest is running, disallow assignment of control domain */
- if (matrix_mdev->kvm)
- return -EBUSY;
-
ret = kstrtoul(buf, 0, &id);
if (ret)
return ret;
@@ -451,13 +449,16 @@ static ssize_t assign_control_domain_store(struct device *dev,
if (id > matrix_mdev->matrix.adm_max)
return -ENODEV;
- /* Set the bit in the ADM (bitmask) corresponding to the AP control
- * domain number (id). The bits in the mask, from most significant to
- * least significant, correspond to IDs 0 up to the one less than the
- * number of control domains that can be assigned.
+ /*
+ * Set the bits in the ADM (bitmask) corresponding to the AP control
+ * domain numbers in dommask. The bits in the mask, from left to right,
+ * correspond to IDs 0 up to the one less than the number of control
+ * domains that can be assigned.
+ *
*/
mutex_lock(&matrix_dev->lock);
set_bit_inv(id, matrix_mdev->matrix.adm);
+ vfio_ap_mdev_update_crycb(matrix_mdev);
mutex_unlock(&matrix_dev->lock);
return count;
@@ -490,10 +491,6 @@ static ssize_t unassign_control_domain_store(struct device *dev,
struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
unsigned long max_domid = matrix_mdev->matrix.adm_max;
- /* If the guest is running, disallow un-assignment of control domain */
- if (matrix_mdev->kvm)
- return -EBUSY;
-
ret = kstrtoul(buf, 0, &domid);
if (ret)
return ret;
@@ -502,6 +499,7 @@ static ssize_t unassign_control_domain_store(struct device *dev,
mutex_lock(&matrix_dev->lock);
clear_bit_inv(domid, matrix_mdev->matrix.adm);
+ vfio_ap_mdev_update_crycb(matrix_mdev);
mutex_unlock(&matrix_dev->lock);
return count;
--
2.7.4
On 11.04.19 23:03, Tony Krowiak wrote:
> Introduces a new driver callback to prevent a root user from unbinding
> an AP queue from its device driver if the queue is in use. This prevents
> a root user from inadvertently taking a queue away from a guest and
> giving it to the host, or vice versa. The callback will be invoked
> whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
> result in one or more AP queues being removed from its driver. If the
> callback responds in the affirmative for any driver queried, the change
> to the apmask or aqmask will be rejected with a device in use 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 manages AP queues passed through to one or more
> guests and we don't want to unexpectedly take AP resources away from
> guests which are most likely independently administered.
>
> Signed-off-by: Tony Krowiak <[email protected]>
Hello Tony
you are going out with your patches but ... what is the result of the discussions
we had in the past ? Do we have a common understanding that we want to have
it this way ? A driver which is able to claim resources and the bus code
has lower precedence ?
> ---
> drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
> drivers/s390/crypto/ap_bus.h | 3 +
> 2 files changed, 135 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
> index 1546389d71db..66a5a9d9fae6 100644
> --- a/drivers/s390/crypto/ap_bus.c
> +++ b/drivers/s390/crypto/ap_bus.c
> @@ -35,6 +35,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"
> @@ -980,9 +981,11 @@ int ap_parse_mask_str(const char *str,
> newmap = kmalloc(size, GFP_KERNEL);
> if (!newmap)
> return -ENOMEM;
> - if (mutex_lock_interruptible(lock)) {
> - kfree(newmap);
> - return -ERESTARTSYS;
> + if (lock) {
> + if (mutex_lock_interruptible(lock)) {
> + kfree(newmap);
> + return -ERESTARTSYS;
> + }
> }
>
> if (*str == '+' || *str == '-') {
> @@ -994,7 +997,10 @@ int ap_parse_mask_str(const char *str,
> }
> if (rc == 0)
> memcpy(bitmap, newmap, size);
> - mutex_unlock(lock);
> +
> + if (lock)
> + mutex_unlock(lock);
> +
> kfree(newmap);
> return rc;
> }
> @@ -1181,12 +1187,72 @@ 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;
> +
> + /*
> + * If the reserved bits do not identify cards 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;
> +
> + /* Pin the driver's module code */
> + if (!try_module_get(drv->owner))
> + return 0;
> +
> + if (ap_drv->in_use)
> + if (ap_drv->in_use(newapm, ap_perms.aqm))
> + rc = -EADDRINUSE;
> +
> + 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;
> + unsigned long newapm[BITS_TO_LONGS(AP_DEVICES)];
> +
> + memcpy(newapm, ap_perms.apm, APMASKSIZE);
> +
> + rc = ap_parse_mask_str(buf, newapm, AP_DEVICES, NULL);
> + if (rc)
> + return rc;
> +
> + if (mutex_lock_interruptible(&ap_perms_mutex))
> + return -ERESTARTSYS;
> +
> + rc = apmask_commit(newapm);
> + mutex_unlock(&ap_perms_mutex);
>
> - rc = ap_parse_mask_str(buf, ap_perms.apm, AP_DEVICES, &ap_perms_mutex);
> if (rc)
> return rc;
>
> @@ -1212,12 +1278,72 @@ 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;
> +
> + /* Pin the driver's module code */
> + if (!try_module_get(drv->owner))
> + return 0;
> +
> + if (ap_drv->in_use)
> + if (ap_drv->in_use(ap_perms.apm, newaqm))
> + rc = -EADDRINUSE;
> +
> + 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, APMASKSIZE);
> +
> + return 0;
> +}
> +
> static ssize_t aqmask_store(struct bus_type *bus, const char *buf,
> size_t count)
> {
> int rc;
> + unsigned long newaqm[BITS_TO_LONGS(AP_DOMAINS)];
> +
> + memcpy(newaqm, ap_perms.aqm, AQMASKSIZE);
> +
> + rc = ap_parse_mask_str(buf, newaqm, AP_DOMAINS, NULL);
> + if (rc)
> + return rc;
> +
> + if (mutex_lock_interruptible(&ap_perms_mutex))
> + return -ERESTARTSYS;
> +
> + rc = aqmask_commit(newaqm);
> + mutex_unlock(&ap_perms_mutex);
>
> - rc = ap_parse_mask_str(buf, ap_perms.aqm, AP_DOMAINS, &ap_perms_mutex);
> if (rc)
> return rc;
>
> diff --git a/drivers/s390/crypto/ap_bus.h b/drivers/s390/crypto/ap_bus.h
> index 15a98a673c5c..b00952d5fdbb 100644
> --- a/drivers/s390/crypto/ap_bus.h
> +++ b/drivers/s390/crypto/ap_bus.h
> @@ -137,6 +137,7 @@ struct ap_driver {
> void (*remove)(struct ap_device *);
> void (*suspend)(struct ap_device *);
> void (*resume)(struct ap_device *);
> + bool (*in_use)(unsigned long *apm, unsigned long *aqm);
> };
>
> #define to_ap_drv(x) container_of((x), struct ap_driver, driver)
> @@ -262,6 +263,8 @@ void ap_queue_reinit_state(struct ap_queue *aq);
> struct ap_card *ap_card_create(int id, int queue_depth, int raw_device_type,
> int comp_device_type, unsigned int functions);
>
> +#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)];
On 11.04.19 23:03, Tony Krowiak wrote:
> Once an APQN is assigned to an mdev device it will remained assigned until
> it is explicitly unassigned from the mdev device. The associated AP queue
> devices, however, can come and go due to failures or deliberate actions by
> a sysadmin. For example, a sysadmin can dynamically remove an AP adapter
> card using the SE or by executing an SCLP command. This patch refactors
> the probe and remove callbacks of the vfio_ap driver to handle dynamic
> changes as follows:
>
> * Probe callback changes:
>
> If the APQN of the queue being probed is assigned to an mdev device, the
> mdev device is in use by a guest, and the APQN is not set in the guest's
> CRYCB, the CRYCB will be dynamically updated to give the guest access to
> the queue.
>
> * Remove callback changes:
>
> If the APQN of the queue being removed is assigned to an mdev device,
> the mdev
> device is in use by a guest, and the APQN is set in the guest's CRYCB,
> the CRYCB will be dynamically updated to remove the guest's access to
> the adapter card associated with the queue. Keep in mind, the
> architecture does not provide a way to remove access to a single queue
> unless only one queue is in the guest's configuration, so it was decided
> that it makes more sense to unplug the adapter from the guest.
>
> The APQN of the queue being removed will remain assigned to the mdev
> device should the queue be dynamically returned to the configuration.
> The queue will also be reset prior to returning control to the caller
> (a.k.a., the AP bus).
Didn't we agree that the reset is done by the AP bus anyway. This was
why I did the rework around the remove callback in the AP bus code.
> Signed-off-by: Tony Krowiak <[email protected]>
> ---
> arch/s390/include/asm/kvm_host.h | 2 ++
> arch/s390/kvm/kvm-s390.c | 37 +++++++++++++++++++
> drivers/s390/crypto/vfio_ap_drv.c | 16 +++++++--
> drivers/s390/crypto/vfio_ap_ops.c | 67 +++++++++++++++++++++++++++++++++--
> drivers/s390/crypto/vfio_ap_private.h | 2 ++
> 5 files changed, 120 insertions(+), 4 deletions(-)
>
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index c47e22bba87f..0ce5d9b0df59 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -895,6 +895,8 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
> void kvm_arch_crypto_clear_masks(struct kvm *kvm);
> void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
> unsigned long *aqm, unsigned long *adm);
> +int kvm_arch_crypto_test_masks(struct kvm *kvm, unsigned long *apm,
> + unsigned long *aqm, unsigned long *adm);
>
> extern int sie64a(struct kvm_s390_sie_block *, u64 *);
> extern char sie_exit;
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 4638303ba6a8..5f423cdd29ba 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2217,6 +2217,43 @@ static void kvm_s390_set_crycb_format(struct kvm *kvm)
> kvm->arch.crypto.crycbd |= CRYCB_FORMAT1;
> }
>
> +int kvm_arch_crypto_test_masks(struct kvm *kvm, unsigned long *apm,
> + unsigned long *aqm, unsigned long *adm)
> +{
> + int ret;
> + struct kvm_s390_crypto_cb *crycb = kvm->arch.crypto.crycb;
> +
> + switch (kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
> + case CRYCB_FORMAT2: /* APCB1 use 256 bits */
> + ret = bitmap_equal(apm, (unsigned long *)crycb->apcb1.apm, 256);
> + VM_EVENT(kvm, 3, "TEST CRYCB: apm %016lx %016lx %016lx %016lx",
> + apm[0], apm[1], apm[2], apm[3]);
> + ret &= bitmap_equal(aqm,
> + (unsigned long *)crycb->apcb1.aqm, 256);
> + VM_EVENT(kvm, 3, "TEST CRYCB: aqm %016lx %016lx %016lx %016lx",
> + aqm[0], aqm[1], aqm[2], aqm[3]);
> + ret &= bitmap_equal(adm,
> + (unsigned long *)crycb->apcb1.adm, 256);
> + VM_EVENT(kvm, 3, "TEST CRYCB: adm %016lx %016lx %016lx %016lx",
> + adm[0], adm[1], adm[2], adm[3]);
> + break;
> + case CRYCB_FORMAT1:
> + case CRYCB_FORMAT0: /* Fall through both use APCB0 */
> + ret = bitmap_equal(apm, (unsigned long *)crycb->apcb1.apm, 64);
> + ret &= bitmap_equal(aqm, (unsigned long *)crycb->apcb1.aqm, 16);
> + ret &= bitmap_equal(adm, (unsigned long *)crycb->apcb1.adm, 16);
> + VM_EVENT(kvm, 3, "TEST CRYCB: apm %016lx aqm %04x adm %04x",
> + apm[0], *((unsigned short *)aqm),
> + *((unsigned short *)adm));
> + break;
> + default: /* Can not happen */
> + ret = 0;
> + break;
> + }
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(kvm_arch_crypto_test_masks);
> +
> void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
> unsigned long *aqm, unsigned long *adm)
> {
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index f340a28c1d65..2a79d27d9730 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -42,12 +42,24 @@ MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>
> static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
> {
> - return 0;
> + struct ap_queue *apq = to_ap_queue(&apdev->device);
> + unsigned long apid = AP_QID_CARD(apq->qid);
> + unsigned long apqi = AP_QID_QUEUE(apq->qid);
> +
> + mutex_lock(&matrix_dev->lock);
> + vfio_ap_mdev_probe_queue(apid, apqi);
> + mutex_unlock(&matrix_dev->lock);
> }
>
> static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
> {
> - /* Nothing to do yet */
> + struct ap_queue *apq = to_ap_queue(&apdev->device);
> + unsigned long apid = AP_QID_CARD(apq->qid);
> + unsigned long apqi = AP_QID_QUEUE(apq->qid);
> +
> + mutex_lock(&matrix_dev->lock);
> + vfio_ap_mdev_remove_queue(apid, apqi);
> + mutex_unlock(&matrix_dev->lock);
> }
>
> static void vfio_ap_matrix_dev_release(struct device *dev)
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index ade2c150fe6b..8a70707bf870 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -683,8 +683,8 @@ static void vfio_ap_mdev_wait_for_qempty(unsigned long apid, unsigned long apqi)
> msleep(20);
> break;
> default:
> - pr_warn("%s: tapq err %02x: 0x%04x may not be empty\n",
> - __func__, status.response_code, q->apqn);
> + pr_warn("%s: tapq err %02x: %02lx%04lx may not be empty\n",
> + __func__, status.response_code, apid, apqi);
> return;
> }
> } while (--retry);
> @@ -840,3 +840,66 @@ void vfio_ap_mdev_unregister(void)
> {
> mdev_unregister_device(&matrix_dev->device);
> }
> +
> +static struct ap_matrix_mdev *vfio_ap_mdev_find_matrix_mdev(unsigned long apid,
> + unsigned long apqi)
> +{
> + struct ap_matrix_mdev *matrix_mdev;
> +
> + list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
> + if (test_bit_inv(apid, matrix_mdev->matrix.apm) &&
> + test_bit_inv(apqi, matrix_mdev->matrix.aqm))
> + return matrix_mdev;
> + }
> +
> + return NULL;
> +}
> +
> +void vfio_ap_mdev_remove_queue(unsigned long apid, unsigned long apqi)
> +{
> + struct ap_matrix_mdev *matrix_mdev;
> +
> + matrix_mdev = vfio_ap_mdev_find_matrix_mdev(apid, apqi);
> +
> + /*
> + * If the queue is assigned to the mdev device and the mdev device
> + * is in use by a guest
> + */
> + if (matrix_mdev && matrix_mdev->kvm) {
> + /*
> + * If the queue is plugged into the guest, unplug the adapter
> + * but keep the adapter assigned to the mdev device
> + */
> + if (!kvm_arch_crypto_test_masks(matrix_mdev->kvm,
> + matrix_mdev->matrix.apm,
> + matrix_mdev->matrix.aqm,
> + matrix_mdev->matrix.adm)) {
> + clear_bit_inv(apid, matrix_mdev->matrix.apm);
> + vfio_ap_mdev_update_crycb(matrix_mdev);
> + set_bit_inv(apid, matrix_mdev->matrix.apm);
> + }
> + }
> +
> + vfio_ap_mdev_reset_queue(apid, apqi);
> +}
> +
> +void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi)
> +{
> + struct ap_matrix_mdev *matrix_mdev;
> +
> + matrix_mdev = vfio_ap_mdev_find_matrix_mdev(apid, apqi);
> +
> + /*
> + * If the queue is assigned to the mdev device and the mdev device
> + * is in use by a guest
> + */
> + if (matrix_mdev && matrix_mdev->kvm) {
> + /* If the queue is not plugged into the guest, plug it in */
> + if (!kvm_arch_crypto_test_masks(matrix_mdev->kvm,
> + matrix_mdev->matrix.apm,
> + matrix_mdev->matrix.aqm,
> + matrix_mdev->matrix.adm)) {
> + vfio_ap_mdev_update_crycb(matrix_mdev);
> + }
> + }
> +}
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 76b7f98e47e9..acdd5bfabaaf 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -85,5 +85,7 @@ struct ap_matrix_mdev {
>
> extern int vfio_ap_mdev_register(void);
> extern void vfio_ap_mdev_unregister(void);
> +void vfio_ap_mdev_remove_queue(unsigned long apid, unsigned long apqi);
> +void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi);
>
> #endif /* _VFIO_AP_PRIVATE_H_ */
On Fri, 12 Apr 2019 08:54:54 +0200
Harald Freudenberger <[email protected]> wrote:
> On 11.04.19 23:03, Tony Krowiak wrote:
> > Introduces a new driver callback to prevent a root user from unbinding
> > an AP queue from its device driver if the queue is in use. This prevents
> > a root user from inadvertently taking a queue away from a guest and
> > giving it to the host, or vice versa. The callback will be invoked
> > whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
> > result in one or more AP queues being removed from its driver. If the
> > callback responds in the affirmative for any driver queried, the change
> > to the apmask or aqmask will be rejected with a device in use 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 manages AP queues passed through to one or more
> > guests and we don't want to unexpectedly take AP resources away from
> > guests which are most likely independently administered.
> >
> > Signed-off-by: Tony Krowiak <[email protected]>
>
> Hello Tony
>
> you are going out with your patches but ... what is the result of the discussions
> we had in the past ? Do we have a common understanding that we want to have
> it this way ? A driver which is able to claim resources and the bus code
> has lower precedence ?
I don't know about previous discussions, but I'm curious how you
arrived at this design. A driver being able to override the bus code
seems odd. Restricting this to 'non-default' drivers looks even more
odd.
What is this trying to solve? Traditionally, root has been able to
shoot any appendages of their choice. I would rather expect that in a
supported setup you'd have some management software keeping track of
device usage and making sure that only unused queues can be unbound. Do
we need to export more information to user space so that management
software can make better choices?
>
> > ---
> > drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
> > drivers/s390/crypto/ap_bus.h | 3 +
> > 2 files changed, 135 insertions(+), 6 deletions(-)
On 4/12/19 5:43 AM, Cornelia Huck wrote:
> On Fri, 12 Apr 2019 08:54:54 +0200
> Harald Freudenberger <[email protected]> wrote:
>
>> On 11.04.19 23:03, Tony Krowiak wrote:
>>> Introduces a new driver callback to prevent a root user from unbinding
>>> an AP queue from its device driver if the queue is in use. This prevents
>>> a root user from inadvertently taking a queue away from a guest and
>>> giving it to the host, or vice versa. The callback will be invoked
>>> whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
>>> result in one or more AP queues being removed from its driver. If the
>>> callback responds in the affirmative for any driver queried, the change
>>> to the apmask or aqmask will be rejected with a device in use 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 manages AP queues passed through to one or more
>>> guests and we don't want to unexpectedly take AP resources away from
>>> guests which are most likely independently administered.
>>>
>>> Signed-off-by: Tony Krowiak <[email protected]>
>>
>> Hello Tony
>>
>> you are going out with your patches but ... what is the result of the discussions
>> we had in the past ? Do we have a common understanding that we want to have
>> it this way ? A driver which is able to claim resources and the bus code
>> has lower precedence ?
This is what Reinhard suggested and what we agreed to as a team quite
some time ago. I submitted three versions of this patch to our
internal mailing list, all of which you reviewed, so I'm not sure
why you are surprised by this now.
>
> I don't know about previous discussions, but I'm curious how you
> arrived at this design. A driver being able to override the bus code
> seems odd. Restricting this to 'non-default' drivers looks even more
> odd.
>
> What is this trying to solve? Traditionally, root has been able to
> shoot any appendages of their choice. I would rather expect that in a
> supported setup you'd have some management software keeping track of
> device usage and making sure that only unused queues can be unbound. Do
> we need to export more information to user space so that management
> software can make better choices?
Is there a reason other than tradition to prevent root from accidentally
shooting himself in the foot or any other appendage? At present,
sysfs is the only supported setup, so IMHO it makes sense to prevent as
much accidentally caused damage as possible in the kernel.
>
>>
>>> ---
>>> drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
>>> drivers/s390/crypto/ap_bus.h | 3 +
>>> 2 files changed, 135 insertions(+), 6 deletions(-)
>
On Fri, 12 Apr 2019 15:38:21 -0400
Tony Krowiak <[email protected]> wrote:
> On 4/12/19 5:43 AM, Cornelia Huck wrote:
> > On Fri, 12 Apr 2019 08:54:54 +0200
> > Harald Freudenberger <[email protected]> wrote:
> >
> >> On 11.04.19 23:03, Tony Krowiak wrote:
> >>> Introduces a new driver callback to prevent a root user from unbinding
> >>> an AP queue from its device driver if the queue is in use. This prevents
> >>> a root user from inadvertently taking a queue away from a guest and
> >>> giving it to the host, or vice versa. The callback will be invoked
> >>> whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
> >>> result in one or more AP queues being removed from its driver. If the
> >>> callback responds in the affirmative for any driver queried, the change
> >>> to the apmask or aqmask will be rejected with a device in use 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 manages AP queues passed through to one or more
> >>> guests and we don't want to unexpectedly take AP resources away from
> >>> guests which are most likely independently administered.
> >>>
> >>> Signed-off-by: Tony Krowiak <[email protected]>
> >>
> >> Hello Tony
> >>
> >> you are going out with your patches but ... what is the result of the discussions
> >> we had in the past ? Do we have a common understanding that we want to have
> >> it this way ? A driver which is able to claim resources and the bus code
> >> has lower precedence ?
>
> This is what Reinhard suggested and what we agreed to as a team quite
> some time ago. I submitted three versions of this patch to our
> internal mailing list, all of which you reviewed, so I'm not sure
> why you are surprised by this now.
>
> >
> > I don't know about previous discussions, but I'm curious how you
> > arrived at this design. A driver being able to override the bus code
> > seems odd. Restricting this to 'non-default' drivers looks even more
> > odd.
> >
> > What is this trying to solve? Traditionally, root has been able to
> > shoot any appendages of their choice. I would rather expect that in a
> > supported setup you'd have some management software keeping track of
> > device usage and making sure that only unused queues can be unbound. Do
> > we need to export more information to user space so that management
> > software can make better choices?
>
> Is there a reason other than tradition to prevent root from accidentally
> shooting himself in the foot or any other appendage? At present,
> sysfs is the only supported setup, so IMHO it makes sense to prevent as
> much accidentally caused damage as possible in the kernel.
I don't think anybody wants an interface where it is easy for root to
accidentally cause damage... but from the patch description, it sounds
like you're creating an interface which makes it easy for a
badly-acting driver to hog resources without any way for root to remove
them forcefully.
Therefore, again my question: What is this trying to solve? I see a
management layer as a better place to make sure that queues are not
accidentally yanked from guests that are using them. Does more
information about queue usage need to be made available to userspace
for this to be feasible? Is anything else missing?
>
> >
> >>
> >>> ---
> >>> drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
> >>> drivers/s390/crypto/ap_bus.h | 3 +
> >>> 2 files changed, 135 insertions(+), 6 deletions(-)
> >
>
On 11/04/2019 23:03, Tony Krowiak wrote:
> Once an APQN is assigned to an mdev device it will remained assigned until
> it is explicitly unassigned from the mdev device. The associated AP queue
> devices, however, can come and go due to failures or deliberate actions by
> a sysadmin. For example, a sysadmin can dynamically remove an AP adapter
> card using the SE or by executing an SCLP command. This patch refactors
> the probe and remove callbacks of the vfio_ap driver to handle dynamic
> changes as follows:
>
> * Probe callback changes:
>
> If the APQN of the queue being probed is assigned to an mdev device, the
> mdev device is in use by a guest, and the APQN is not set in the guest's
> CRYCB, the CRYCB will be dynamically updated to give the guest access to
> the queue.
>
> * Remove callback changes:
>
> If the APQN of the queue being removed is assigned to an mdev device,
> the mdev
> device is in use by a guest, and the APQN is set in the guest's CRYCB,
> the CRYCB will be dynamically updated to remove the guest's access to
> the adapter card associated with the queue. Keep in mind, the
> architecture does not provide a way to remove access to a single queue
> unless only one queue is in the guest's configuration, so it was decided
> that it makes more sense to unplug the adapter from the guest.
>
> The APQN of the queue being removed will remain assigned to the mdev
> device should the queue be dynamically returned to the configuration.
> The queue will also be reset prior to returning control to the caller
> (a.k.a., the AP bus).
>
> Signed-off-by: Tony Krowiak <[email protected]>
> ---
> arch/s390/include/asm/kvm_host.h | 2 ++
> arch/s390/kvm/kvm-s390.c | 37 +++++++++++++++++++
> drivers/s390/crypto/vfio_ap_drv.c | 16 +++++++--
> drivers/s390/crypto/vfio_ap_ops.c | 67 +++++++++++++++++++++++++++++++++--
> drivers/s390/crypto/vfio_ap_private.h | 2 ++
> 5 files changed, 120 insertions(+), 4 deletions(-)
>
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index c47e22bba87f..0ce5d9b0df59 100644kvm_s390_crypto_cb
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -895,6 +895,8 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
> void kvm_arch_crypto_clear_masks(struct kvm *kvm);
> void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
> unsigned long *aqm, unsigned long *adm);
> +int kvm_arch_crypto_test_masks(struct kvm *kvm, unsigned long *apm,
> + unsigned long *aqm, unsigned long *adm);
>
> extern int sie64a(struct kvm_s390_sie_block *, u64 *);
> extern char sie_exit;
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 4638303ba6a8..5f423cdd29ba 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2217,6 +2217,43 @@ static void kvm_s390_set_crycb_format(struct kvm *kvm)
> kvm->arch.crypto.crycbd |= CRYCB_FORMAT1;
> }
>
This function requires the big matrix lock, may be add a comment.
> +int kvm_arch_crypto_test_masks(struct kvm *kvm, unsigned long *apm,
> + unsigned long *aqm, unsigned long *adm)
> +{
> + int ret;
> + struct kvm_s390_crypto_cb *crycb = kvm->arch.crypto.crycb;
> +
> + switch (kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
> + case CRYCB_FORMAT2: /* APCB1 use 256 bits */
> + ret = bitmap_equal(apm, (unsigned long *)crycb->apcb1.apm, 256);
> + VM_EVENT(kvm, 3, "TEST CRYCB: apm %016lx %016lx %016lx %016lx",
> + apm[0], apm[1], apm[2], apm[3]);
> + ret &= bitmap_equal(aqm,
> + (unsigned long *)crycb->apcb1.aqm, 256);
> + VM_EVENT(kvm, 3, "TEST CRYCB: aqm %016lx %016lx %016lx %016lx",
> + aqm[0], aqm[1], aqm[2], aqm[3]);
> + ret &= bitmap_equal(adm,
> + (unsigned long *)crycb->apcb1.adm, 256);
> + VM_EVENT(kvm, 3, "TEST CRYCB: adm %016lx %016lx %016lx %016lx",
> + adm[0], adm[1], adm[2], adm[3]);
> + break;
> + case CRYCB_FORMAT1:
> + case CRYCB_FORMAT0: /* Fall through both use APCB0 */
> + ret = bitmap_equal(apm, (unsigned long *)crycb->apcb1.apm, 64);
> + ret &= bitmap_equal(aqm, (unsigned long *)crycb->apcb1.aqm, 16);
> + ret &= bitmap_equal(adm, (unsigned long *)crycb->apcb1.adm, 16);
> + VM_EVENT(kvm, 3, "TEST CRYCB: apm %016lx aqm %04x adm %04x",
> + apm[0], *((unsigned short *)aqm),
> + *((unsigned short *)adm));
> + break;
> + default: /* Can not happen */
> + ret = 0;
> + break;
> + }
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(kvm_arch_crypto_test_masks);
Wouldn't it be interesting to work on the ap_matrix structure instead of
on the real CRYCB?
You could spare a lot of tests and wouldn't require to change this file.
Regards,
Pierre
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
On 4/15/19 5:50 AM, Cornelia Huck wrote:
> On Fri, 12 Apr 2019 15:38:21 -0400
> Tony Krowiak <[email protected]> wrote:
>
>> On 4/12/19 5:43 AM, Cornelia Huck wrote:
>>> On Fri, 12 Apr 2019 08:54:54 +0200
>>> Harald Freudenberger <[email protected]> wrote:
>>>
>>>> On 11.04.19 23:03, Tony Krowiak wrote:
>>>>> Introduces a new driver callback to prevent a root user from unbinding
>>>>> an AP queue from its device driver if the queue is in use. This prevents
>>>>> a root user from inadvertently taking a queue away from a guest and
>>>>> giving it to the host, or vice versa. The callback will be invoked
>>>>> whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
>>>>> result in one or more AP queues being removed from its driver. If the
>>>>> callback responds in the affirmative for any driver queried, the change
>>>>> to the apmask or aqmask will be rejected with a device in use 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 manages AP queues passed through to one or more
>>>>> guests and we don't want to unexpectedly take AP resources away from
>>>>> guests which are most likely independently administered.
>>>>>
>>>>> Signed-off-by: Tony Krowiak <[email protected]>
>>>>
>>>> Hello Tony
>>>>
>>>> you are going out with your patches but ... what is the result of the discussions
>>>> we had in the past ? Do we have a common understanding that we want to have
>>>> it this way ? A driver which is able to claim resources and the bus code
>>>> has lower precedence ?
>>
>> This is what Reinhard suggested and what we agreed to as a team quite
>> some time ago. I submitted three versions of this patch to our
>> internal mailing list, all of which you reviewed, so I'm not sure
>> why you are surprised by this now.
>>
>>>
>>> I don't know about previous discussions, but I'm curious how you
>>> arrived at this design. A driver being able to override the bus code
>>> seems odd. Restricting this to 'non-default' drivers looks even more
>>> odd.
>>>
>>> What is this trying to solve? Traditionally, root has been able to
>>> shoot any appendages of their choice. I would rather expect that in a
>>> supported setup you'd have some management software keeping track of
>>> device usage and making sure that only unused queues can be unbound. Do
>>> we need to export more information to user space so that management
>>> software can make better choices?
>>
>> Is there a reason other than tradition to prevent root from accidentally
>> shooting himself in the foot or any other appendage? At present,
>> sysfs is the only supported setup, so IMHO it makes sense to prevent as
>> much accidentally caused damage as possible in the kernel.
>
> I don't think anybody wants an interface where it is easy for root to
> accidentally cause damage... but from the patch description, it sounds
> like you're creating an interface which makes it easy for a
> badly-acting driver to hog resources without any way for root to remove
> them forcefully.
>
> Therefore, again my question: What is this trying to solve? I see a
> management layer as a better place to make sure that queues are not
> accidentally yanked from guests that are using them. Does more
> information about queue usage need to be made available to userspace
> for this to be feasible? Is anything else missing?
Accidentally yanking queues from guests is only part of the equation.
One has to consider the case where queues go away without root user
intervention. Take, for example, the case where an AP card temporarily
goes offline for some reason; possibly, due to glich or temporary
hardware malfunction of some sort. When the AP bus scan subsequently
runs, the card device and all associated queue devices will be unbound
from their respective device drivers. If the card then comes back
online, the next bus scan will recreate the card and queue devices and
bind them to their respective device drivers. These patches ensure the
queues are given back to the guest from which they were taken when
unbound.
Having said that, I understand your concern about a driver hogging
resources. I think I can provide a solution that serves both the
purpose of preventing problems associated with accidental removal
of AP resources as well as allowing root to remove them
forcefully. I'll work on that for v2.
>
>>
>>>
>>>>
>>>>> ---
>>>>> drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
>>>>> drivers/s390/crypto/ap_bus.h | 3 +
>>>>> 2 files changed, 135 insertions(+), 6 deletions(-)
>>>
>>
>
On Mon, 15 Apr 2019 12:51:23 -0400
Tony Krowiak <[email protected]> wrote:
> On 4/15/19 5:50 AM, Cornelia Huck wrote:
> > On Fri, 12 Apr 2019 15:38:21 -0400
> > Tony Krowiak <[email protected]> wrote:
> >
> >> On 4/12/19 5:43 AM, Cornelia Huck wrote:
> >>> On Fri, 12 Apr 2019 08:54:54 +0200
> >>> Harald Freudenberger <[email protected]> wrote:
> >>>
> >>>> On 11.04.19 23:03, Tony Krowiak wrote:
> >>>>> Introduces a new driver callback to prevent a root user from unbinding
> >>>>> an AP queue from its device driver if the queue is in use. This prevents
> >>>>> a root user from inadvertently taking a queue away from a guest and
> >>>>> giving it to the host, or vice versa. The callback will be invoked
> >>>>> whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
> >>>>> result in one or more AP queues being removed from its driver. If the
> >>>>> callback responds in the affirmative for any driver queried, the change
> >>>>> to the apmask or aqmask will be rejected with a device in use 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 manages AP queues passed through to one or more
> >>>>> guests and we don't want to unexpectedly take AP resources away from
> >>>>> guests which are most likely independently administered.
> >>>>>
> >>>>> Signed-off-by: Tony Krowiak <[email protected]>
> >>>>
> >>>> Hello Tony
> >>>>
> >>>> you are going out with your patches but ... what is the result of the discussions
> >>>> we had in the past ? Do we have a common understanding that we want to have
> >>>> it this way ? A driver which is able to claim resources and the bus code
> >>>> has lower precedence ?
> >>
> >> This is what Reinhard suggested and what we agreed to as a team quite
> >> some time ago. I submitted three versions of this patch to our
> >> internal mailing list, all of which you reviewed, so I'm not sure
> >> why you are surprised by this now.
> >>
> >>>
> >>> I don't know about previous discussions, but I'm curious how you
> >>> arrived at this design. A driver being able to override the bus code
> >>> seems odd. Restricting this to 'non-default' drivers looks even more
> >>> odd.
> >>>
> >>> What is this trying to solve? Traditionally, root has been able to
> >>> shoot any appendages of their choice. I would rather expect that in a
> >>> supported setup you'd have some management software keeping track of
> >>> device usage and making sure that only unused queues can be unbound. Do
> >>> we need to export more information to user space so that management
> >>> software can make better choices?
> >>
> >> Is there a reason other than tradition to prevent root from accidentally
> >> shooting himself in the foot or any other appendage? At present,
> >> sysfs is the only supported setup, so IMHO it makes sense to prevent as
> >> much accidentally caused damage as possible in the kernel.
> >
> > I don't think anybody wants an interface where it is easy for root to
> > accidentally cause damage... but from the patch description, it sounds
> > like you're creating an interface which makes it easy for a
> > badly-acting driver to hog resources without any way for root to remove
> > them forcefully.
> >
> > Therefore, again my question: What is this trying to solve? I see a
> > management layer as a better place to make sure that queues are not
> > accidentally yanked from guests that are using them. Does more
> > information about queue usage need to be made available to userspace
> > for this to be feasible? Is anything else missing?
>
> Accidentally yanking queues from guests is only part of the equation.
> One has to consider the case where queues go away without root user
> intervention. Take, for example, the case where an AP card temporarily
> goes offline for some reason; possibly, due to glich or temporary
> hardware malfunction of some sort. When the AP bus scan subsequently
> runs, the card device and all associated queue devices will be unbound
> from their respective device drivers. If the card then comes back
> online, the next bus scan will recreate the card and queue devices and
> bind them to their respective device drivers. These patches ensure the
> queues are given back to the guest from which they were taken when
> unbound.
Ok, that sounds a bit like the 'disconnected' state for ccw devices,
and this sounds more useful to me than trying to prevent root from
removing devices. (Put that explanation in the patch description,
maybe?)
>
> Having said that, I understand your concern about a driver hogging
> resources. I think I can provide a solution that serves both the
> purpose of preventing problems associated with accidental removal
> of AP resources as well as allowing root to remove them
> forcefully. I'll work on that for v2.
Ok, that and the explanation above makes this approach a lot more
reasonable.
>
> >
> >>
> >>>
> >>>>
> >>>>> ---
> >>>>> drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
> >>>>> drivers/s390/crypto/ap_bus.h | 3 +
> >>>>> 2 files changed, 135 insertions(+), 6 deletions(-)
> >>>
> >>
> >
>
On 4/11/19 5:03 PM, Tony Krowiak wrote:
> Once an APQN is assigned to an mdev device it will remained assigned until
> it is explicitly unassigned from the mdev device. The associated AP queue
> devices, however, can come and go due to failures or deliberate actions by
> a sysadmin. For example, a sysadmin can dynamically remove an AP adapter
> card using the SE or by executing an SCLP command. This patch refactors
> the probe and remove callbacks of the vfio_ap driver to handle dynamic
> changes as follows:
>
> * Probe callback changes:
>
> If the APQN of the queue being probed is assigned to an mdev device, the
> mdev device is in use by a guest, and the APQN is not set in the guest's
> CRYCB, the CRYCB will be dynamically updated to give the guest access to
> the queue.
>
> * Remove callback changes:
>
> If the APQN of the queue being removed is assigned to an mdev device,
> the mdev
> device is in use by a guest, and the APQN is set in the guest's CRYCB,
> the CRYCB will be dynamically updated to remove the guest's access to
> the adapter card associated with the queue. Keep in mind, the
> architecture does not provide a way to remove access to a single queue
> unless only one queue is in the guest's configuration, so it was decided
> that it makes more sense to unplug the adapter from the guest.
>
> The APQN of the queue being removed will remain assigned to the mdev
> device should the queue be dynamically returned to the configuration.
> The queue will also be reset prior to returning control to the caller
> (a.k.a., the AP bus).
>
> Signed-off-by: Tony Krowiak <[email protected]>
> ---
> arch/s390/include/asm/kvm_host.h | 2 ++
> arch/s390/kvm/kvm-s390.c | 37 +++++++++++++++++++
> drivers/s390/crypto/vfio_ap_drv.c | 16 +++++++--
> drivers/s390/crypto/vfio_ap_ops.c | 67 +++++++++++++++++++++++++++++++++--
> drivers/s390/crypto/vfio_ap_private.h | 2 ++
> 5 files changed, 120 insertions(+), 4 deletions(-)
>
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index c47e22bba87f..0ce5d9b0df59 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -895,6 +895,8 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
> void kvm_arch_crypto_clear_masks(struct kvm *kvm);
> void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
> unsigned long *aqm, unsigned long *adm);
> +int kvm_arch_crypto_test_masks(struct kvm *kvm, unsigned long *apm,
> + unsigned long *aqm, unsigned long *adm);
>
> extern int sie64a(struct kvm_s390_sie_block *, u64 *);
> extern char sie_exit;
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 4638303ba6a8..5f423cdd29ba 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2217,6 +2217,43 @@ static void kvm_s390_set_crycb_format(struct kvm *kvm)
> kvm->arch.crypto.crycbd |= CRYCB_FORMAT1;
> }
>
> +int kvm_arch_crypto_test_masks(struct kvm *kvm, unsigned long *apm,
> + unsigned long *aqm, unsigned long *adm)
> +{
> + int ret;
> + struct kvm_s390_crypto_cb *crycb = kvm->arch.crypto.crycb;
> +
> + switch (kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
> + case CRYCB_FORMAT2: /* APCB1 use 256 bits */
> + ret = bitmap_equal(apm, (unsigned long *)crycb->apcb1.apm, 256);
> + VM_EVENT(kvm, 3, "TEST CRYCB: apm %016lx %016lx %016lx %016lx",
> + apm[0], apm[1], apm[2], apm[3]);
> + ret &= bitmap_equal(aqm,
> + (unsigned long *)crycb->apcb1.aqm, 256);
> + VM_EVENT(kvm, 3, "TEST CRYCB: aqm %016lx %016lx %016lx %016lx",
> + aqm[0], aqm[1], aqm[2], aqm[3]);
> + ret &= bitmap_equal(adm,
> + (unsigned long *)crycb->apcb1.adm, 256);
> + VM_EVENT(kvm, 3, "TEST CRYCB: adm %016lx %016lx %016lx %016lx",
> + adm[0], adm[1], adm[2], adm[3]);
> + break;
> + case CRYCB_FORMAT1:
> + case CRYCB_FORMAT0: /* Fall through both use APCB0 */
> + ret = bitmap_equal(apm, (unsigned long *)crycb->apcb1.apm, 64);
> + ret &= bitmap_equal(aqm, (unsigned long *)crycb->apcb1.aqm, 16);
> + ret &= bitmap_equal(adm, (unsigned long *)crycb->apcb1.adm, 16);
All of the above need to access crycb->apcb0
> + VM_EVENT(kvm, 3, "TEST CRYCB: apm %016lx aqm %04x adm %04x",
> + apm[0], *((unsigned short *)aqm),
> + *((unsigned short *)adm));
> + break;
> + default: /* Can not happen */
> + ret = 0;
> + break;
> + }
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(kvm_arch_crypto_test_masks);
> +
> void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
> unsigned long *aqm, unsigned long *adm)
> {
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index f340a28c1d65..2a79d27d9730 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -42,12 +42,24 @@ MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>
> static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
> {
> - return 0;
> + struct ap_queue *apq = to_ap_queue(&apdev->device);
> + unsigned long apid = AP_QID_CARD(apq->qid);
> + unsigned long apqi = AP_QID_QUEUE(apq->qid);
> +
> + mutex_lock(&matrix_dev->lock);
> + vfio_ap_mdev_probe_queue(apid, apqi);
> + mutex_unlock(&matrix_dev->lock);
> }
>
> static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
> {
> - /* Nothing to do yet */
> + struct ap_queue *apq = to_ap_queue(&apdev->device);
> + unsigned long apid = AP_QID_CARD(apq->qid);
> + unsigned long apqi = AP_QID_QUEUE(apq->qid);
> +
> + mutex_lock(&matrix_dev->lock);
> + vfio_ap_mdev_remove_queue(apid, apqi);
> + mutex_unlock(&matrix_dev->lock);
> }
>
> static void vfio_ap_matrix_dev_release(struct device *dev)
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index ade2c150fe6b..8a70707bf870 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -683,8 +683,8 @@ static void vfio_ap_mdev_wait_for_qempty(unsigned long apid, unsigned long apqi)
> msleep(20);
> break;
> default:
> - pr_warn("%s: tapq err %02x: 0x%04x may not be empty\n",
> - __func__, status.response_code, q->apqn);
> + pr_warn("%s: tapq err %02x: %02lx%04lx may not be empty\n",
> + __func__, status.response_code, apid, apqi);
> return;
> }
> } while (--retry);
> @@ -840,3 +840,66 @@ void vfio_ap_mdev_unregister(void)
> {
> mdev_unregister_device(&matrix_dev->device);
> }
> +
> +static struct ap_matrix_mdev *vfio_ap_mdev_find_matrix_mdev(unsigned long apid,
> + unsigned long apqi)
> +{
> + struct ap_matrix_mdev *matrix_mdev;
> +
> + list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
> + if (test_bit_inv(apid, matrix_mdev->matrix.apm) &&
> + test_bit_inv(apqi, matrix_mdev->matrix.aqm))
> + return matrix_mdev;
> + }
> +
> + return NULL;
> +}
> +
> +void vfio_ap_mdev_remove_queue(unsigned long apid, unsigned long apqi)
> +{
> + struct ap_matrix_mdev *matrix_mdev;
> +
> + matrix_mdev = vfio_ap_mdev_find_matrix_mdev(apid, apqi);
> +
> + /*
> + * If the queue is assigned to the mdev device and the mdev device
> + * is in use by a guest
> + */
> + if (matrix_mdev && matrix_mdev->kvm) {
> + /*
> + * If the queue is plugged into the guest, unplug the adapter
> + * but keep the adapter assigned to the mdev device
> + */
> + if (!kvm_arch_crypto_test_masks(matrix_mdev->kvm,
> + matrix_mdev->matrix.apm,
> + matrix_mdev->matrix.aqm,
> + matrix_mdev->matrix.adm)) {
> + clear_bit_inv(apid, matrix_mdev->matrix.apm);
> + vfio_ap_mdev_update_crycb(matrix_mdev);
> + set_bit_inv(apid, matrix_mdev->matrix.apm);
> + }
> + }
> +
> + vfio_ap_mdev_reset_queue(apid, apqi);
> +}
> +
> +void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi)
> +{
> + struct ap_matrix_mdev *matrix_mdev;
> +
> + matrix_mdev = vfio_ap_mdev_find_matrix_mdev(apid, apqi);
> +
> + /*
> + * If the queue is assigned to the mdev device and the mdev device
> + * is in use by a guest
> + */
> + if (matrix_mdev && matrix_mdev->kvm) {
> + /* If the queue is not plugged into the guest, plug it in */
> + if (!kvm_arch_crypto_test_masks(matrix_mdev->kvm,
> + matrix_mdev->matrix.apm,
> + matrix_mdev->matrix.aqm,
> + matrix_mdev->matrix.adm)) {
> + vfio_ap_mdev_update_crycb(matrix_mdev);
> + }
> + }
> +}
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 76b7f98e47e9..acdd5bfabaaf 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -85,5 +85,7 @@ struct ap_matrix_mdev {
>
> extern int vfio_ap_mdev_register(void);
> extern void vfio_ap_mdev_unregister(void);
> +void vfio_ap_mdev_remove_queue(unsigned long apid, unsigned long apqi);
> +void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi);
>
> #endif /* _VFIO_AP_PRIVATE_H_ */
>
On Mon, 15 Apr 2019 12:51:23 -0400
Tony Krowiak <[email protected]> wrote:
> Having said that, I understand your concern about a driver hogging
> resources. I think I can provide a solution that serves both the
> purpose of preventing problems associated with accidental removal
> of AP resources as well as allowing root to remove them
> forcefully. I'll work on that for v2.
Can you tell us some more about this solution? Should we stop reviewing
v1 because v2 is going to be different anyway?
Regards,
Halil
On 4/15/19 2:59 PM, Halil Pasic wrote:
> On Mon, 15 Apr 2019 12:51:23 -0400
> Tony Krowiak <[email protected]> wrote:
>
>> Having said that, I understand your concern about a driver hogging
>> resources. I think I can provide a solution that serves both the
>> purpose of preventing problems associated with accidental removal
>> of AP resources as well as allowing root to remove them
>> forcefully. I'll work on that for v2.
>
> Can you tell us some more about this solution? Should we stop reviewing
> v1 because v2 is going to be different anyway?
Patch 1 and 2 will be removed. There will not be a major design change
between these patches and v2. In order to avoid a long explanation of
my proposed changes, I'd prefer to state that the patch set will
establish and enforce the following rules:
1. An APQN can be assigned to an mdev device iff it is NOT
reserved for use by a zcrypt driver and is not assigned to
another mdev device.
2. Once an APQN is assigned to an mdev device, it will remain
assigned until it is explicitly unassigned.
3. A queue's APQN can be set in the guest's CRYCB iff the APQN is
assigned to the mdev device used by the guest; however, if the
queue is also in the host configuration (i.e., online), it MUST
also be bound to the vfio_ap device driver.
4. When a queue is bound to the vfio_ap driver and its APQN
is assigned to an mdev device in use by a guest, the guest will
be given access to the queue.
5. When a queue is unbound from the vfio_ap driver and its APQN
is assigned to an mdev device in use by the guest, access to
the card containing the queue will be removed from the guest.
Keep in mind that we can not deny access to a specific queue
due to the architecture (i.e., clearing a bit in the AQM
removes access to the queue for all adapters)
6. When an adapter is assigned to an mdev device that is in use
by a guest, the guest will be given access to the adapter.
7. When an adapter is unassigned from an mdev device that is in use
by a guest, access to the adapter will removed from the guest.
8. When a domain is assigned to an mdev device that is in use
by a guest, the guest will be given access to the domain.
9. When a domain is unassigned from an mdev device that is in use
by a guest, access to the domain will removed from the guest.
>
> Regards,
> Halil
>
On 11/04/2019 23:03, Tony Krowiak wrote:
> Introduces a new driver callback to prevent a root user from unbinding
> an AP queue from its device driver if the queue is in use. This prevents
> a root user from inadvertently taking a queue away from a guest and
> giving it to the host, or vice versa. The callback will be invoked
> whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
> result in one or more AP queues being removed from its driver. If the
> callback responds in the affirmative for any driver queried, the change
> to the apmask or aqmask will be rejected with a device in use 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 manages AP queues passed through to one or more
> guests and we don't want to unexpectedly take AP resources away from
> guests which are most likely independently administered.
>
> Signed-off-by: Tony Krowiak <[email protected]>
> ---
> drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
> drivers/s390/crypto/ap_bus.h | 3 +
> 2 files changed, 135 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
> index 1546389d71db..66a5a9d9fae6 100644
> --- a/drivers/s390/crypto/ap_bus.c
> +++ b/drivers/s390/crypto/ap_bus.c
> @@ -35,6 +35,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"
> @@ -980,9 +981,11 @@ int ap_parse_mask_str(const char *str,
> newmap = kmalloc(size, GFP_KERNEL);
> if (!newmap)
> return -ENOMEM;
> - if (mutex_lock_interruptible(lock)) {
> - kfree(newmap);
> - return -ERESTARTSYS;
> + if (lock) {
> + if (mutex_lock_interruptible(lock)) {
> + kfree(newmap);
> + return -ERESTARTSYS;
> + }
> }
>
> if (*str == '+' || *str == '-') {
> @@ -994,7 +997,10 @@ int ap_parse_mask_str(const char *str,
> }
> if (rc == 0)
> memcpy(bitmap, newmap, size);
> - mutex_unlock(lock);
> +
> + if (lock)
> + mutex_unlock(lock);
> +
> kfree(newmap);
> return rc;
> }
> @@ -1181,12 +1187,72 @@ 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;
> +
> + /*
> + * If the reserved bits do not identify cards 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;
I prefer you suppress this asymmetry with the assumption that the
"default driver" will not register a "in_use" callback.
> +
> + /* Pin the driver's module code */
> + if (!try_module_get(drv->owner))
> + return 0;
> +
> + if (ap_drv->in_use)
> + if (ap_drv->in_use(newapm, ap_perms.aqm))
> + rc = -EADDRINUSE;
> +
> + module_put(drv->owner);
> +
> + return rc;
> +}
> +
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
On 4/16/19 3:52 AM, Pierre Morel wrote:
> On 11/04/2019 23:03, Tony Krowiak wrote:
>> Introduces a new driver callback to prevent a root user from unbinding
>> an AP queue from its device driver if the queue is in use. This prevents
>> a root user from inadvertently taking a queue away from a guest and
>> giving it to the host, or vice versa. The callback will be invoked
>> whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
>> result in one or more AP queues being removed from its driver. If the
>> callback responds in the affirmative for any driver queried, the change
>> to the apmask or aqmask will be rejected with a device in use 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 manages AP queues passed through to one or more
>> guests and we don't want to unexpectedly take AP resources away from
>> guests which are most likely independently administered.
>>
>> Signed-off-by: Tony Krowiak <[email protected]>
>> ---
>> drivers/s390/crypto/ap_bus.c | 138
>> +++++++++++++++++++++++++++++++++++++++++--
>> drivers/s390/crypto/ap_bus.h | 3 +
>> 2 files changed, 135 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
>> index 1546389d71db..66a5a9d9fae6 100644
>> --- a/drivers/s390/crypto/ap_bus.c
>> +++ b/drivers/s390/crypto/ap_bus.c
>> @@ -35,6 +35,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"
>> @@ -980,9 +981,11 @@ int ap_parse_mask_str(const char *str,
>> newmap = kmalloc(size, GFP_KERNEL);
>> if (!newmap)
>> return -ENOMEM;
>> - if (mutex_lock_interruptible(lock)) {
>> - kfree(newmap);
>> - return -ERESTARTSYS;
>> + if (lock) {
>> + if (mutex_lock_interruptible(lock)) {
>> + kfree(newmap);
>> + return -ERESTARTSYS;
>> + }
>> }
>> if (*str == '+' || *str == '-') {
>> @@ -994,7 +997,10 @@ int ap_parse_mask_str(const char *str,
>> }
>> if (rc == 0)
>> memcpy(bitmap, newmap, size);
>> - mutex_unlock(lock);
>> +
>> + if (lock)
>> + mutex_unlock(lock);
>> +
>> kfree(newmap);
>> return rc;
>> }
>> @@ -1181,12 +1187,72 @@ 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;
>> +
>> + /*
>> + * If the reserved bits do not identify cards 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;
>
> I prefer you suppress this asymmetry with the assumption that the
> "default driver" will not register a "in_use" callback.
Based on comments by Connie, I plan on removing this patch from the
next version.
>
>
>> +
>> + /* Pin the driver's module code */
>> + if (!try_module_get(drv->owner))
>> + return 0;
>> +
>> + if (ap_drv->in_use)
>> + if (ap_drv->in_use(newapm, ap_perms.aqm))
>> + rc = -EADDRINUSE;
>> +
>> + module_put(drv->owner);
>> +
>> + return rc;
>> +}
>> +
>
On 16/04/2019 15:11, Tony Krowiak wrote:
> On 4/16/19 3:52 AM, Pierre Morel wrote:
>> On 11/04/2019 23:03, Tony Krowiak wrote:
>>> Introduces a new driver callback to prevent a root user from unbinding
>>> an AP queue from its device driver if the queue is in use. This prevents
>>> a root user from inadvertently taking a queue away from a guest and
>>> giving it to the host, or vice versa. The callback will be invoked
>>> whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
>>> result in one or more AP queues being removed from its driver. If the
>>> callback responds in the affirmative for any driver queried, the change
>>> to the apmask or aqmask will be rejected with a device in use 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 manages AP queues passed through to one or more
>>> guests and we don't want to unexpectedly take AP resources away from
>>> guests which are most likely independently administered.
>>>
>>> Signed-off-by: Tony Krowiak <[email protected]>
>>> ---
>>> drivers/s390/crypto/ap_bus.c | 138
>>> +++++++++++++++++++++++++++++++++++++++++--
>>> drivers/s390/crypto/ap_bus.h | 3 +
>>> 2 files changed, 135 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
>>> index 1546389d71db..66a5a9d9fae6 100644
>>> --- a/drivers/s390/crypto/ap_bus.c
>>> +++ b/drivers/s390/crypto/ap_bus.c
>>> @@ -35,6 +35,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"
>>> @@ -980,9 +981,11 @@ int ap_parse_mask_str(const char *str,
>>> newmap = kmalloc(size, GFP_KERNEL);
>>> if (!newmap)
>>> return -ENOMEM;
>>> - if (mutex_lock_interruptible(lock)) {
>>> - kfree(newmap);
>>> - return -ERESTARTSYS;
>>> + if (lock) {
>>> + if (mutex_lock_interruptible(lock)) {
>>> + kfree(newmap);
>>> + return -ERESTARTSYS;
>>> + }
>>> }
>>> if (*str == '+' || *str == '-') {
>>> @@ -994,7 +997,10 @@ int ap_parse_mask_str(const char *str,
>>> }
>>> if (rc == 0)
>>> memcpy(bitmap, newmap, size);
>>> - mutex_unlock(lock);
>>> +
>>> + if (lock)
>>> + mutex_unlock(lock);
>>> +
>>> kfree(newmap);
>>> return rc;
>>> }
>>> @@ -1181,12 +1187,72 @@ 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;
>>> +
>>> + /*
>>> + * If the reserved bits do not identify cards 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;
>>
>> I prefer you suppress this asymmetry with the assumption that the
>> "default driver" will not register a "in_use" callback.
>
> Based on comments by Connie, I plan on removing this patch from the
> next version.
Yes it was the goal.
>
>>
>>
>>> +
>>> + /* Pin the driver's module code */
>>> + if (!try_module_get(drv->owner))
>>> + return 0;
>>> +
>>> + if (ap_drv->in_use)
>>> + if (ap_drv->in_use(newapm, ap_perms.aqm))
>>> + rc = -EADDRINUSE;
>>> +
>>> + module_put(drv->owner);
>>> +
>>> + return rc;
>>> +}
>>> +
>>
>
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
On Mon, 15 Apr 2019 18:43:24 -0400
Tony Krowiak <[email protected]> wrote:
> On 4/15/19 2:59 PM, Halil Pasic wrote:
> > On Mon, 15 Apr 2019 12:51:23 -0400
> > Tony Krowiak <[email protected]> wrote:
> >
> >> Having said that, I understand your concern about a driver hogging
> >> resources. I think I can provide a solution that serves both the
> >> purpose of preventing problems associated with accidental removal
> >> of AP resources as well as allowing root to remove them
> >> forcefully. I'll work on that for v2.
> >
> > Can you tell us some more about this solution? Should we stop reviewing
> > v1 because v2 is going to be different anyway?
>
> Patch 1 and 2 will be removed. There will not be a major design change
> between these patches and v2. In order to avoid a long explanation of
> my proposed changes, I'd prefer to state that the patch set will
> establish and enforce the following rules:
>
> 1. An APQN can be assigned to an mdev device iff it is NOT
> reserved for use by a zcrypt driver and is not assigned to
> another mdev device.
>
> 2. Once an APQN is assigned to an mdev device, it will remain
> assigned until it is explicitly unassigned.
>
> 3. A queue's APQN can be set in the guest's CRYCB iff the APQN is
> assigned to the mdev device used by the guest; however, if the
> queue is also in the host configuration (i.e., online), it MUST
> also be bound to the vfio_ap device driver.
>
> 4. When a queue is bound to the vfio_ap driver and its APQN
> is assigned to an mdev device in use by a guest, the guest will
> be given access to the queue.
>
> 5. When a queue is unbound from the vfio_ap driver and its APQN
> is assigned to an mdev device in use by the guest, access to
> the card containing the queue will be removed from the guest.
> Keep in mind that we can not deny access to a specific queue
> due to the architecture (i.e., clearing a bit in the AQM
> removes access to the queue for all adapters)
>
> 6. When an adapter is assigned to an mdev device that is in use
> by a guest, the guest will be given access to the adapter.
>
> 7. When an adapter is unassigned from an mdev device that is in use
> by a guest, access to the adapter will removed from the guest.
>
> 8. When a domain is assigned to an mdev device that is in use
> by a guest, the guest will be given access to the domain.
>
> 9. When a domain is unassigned from an mdev device that is in use
> by a guest, access to the domain will removed from the guest.
>
Based on our off-the-list chat and this list I think I know
where are you heading :). I think it's actually the design that I
currently prefer the most. But in that case, it may be wise to touch
base with Reinhard -- AFAIR he was the strongest proponent of the 'do
not let a[pq]mask changes take away queues from guests' design.
Regards,
Halil