| Message ID | 1534196899-16987-14-git-send-email-akrowiak@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | guest dedicated crypto adapters | expand |
On Mon, 13 Aug 2018 17:48:10 -0400 Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote: > From: Tony Krowiak <akrowiak@linux.ibm.com> > > Provides a sysfs interface to view the AP matrix configured for the > mediated matrix device. > > The relevant sysfs structures are: > > /sys/devices/vfio_ap/matrix/ > ...... [mdev_supported_types] > ......... [vfio_ap-passthrough] > ............ [devices] > ...............[$uuid] > .................. matrix > > To view the matrix configured for the mediated matrix device, > print the matrix file: > > cat matrix > > Below are examples of the output from the above command: > > Example 1: Adapters and domains assigned > Assignments: > Adapters 5 and 6 > Domains 4 and 71 (0x47) > > Output > 05.0004 > 05.0047 > 06.0004 > 06.0047 > > Examples 2: Only adapters assigned > Assignments: > Adapters 5 and 6 > > Output: > 05. > 06. > > Examples 3: Only domains assigned > Assignments: > Domains 4 and 71 (0x47) > > Output: > .0004 > .0047 I find this output to be a bit confusing; but OTOH, I'm probably not the person to parse it :) Still, some comments. From previous discussions, ISTR that this is mainly supposed to be a debugging/administration aid. Of course, this generates some questions: - Should this be in sysfs (sysfs attributes are supposed to follow the "one value per file" rule, at least for the most part), or would debugfs be a better fit? - Should userspace code be able to introspect the current configuration? If yes, it might be better to have some not-so-nice-but-easily-parsable output, possibly one attribute for the assigned adapters and one for the assigned domains, and a tool which distills that into a nice "matrix" with labeled rows and columns for human consumption. That said, I don't really have major objections to that interface. > > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > Reviewed-by: Halil Pasic <pasic@linux.ibm.com> > Tested-by: Michael Mueller <mimu@linux.ibm.com> > Tested-by: Farhan Ali <alifm@linux.ibm.com> > Tested-by: Pierre Morel <pmorel@linux.ibm.com> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > drivers/s390/crypto/vfio_ap_ops.c | 52 +++++++++++++++++++++++++++++++++++++ > 1 files changed, 52 insertions(+), 0 deletions(-)
On 08/20/2018 10:08 AM, Cornelia Huck wrote: > I find this output to be a bit confusing; but OTOH, I'm probably not > the person to parse it :) Still, some comments. > > From previous discussions, ISTR that this is mainly supposed to be a > debugging/administration aid. Of course, this generates some questions: > - Should this be in sysfs (sysfs attributes are supposed to follow the > "one value per file" rule, at least for the most part), or would > debugfs be a better fit? > - Should userspace code be able to introspect the current > configuration? If yes, it might be better to have some > not-so-nice-but-easily-parsable output, possibly one attribute for > the assigned adapters and one for the assigned domains, and a tool > which distills that into a nice "matrix" with labeled rows and > columns for human consumption. > > That said, I don't really have major objections to that interface. For now, I will leave it in, but we could conceivably simplify configuration of the matrix and adhere to sysfs rules by modeling this after the AP bus apmask and aqmask sysfs interfaces. If we did that, there would need to be only three RW sysfs interfaces: apm or apmask or ap_mask or adapter_mask or apid_mask aqm or aqmask or aq_mask or domains or usage_domain_mask or apqi_mask adm or admask or ad_mask or control_domains or control_domain_mask or domain_mask To assign a device, either a bitmask or a comma separated list of IDs prepended with a '+' could be passed in. To unassign a device, either a bitmask in or a comma-separated list of IDs prepended with a '-' could be passed in. Reading an attribute would return the mask. As you suggested, tools could be provided to parse the output and display it in a human-readable format. > >> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >> Reviewed-by: Halil Pasic <pasic@linux.ibm.com> >> Tested-by: Michael Mueller <mimu@linux.ibm.com> >> Tested-by: Farhan Ali <alifm@linux.ibm.com> >> Tested-by: Pierre Morel <pmorel@linux.ibm.com> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> drivers/s390/crypto/vfio_ap_ops.c | 52 +++++++++++++++++++++++++++++++++++++ >> 1 files changed, 52 insertions(+), 0 deletions(-)
On 09/12/2018 07:01 PM, Tony Krowiak wrote: > On 08/20/2018 10:08 AM, Cornelia Huck wrote: >> I find this output to be a bit confusing; but OTOH, I'm probably not >> the person to parse it :) Still, some comments. >> >> From previous discussions, ISTR that this is mainly supposed to be a >> debugging/administration aid. Of course, this generates some questions: >> - Should this be in sysfs (sysfs attributes are supposed to follow the >> "one value per file" rule, at least for the most part), or would >> debugfs be a better fit? >> - Should userspace code be able to introspect the current >> configuration? If yes, it might be better to have some >> not-so-nice-but-easily-parsable output, possibly one attribute for >> the assigned adapters and one for the assigned domains, and a tool >> which distills that into a nice "matrix" with labeled rows and >> columns for human consumption. >> >> That said, I don't really have major objections to that interface. > > For now, I will leave it in, but we could conceivably simplify configuration > of the matrix and adhere to sysfs rules by modeling this after the AP bus > apmask and aqmask sysfs interfaces. If we did that, there would need to be only > three RW sysfs interfaces: > > apm or apmask or ap_mask or adapter_mask or apid_mask > aqm or aqmask or aq_mask or domains or usage_domain_mask or apqi_mask > adm or admask or ad_mask or control_domains or control_domain_mask or domain_mask > > To assign a device, either a bitmask or a comma separated list of IDs prepended > with a '+' could be passed in. To unassign a device, either a bitmask in or a > comma-separated list of IDs prepended with a '-' could be passed in. > > Reading an attribute would return the mask. As you suggested, tools could be > provided to parse the output and display it in a human-readable format. > I would prefer an interface where one can set at least one whole mask in one go over the current one that let us manipulate just one bit at a time. Hallil >> >>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >>> Reviewed-by: Halil Pasic <pasic@linux.ibm.com> >>> Tested-by: Michael Mueller <mimu@linux.ibm.com> >>> Tested-by: Farhan Ali <alifm@linux.ibm.com> >>> Tested-by: Pierre Morel <pmorel@linux.ibm.com> >>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>> --- >>> drivers/s390/crypto/vfio_ap_ops.c | 52 +++++++++++++++++++++++++++++++++++++ >>> 1 files changed, 52 insertions(+), 0 deletions(-) > >
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index f732177..af3b55f 100644 --- a/drivers/s390/crypto/vfio_ap_ops.c +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -552,6 +552,57 @@ static ssize_t control_domains_show(struct device *dev, } DEVICE_ATTR_RO(control_domains); +static ssize_t matrix_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct mdev_device *mdev = mdev_from_dev(dev); + struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); + char *bufpos = buf; + unsigned long apid; + unsigned long apqi; + unsigned long apid1; + unsigned long apqi1; + unsigned long napm_bits = matrix_mdev->matrix.apm_max + 1; + unsigned long naqm_bits = matrix_mdev->matrix.aqm_max + 1; + int nchars = 0; + int n; + + apid1 = find_first_bit_inv(matrix_mdev->matrix.apm, napm_bits); + apqi1 = find_first_bit_inv(matrix_mdev->matrix.aqm, naqm_bits); + + mutex_lock(&matrix_dev.lock); + + if ((apid1 < napm_bits) && (apqi1 < naqm_bits)) { + for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, napm_bits) { + for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, + naqm_bits) { + n = sprintf(bufpos, "%02lx.%04lx\n", apid, + apqi); + bufpos += n; + nchars += n; + } + } + } else if (apid1 < napm_bits) { + for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, napm_bits) { + n = sprintf(bufpos, "%02lx.\n", apid); + bufpos += n; + nchars += n; + } + } else if (apqi1 < naqm_bits) { + for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, naqm_bits) { + n = sprintf(bufpos, ".%04lx\n", apqi); + bufpos += n; + nchars += n; + } + } + + mutex_unlock(&matrix_dev.lock); + + return nchars; +} +DEVICE_ATTR_RO(matrix); + + static struct attribute *vfio_ap_mdev_attrs[] = { &dev_attr_assign_adapter.attr, &dev_attr_unassign_adapter.attr, @@ -560,6 +611,7 @@ static ssize_t control_domains_show(struct device *dev, &dev_attr_assign_control_domain.attr, &dev_attr_unassign_control_domain.attr, &dev_attr_control_domains.attr, + &dev_attr_matrix.attr, NULL, };