On 16.08.2018 12:07, Cornelia Huck wrote:
> Hi,
>
> I noticed that a new version of the default/alternate driver patch(es)
> seems to have made its way onto the s390/features branch. As I did not
> see any posting of that patch, and it has interaction with vfio-ap,
> I'll repost the patch here in full for convenience and add some
> comments below. (At least, the documentation for vfio-ap will need an
> update if it is rebased on top of this patch.)
>
> One thing on top: What about control domains? The vfio-ap interface
> allows to assign control domains as well. Do we want to be able to have
> any control domain available to be used for sending control commands by
> guests, or do we want to restrict usage of some control domains as well?
>
>> commit fc303d54f83b22b0f2b40e85f8f3e56658d59bc9
>> Author: Harald Freudenberger <freude@xxxxxxxxxxxxx>
>> Date: Fri Jul 20 08:36:53 2018 +0200
>>
>> s390/zcrypt: AP bus support for alternate driver(s)
>>
>> The current AP bus, AP devices and AP device drivers implementation
>> uses a clearly defined mapping for binding AP devices to AP device
>> drivers. So for example a CEX6C queue will always be bound to the
>> cex4queue device driver.
>>
>> The Linux Device Driver model has no sensitivity for more than one
>> device driver eligible for one device type. If there exist more than
>> one drivers matching to the device type, simple all drivers are tried
>> consecutively. There is no way to determine and influence the probing
>> order of the drivers.
>>
>> With KVM there is a need to provide additional device drivers matching
>> to the very same type of AP devices. With a simple implementation the
>> KVM drivers run in competition to the regular drivers. Whichever
>> 'wins' a device depends on build order and implementation details
>> within the common Linux Device Driver Model and is not
>> deterministic. However, a userspace process could figure out which
>> device should be bound to which driver and sort out the correct
>> binding by manipulating attributes in the sysfs.
>>
>> If for security reasons a AP device must not get bound to the 'wrong'
>> device driver the sorting out has to be done within the Linux kernel
>> by the AP bus code. This patch modifies the behavior of the AP bus
>> for probing drivers for devices in a way that two sets of drivers are
>> usable. Two new bitmasks 'apmask' and 'aqmask' are used to mark a
>> subset of the APQN range for 'usable by the ap bus and the default
>> drivers' or 'not usable by the default drivers and thus available for
>> alternate drivers like vfio-xxx'. So an APQN which is addressed by
>> this masking only the default drivers will be probed. In contrary an
>> APQN which is not addressed by the masks will never be probed and
>> bound to default drivers but onny to alternate drivers.
> s/onny/only/
>
> (already in my original comments)
>
>>
>> Eventually the two masks give a way to divide the range of APQNs into
>> two pools: one pool of APQNs used by the AP bus and the default
>> drivers and thus via zcrypt drivers available to the userspace of the
>> system. And another pool where no zcrypt drivers are bound to and
>> which can be used by alternate drivers (like vfio-xxx) for their
>> needs. This division is hot-plug save and makes sure a APQN assigned
>> to an alternate driver is at no time somehow exploitable by the wrong
>> party.
>>
>> The two masks are located in sysfs at /sys/bus/ap/apmask and
>> /sys/bus/ap/aqmask. The mask syntax is exactly the same as the
>> already existing mask attributes in the /sys/bus/ap directory (for
>> example ap_usage_domain_mask and ap_control_domain_mask).
>>
>> By default all APQNs belong to the ap bus and the default drivers:
>>
>> cat /sys/bus/ap/apmask
>> 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>> cat /sys/bus/ap/aqmask
>> 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
>>
>> The masks can be changed at boot time with the kernel command line
>> like this:
>>
>> ... ap.apmask=0xffff ap.aqmask=0x40
>>
>> This would give these two pools:
>>
>> default drivers pool: adapter 0 - 15, domain 1
>> alternate drivers pool: adapter 0 - 15, all but domain 1
>> adapter 16-255, all domains
>>
>> The sysfs attributes for this two masks are writeable and an
>> administrator is able to reconfigure the assignements on the fly by
>> writing new mask values into. With changing the mask(s) a revision of
>> the existing queue to driver bindings is done. So all APQNs which are
>> bound to the 'wrong' driver are reprobed via kernel function
>> device_reprobe() and thus the new correct driver will be assigned with
>> respect of the changed apmask and aqmask bits.
>>
>> The mask values are bitmaps in big endian order starting with bit 0.
>> So adapter number 0 is the leftmost bit, mask is 0x8000... The sysfs
>> attributes accept 2 different formats:
>> - Absolute hex string starting with 0x like "0x12345678" does set
>> the mask starting from left to right. If the given string is shorter
>> than the mask it is padded with 0s on the right. If the string is
>> longer than the mask an error comes back (EINVAL).
>> - '+' or '-' followed by a numerical value. Valid examples are "+1",
>> "-13", "+0x41", "-0xff" and even "+0" and "-0". Only the addressed
>> bit in the mask is switched on ('+') or off ('-').
>>
>> This patch will also be the base for an upcoming extension to the
>> zcrypt drivers to be able to provide additional zcrypt device nodes
>> with filtering based on ap and aq masks.
>>
>> Signed-off-by: Harald Freudenberger <freude@xxxxxxxxxxxxx>
>> Signed-off-by: Martin Schwidefsky <schwidefsky@xxxxxxxxxx>
>> Signed-off-by: Heiko Carstens <heiko.carstens@xxxxxxxxxx>
>>
>> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
>> index bf27fc4d1335..b023e35786ec 100644
>> --- a/drivers/s390/crypto/ap_bus.c
>> +++ b/drivers/s390/crypto/ap_bus.c
>> @@ -34,6 +34,7 @@
>> #include <linux/crypto.h>
>> #include <linux/mod_devicetable.h>
>> #include <linux/debugfs.h>
>> +#include <linux/ctype.h>
>>
>> #include "ap_bus.h"
>> #include "ap_debug.h"
>> @@ -51,11 +52,26 @@ static int ap_thread_flag = 0;
>> module_param_named(poll_thread, ap_thread_flag, int, S_IRUSR|S_IRGRP);
>> MODULE_PARM_DESC(poll_thread, "Turn on/off poll thread, default is 0 (off).");
>>
>> +static char *apm_str;
>> +module_param_named(apmask, apm_str, charp, 0440);
>> +MODULE_PARM_DESC(apmask, "AP bus adapter mask.");
>> +
>> +static char *aqm_str;
>> +module_param_named(aqmask, aqm_str, charp, 0440);
>> +MODULE_PARM_DESC(aqmask, "AP bus domain mask.");
>> +
>> static struct device *ap_root_device;
>>
>> DEFINE_SPINLOCK(ap_list_lock);
>> LIST_HEAD(ap_card_list);
>>
>> +/* Default permissions (card and domain masking) */
>> +static struct ap_perms {
>> + DECLARE_BITMAP(apm, AP_DEVICES);
>> + DECLARE_BITMAP(aqm, AP_DOMAINS);
>> +} ap_perms;
>> +static DEFINE_MUTEX(ap_perms_mutex);
>> +
>> static struct ap_config_info *ap_configuration;
>> static bool initialised;
>>
>> @@ -666,11 +682,96 @@ static struct bus_type ap_bus_type = {
>> .pm = &ap_bus_pm_ops,
>> };
>>
>> +static int __ap_revice_reserved(struct device *dev, void *dummy)
>
> This probably wants to be either "device" or "revise"...
>
>> +{
>> + int rc, card, queue, devres, drvres;
>> +
>> + if (is_queue_dev(dev)) {
>> + card = AP_QID_CARD(to_ap_queue(dev)->qid);
>> + queue = AP_QID_QUEUE(to_ap_queue(dev)->qid);
>> + mutex_lock(&ap_perms_mutex);
>> + devres = test_bit_inv(card, ap_perms.apm)
>> + && test_bit_inv(queue, ap_perms.aqm);
>> + mutex_unlock(&ap_perms_mutex);
>> + drvres = to_ap_drv(dev->driver)->flags
>> + & AP_DRIVER_FLAG_DEFAULT;
>> + if (!!devres != !!drvres) {
>> + AP_DBF(DBF_DEBUG, "reprobing queue=%02x.%04x\n",
>> + card, queue);
>> + rc = device_reprobe(dev);
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void ap_bus_revise_bindings(void)
>> +{
>> + bus_for_each_dev(&ap_bus_type, NULL, NULL, __ap_revice_reserved);
>> +}
>> +
>> +int ap_owned_by_def_drv(int card, int queue)
>> +{
>> + int rc = 0;
>> +
>> + if (card < 0 || card >= AP_DEVICES || queue < 0 || queue >= AP_DOMAINS)
>> + return -EINVAL;
>> +
>> + mutex_lock(&ap_perms_mutex);
>> +
>> + if (test_bit_inv(card, ap_perms.apm)
>> + && test_bit_inv(queue, ap_perms.aqm))
>> + rc = 1;
>> +
>> + mutex_unlock(&ap_perms_mutex);
>> +
>> + return rc;
>> +}
>> +EXPORT_SYMBOL(ap_owned_by_def_drv);
>> +
>> +int ap_apqn_in_matrix_owned_by_def_drv(unsigned long *apm,
>> + unsigned long *aqm)
>> +{
>> + int card, queue, rc = 0;
>> +
>> + mutex_lock(&ap_perms_mutex);
>> +
>> + for (card = 0; !rc && card < AP_DEVICES; card++)
>> + if (test_bit_inv(card, apm) &&
>> + test_bit_inv(card, ap_perms.apm))
>> + for (queue = 0; !rc && queue < AP_DOMAINS; queue++)
>> + if (test_bit_inv(queue, aqm) &&
>> + test_bit_inv(queue, ap_perms.aqm))
>> + rc = 1;
>> +
>> + mutex_unlock(&ap_perms_mutex);
>> +
>> + return rc;
>> +}
>> +EXPORT_SYMBOL(ap_apqn_in_matrix_owned_by_def_drv);
>> +
>> static int ap_device_probe(struct device *dev)
>> {
>> struct ap_device *ap_dev = to_ap_dev(dev);
>> struct ap_driver *ap_drv = to_ap_drv(dev->driver);
>> - int rc;
>> + int card, queue, devres, drvres, rc;
>> +
>> + if (is_queue_dev(dev)) {
>> + /*
>> + * For apqns marked as reserved/used by ap bus and
>> + * default drivers only drivers with a default flag
>> + * will be called for probe this device.
>> + */
>> + card = AP_QID_CARD(to_ap_queue(dev)->qid);
>> + queue = AP_QID_QUEUE(to_ap_queue(dev)->qid);
>> + mutex_lock(&ap_perms_mutex);
>> + devres = test_bit_inv(card, ap_perms.apm)
>> + && test_bit_inv(queue, ap_perms.aqm);
>> + mutex_unlock(&ap_perms_mutex);
>> + drvres = ap_drv->flags & AP_DRIVER_FLAG_DEFAULT;
>> + if (!!devres != !!drvres)
>> + return -ENODEV;
>> + }
> I think the following comments from my original review still apply:
>
> "It's a bit more than that, right? You also won't probe non-reserved
> devices if the driver has the default flag set.
>
> /*
> * If the apqn is marked as reserved/used by ap bus and
> * default drivers, only probe with drivers with the default
> * flag set. If it is not marked, only probe with drivers
> * with the default flag not set.
> */
>
> [As an aside: Is 'default' a good name for the driver flag? To a casual
> reader I think it implies that it is the default driver if no other
> driver comes along. Call it maybe 'trusted' or something like that?]"
>
>>
>> /* Add queue/card to list of active queues/cards */
>> spin_lock_bh(&ap_list_lock);
>> @@ -753,6 +854,36 @@ EXPORT_SYMBOL(ap_bus_force_rescan);
>> /*
>> * AP bus attributes.
>> */
>> +
>> +static int hex2bitmap(const char *str, unsigned long *bitmap, int bits)
>> +{
>> + int i, n, b;
>> +
>> + /* bits needs to be a multiple of 8 */
>> + if (bits & 0x07)
>> + return -EINVAL;
>> +
>> + memset(bitmap, 0, bits / 8);
>> +
>> + if (str[0] == '0' && str[1] == 'x')
>> + str++;
>> + if (*str == 'x')
>> + str++;
>> +
>> + for (i = 0; isxdigit(*str) && i < bits; str++) {
>> + b = hex_to_bin(*str);
>> + for (n = 0; n < 4; n++)
>> + if (b & (0x08 >> n))
>> + set_bit_inv(i + n, bitmap);
>> + i += 4;
>> + }
>> +
>> + if (i < 4 || isxdigit(*str))
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> static ssize_t ap_domain_show(struct bus_type *bus, char *buf)
>> {
>> return snprintf(buf, PAGE_SIZE, "%d\n", ap_domain_index);
>> @@ -764,7 +895,8 @@ static ssize_t ap_domain_store(struct bus_type *bus,
>> int domain;
>>
>> if (sscanf(buf, "%i\n", &domain) != 1 ||
>> - domain < 0 || domain > ap_max_domain_id)
>> + domain < 0 || domain > ap_max_domain_id ||
>> + !test_bit_inv(domain, ap_perms.aqm))
>> return -EINVAL;
>> spin_lock_bh(&ap_domain_lock);
>> ap_domain_index = domain;
>> @@ -901,6 +1033,114 @@ static ssize_t ap_max_domain_id_show(struct bus_type *bus, char *buf)
>>
>> static BUS_ATTR(ap_max_domain_id, 0444, ap_max_domain_id_show, NULL);
>>
>> +static ssize_t ap_apmask_show(struct bus_type *bus, char *buf)
>> +{
>> + int rc;
>> +
>> + if (mutex_lock_interruptible(&ap_perms_mutex))
>> + return -ERESTARTSYS;
>> + rc = snprintf(buf, PAGE_SIZE,
>> + "0x%016lx%016lx%016lx%016lx\n",
>> + ap_perms.apm[0], ap_perms.apm[1],
>> + ap_perms.apm[2], ap_perms.apm[3]);
>> + mutex_unlock(&ap_perms_mutex);
>> +
>> + return rc;
>> +}
>> +
>> +static ssize_t ap_apmask_store(struct bus_type *bus, const char *buf,
>> + size_t count)
>> +{
>> + int i;
>> +
>> + if (*buf == '+' || *buf == '-') {
>> + if (kstrtoint(buf, 0, &i))
>> + return -EINVAL;
>> + if (i <= -AP_DEVICES || i >= AP_DEVICES)
>> + return -EINVAL;
>> + if (mutex_lock_interruptible(&ap_perms_mutex))
>> + return -ERESTARTSYS;
>> + if (*buf == '-')
>> + clear_bit_inv(-i, ap_perms.apm);
>> + else
>> + set_bit_inv(i, ap_perms.apm);
>> + } else {
>> + DECLARE_BITMAP(apm, AP_DEVICES);
>> +
>> + i = hex2bitmap(buf, apm, AP_DEVICES);
>> + if (i)
>> + return i;
>> + if (mutex_lock_interruptible(&ap_perms_mutex))
>> + return -ERESTARTSYS;
>> + for (i = 0; i < AP_DEVICES; i++)
>> + if (test_bit_inv(i, apm))
>> + set_bit_inv(i, ap_perms.apm);
>> + else
>> + clear_bit_inv(i, ap_perms.apm);
>> + }
>> + mutex_unlock(&ap_perms_mutex);
>> +
>> + ap_bus_revise_bindings();
>> +
>> + return count;
>> +}
>> +
>> +static BUS_ATTR(apmask, 0644, ap_apmask_show, ap_apmask_store);
>> +
>> +static ssize_t ap_aqmask_show(struct bus_type *bus, char *buf)
>> +{
>> + int rc;
>> +
>> + if (mutex_lock_interruptible(&ap_perms_mutex))
>> + return -ERESTARTSYS;
>> + rc = snprintf(buf, PAGE_SIZE,
>> + "0x%016lx%016lx%016lx%016lx\n",
>> + ap_perms.aqm[0], ap_perms.aqm[1],
>> + ap_perms.aqm[2], ap_perms.aqm[3]);
>> + mutex_unlock(&ap_perms_mutex);
>> +
>> + return rc;
>> +}
>> +
>> +static ssize_t ap_aqmask_store(struct bus_type *bus, const char *buf,
>> + size_t count)
>> +{
>> + int i;
>> +
>> + if (*buf == '+' || *buf == '-') {
>> + if (kstrtoint(buf, 0, &i))
>> + return -EINVAL;
>> + if (i <= -AP_DEVICES || i >= AP_DEVICES)
>> + return -EINVAL;
>> + if (mutex_lock_interruptible(&ap_perms_mutex))
>> + return -ERESTARTSYS;
>> + if (*buf == '-')
>> + clear_bit_inv(-i, ap_perms.aqm);
>> + else
>> + set_bit_inv(i, ap_perms.aqm);
>> + } else {
>> + DECLARE_BITMAP(aqm, AP_DEVICES);
>> +
>> + i = hex2bitmap(buf, aqm, AP_DEVICES);
>> + if (i)
>> + return i;
>> + if (mutex_lock_interruptible(&ap_perms_mutex))
>> + return -ERESTARTSYS;
>> + for (i = 0; i < AP_DEVICES; i++)
>> + if (test_bit_inv(i, aqm))
>> + set_bit_inv(i, ap_perms.aqm);
>> + else
>> + clear_bit_inv(i, ap_perms.aqm);
>> + }
>> + mutex_unlock(&ap_perms_mutex);
>> +
>> + ap_bus_revise_bindings();
>> +
>> + return count;
>> +}
> There a two things that bother me a bit here:
> - The sysfs interface accepts both the hex format and the +/- format,
> but the module parameters only the hex format. Probably not a big
> deal, but it looks a bit inconsistent.
> - With the more end-user friendly +/- interface, you can always only
> add/remove one domain/queue at once. For every change, it will walk
> along the bus and do the reprobing, if needed. It would be more
> effective if you could trigger all needed changes at once, without
> being forced to resort to the hex interface for that.
>
>> +
>> +static BUS_ATTR(aqmask, 0644, ap_aqmask_show, ap_aqmask_store);
>> +
>> static struct bus_attribute *const ap_bus_attrs[] = {
>> &bus_attr_ap_domain,
>> &bus_attr_ap_control_domain_mask,
>> @@ -910,6 +1150,8 @@ static struct bus_attribute *const ap_bus_attrs[] = {
>> &bus_attr_ap_interrupts,
>> &bus_attr_poll_timeout,
>> &bus_attr_ap_max_domain_id,
>> + &bus_attr_apmask,
>> + &bus_attr_aqmask,
>> NULL,
>> };
>>
>> @@ -938,7 +1180,8 @@ static int ap_select_domain(void)
>> best_domain = -1;
>> max_count = 0;
>> for (i = 0; i < AP_DOMAINS; i++) {
>> - if (!ap_test_config_domain(i))
>> + if (!ap_test_config_domain(i) ||
>> + !test_bit_inv(i, ap_perms.aqm))
>> continue;
>> count = 0;
>> for (j = 0; j < AP_DEVICES; j++) {
>> @@ -1187,6 +1430,37 @@ static int __init ap_debug_init(void)
>> return 0;
>> }
>>
>> +static void __init ap_perms_init(void)
>> +{
>> + int i, rc;
>> +
>> + /* start with all resources useable */
>> + memset(&ap_perms.apm, 0xFF, sizeof(ap_perms.apm));
>> + memset(&ap_perms.aqm, 0xFF, sizeof(ap_perms.aqm));
>> +
>> + /* process kernel parameters apm and aqm if given */
>> + if (apm_str) {
>> + DECLARE_BITMAP(apm, AP_DEVICES);
>> +
>> + rc = hex2bitmap(apm_str, apm, AP_DEVICES);
>> + if (rc == 0) {
>> + for (i = 0; i < AP_DEVICES; i++)
>> + if (!test_bit_inv(i, apm))
>> + clear_bit_inv(i, ap_perms.apm);
>> + }
>> + }
>> + if (aqm_str) {
>> + DECLARE_BITMAP(aqm, AP_DOMAINS);
>> +
>> + rc = hex2bitmap(aqm_str, aqm, AP_DOMAINS);
>> + if (rc == 0) {
>> + for (i = 0; i < AP_DOMAINS; i++)
>> + if (!test_bit_inv(i, aqm))
>> + clear_bit_inv(i, ap_perms.aqm);
>> + }
>> + }
>> +}
>> +
>> /**
>> * ap_module_init(): The module initialization code.
>> *
>> @@ -1206,6 +1480,9 @@ static int __init ap_module_init(void)
>> return -ENODEV;
>> }
>>
>> + /* set up the AP permissions (ap and aq masks) */
>> + ap_perms_init();
>> +
>> /* Get AP configuration data if available */
>> ap_init_configuration();
>>
>> @@ -1214,7 +1491,9 @@ static int __init ap_module_init(void)
>> ap_max_domain_id ? ap_max_domain_id : AP_DOMAINS - 1;
>> else
>> max_domain_id = 15;
>> - if (ap_domain_index < -1 || ap_domain_index > max_domain_id) {
>> + if (ap_domain_index < -1 || ap_domain_index > max_domain_id ||
>> + (ap_domain_index >= 0 &&
>> + !test_bit_inv(ap_domain_index, ap_perms.aqm))) {
>> pr_warn("%d is not a valid cryptographic domain\n",
>> ap_domain_index);
>> ap_domain_index = -1;
>> diff --git a/drivers/s390/crypto/ap_bus.h b/drivers/s390/crypto/ap_bus.h
>> index 936541937e15..380612762134 100644
>> --- a/drivers/s390/crypto/ap_bus.h
>> +++ b/drivers/s390/crypto/ap_bus.h
>> @@ -117,9 +117,18 @@ enum ap_wait {
>> struct ap_device;
>> struct ap_message;
>>
>> +/*
>> + * The ap driver struct includes a flags field which holds some info for
>> + * the ap bus about the driver. Currently only one flag is supported and
>> + * used: The DEFAULT flag marks an ap driver as a default driver which is
>> + * used together with the apmask and aqmask whitelisting of the ap bus.
>> + */
>> +#define AP_DRIVER_FLAG_DEFAULT 0x0001
>> +
>> struct ap_driver {
>> struct device_driver driver;
>> struct ap_device_id *ids;
>> + unsigned int flags;
>>
>> int (*probe)(struct ap_device *);
>> void (*remove)(struct ap_device *);
>> @@ -248,4 +257,27 @@ void ap_queue_resume(struct ap_device *ap_dev);
>> struct ap_card *ap_card_create(int id, int queue_depth, int raw_device_type,
>> int comp_device_type, unsigned int functions);
>>
>> +/*
>> + * check APQN for owned/reserved by ap bus and default driver(s).
>> + * Checks if this APQN is or will be in use by the ap bus
>> + * and the default set of drivers.
>> + * If yes, returns 1, if not returns 0. On error a negative
>> + * errno value is returned.
>> + */
>> +int ap_owned_by_def_drv(int card, int queue);
>> +
>> +/*
>> + * check 'matrix' of APQNs for owned/reserved by ap bus and
>> + * default driver(s).
>> + * Checks if there is at least one APQN in the given 'matrix'
>> + * marked as owned/reserved by the ap bus and default driver(s).
>> + * If such an APQN is found the return value is 1, otherwise
>> + * 0 is returned. On error a negative errno value is returned.
>> + * The parameter apm is a bitmask which should be declared
>> + * as DECLARE_BITMAP(apm, AP_DEVICES), the aqm parameter is
>> + * similar, should be declared as DECLARE_BITMAP(aqm, AP_DOMAINS).
>> + */
>> +int ap_apqn_in_matrix_owned_by_def_drv(unsigned long *apm,
>> + unsigned long *aqm);
>> +
>> #endif /* _AP_BUS_H_ */
>> diff --git a/drivers/s390/crypto/zcrypt_cex2a.c b/drivers/s390/crypto/zcrypt_cex2a.c
>> index e701194d3611..f4ae5fa30ec9 100644
>> --- a/drivers/s390/crypto/zcrypt_cex2a.c
>> +++ b/drivers/s390/crypto/zcrypt_cex2a.c
>> @@ -145,6 +145,7 @@ static struct ap_driver zcrypt_cex2a_card_driver = {
>> .probe = zcrypt_cex2a_card_probe,
>> .remove = zcrypt_cex2a_card_remove,
>> .ids = zcrypt_cex2a_card_ids,
>> + .flags = AP_DRIVER_FLAG_DEFAULT,
>> };
>>
>> /**
>> @@ -208,6 +209,7 @@ static struct ap_driver zcrypt_cex2a_queue_driver = {
>> .suspend = ap_queue_suspend,
>> .resume = ap_queue_resume,
>> .ids = zcrypt_cex2a_queue_ids,
>> + .flags = AP_DRIVER_FLAG_DEFAULT,
>> };
>>
>> int __init zcrypt_cex2a_init(void)
>> diff --git a/drivers/s390/crypto/zcrypt_cex4.c b/drivers/s390/crypto/zcrypt_cex4.c
>> index f305538334ad..35d58dbbc4da 100644
>> --- a/drivers/s390/crypto/zcrypt_cex4.c
>> +++ b/drivers/s390/crypto/zcrypt_cex4.c
>> @@ -214,6 +214,7 @@ static struct ap_driver zcrypt_cex4_card_driver = {
>> .probe = zcrypt_cex4_card_probe,
>> .remove = zcrypt_cex4_card_remove,
>> .ids = zcrypt_cex4_card_ids,
>> + .flags = AP_DRIVER_FLAG_DEFAULT,
>> };
>>
>> /**
>> @@ -283,6 +284,7 @@ static struct ap_driver zcrypt_cex4_queue_driver = {
>> .suspend = ap_queue_suspend,
>> .resume = ap_queue_resume,
>> .ids = zcrypt_cex4_queue_ids,
>> + .flags = AP_DRIVER_FLAG_DEFAULT,
>> };
>>
>> int __init zcrypt_cex4_init(void)
>> diff --git a/drivers/s390/crypto/zcrypt_pcixcc.c b/drivers/s390/crypto/zcrypt_pcixcc.c
>> index 159b0a0dd211..e5e14d723163 100644
>> --- a/drivers/s390/crypto/zcrypt_pcixcc.c
>> +++ b/drivers/s390/crypto/zcrypt_pcixcc.c
>> @@ -223,6 +223,7 @@ static struct ap_driver zcrypt_pcixcc_card_driver = {
>> .probe = zcrypt_pcixcc_card_probe,
>> .remove = zcrypt_pcixcc_card_remove,
>> .ids = zcrypt_pcixcc_card_ids,
>> + .flags = AP_DRIVER_FLAG_DEFAULT,
>> };
>>
>> /**
>> @@ -286,6 +287,7 @@ static struct ap_driver zcrypt_pcixcc_queue_driver = {
>> .suspend = ap_queue_suspend,
>> .resume = ap_queue_resume,
>> .ids = zcrypt_pcixcc_queue_ids,
>> + .flags = AP_DRIVER_FLAG_DEFAULT,
>> };
>>
>> int __init zcrypt_pcixcc_init(void)
I've sent a patch to Heiko/Martin for upstream integration which fixes some of the issues you mentioned:
And I am working on a patch which makes it possible to give ranges for the bitmap manipulations.
regards
Harald Freudenberger
>>====================<<
From: Harald Freudenberger <freude@xxxxxxxxxxxxx>
Date: Fri, 17 Aug 2018 09:01:09 +0200
Subject: [PATCH] s390/zcrypt: Fix returncode type, typo and block comment
Minor cleanup patch which fixes 3 complains from upstream review:
- function ap_instructions_available() had returntype int but
in fact returned 1 for true and 0 for false. Changed returntype
to bool.
- Typo in static function name __ap_revice_reserved.
revice -> revise for name and the code which called it.
- Extension on a block comment regarding how devices and drivers
with respect of the default flag are handled.
Signed-off-by: Harald Freudenberger <freude@xxxxxxxxxxxxx>
---
arch/s390/include/asm/ap.h | 6 +++---
drivers/s390/crypto/ap_bus.c | 11 ++++++-----
2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/arch/s390/include/asm/ap.h b/arch/s390/include/asm/ap.h
index 887494aa164f..8c00fd509c45 100644
--- a/arch/s390/include/asm/ap.h
+++ b/arch/s390/include/asm/ap.h
@@ -49,9 +49,9 @@ struct ap_queue_status {
/**
* ap_intructions_available() - Test if AP instructions are available.
*
- * Returns 1 if the AP instructions are installed, otherwise 0.
+ * Returns true if the AP instructions are installed, otherwise false.
*/
-static inline int ap_instructions_available(void)
+static inline bool ap_instructions_available(void)
{
register unsigned long reg0 asm ("0") = AP_MKQID(0, 0);
register unsigned long reg1 asm ("1") = 0;
@@ -65,7 +65,7 @@ static inline int ap_instructions_available(void)
: "+d" (reg1), "+d" (reg2)
: "d" (reg0)
: "cc");
- return reg1;
+ return reg1 != 0;
}
/**
diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
index b1b73945d746..b739a97f6064 100644
--- a/drivers/s390/crypto/ap_bus.c
+++ b/drivers/s390/crypto/ap_bus.c
@@ -682,7 +682,7 @@ static struct bus_type ap_bus_type = {
.pm = &ap_bus_pm_ops,
};
-static int __ap_revice_reserved(struct device *dev, void *dummy)
+static int __ap_revise_reserved(struct device *dev, void *dummy)
{
int rc, card, queue, devres, drvres;
@@ -707,7 +707,7 @@ static int __ap_revice_reserved(struct device *dev, void *dummy)
static void ap_bus_revise_bindings(void)
{
- bus_for_each_dev(&ap_bus_type, NULL, NULL, __ap_revice_reserved);
+ bus_for_each_dev(&ap_bus_type, NULL, NULL, __ap_revise_reserved);
}
int ap_owned_by_def_drv(int card, int queue)
@@ -758,9 +758,10 @@ static int ap_device_probe(struct device *dev)
if (is_queue_dev(dev)) {
/*
- * For apqns marked as reserved/used by ap bus and
- * default drivers only drivers with a default flag
- * will be called for probe this device.
+ * If the apqn is marked as reserved/used by ap bus and
+ * default drivers, only probe with drivers with the default
+ * flag set. If it is not marked, only probe with drivers
+ * with the default flag not set.
*/
card = AP_QID_CARD(to_ap_queue(dev)->qid);
queue = AP_QID_QUEUE(to_ap_queue(dev)->qid);
--
2.17.1
