| Message ID | 20201022171209.19494-8-akrowiak@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390/vfio-ap: dynamic configuration support | expand |
On Thu, 22 Oct 2020 13:12:02 -0400 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > +static ssize_t guest_matrix_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + ssize_t nchars; > + struct mdev_device *mdev = mdev_from_dev(dev); > + struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); > + > + if (!vfio_ap_mdev_has_crycb(matrix_mdev)) > + return -ENODEV; I'm wondering, would it make sense to have guest_matrix display the would be guest matrix when we don't have a KVM? With the filtering in place, the question in what guest_matrix would my (assign) matrix result right now if I were to hook up my vfio_ap_mdev to a guest seems a legitimate one. > + > + mutex_lock(&matrix_dev->lock); > + nchars = vfio_ap_mdev_matrix_show(&matrix_mdev->shadow_apcb, buf); > + mutex_unlock(&matrix_dev->lock); > + > + return nchars; > +} > +static DEVICE_ATTR_RO(guest_matrix);
On 10/28/20 4:17 AM, Halil Pasic wrote: > On Thu, 22 Oct 2020 13:12:02 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> +static ssize_t guest_matrix_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + ssize_t nchars; >> + struct mdev_device *mdev = mdev_from_dev(dev); >> + struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); >> + >> + if (!vfio_ap_mdev_has_crycb(matrix_mdev)) >> + return -ENODEV; > I'm wondering, would it make sense to have guest_matrix display the would > be guest matrix when we don't have a KVM? With the filtering in > place, the question in what guest_matrix would my (assign) matrix result > right now if I were to hook up my vfio_ap_mdev to a guest seems a > legitimate one. A couple of thoughts here: * The ENODEV informs the user that there is no guest running which makes sense to me given this interface displays the guest matrix. The alternative, which I considered, was to display an empty matrix (i.e., nothing). * This would be a pretty drastic change to the design because the shadow_apcb - which is what is displayed via this interface - is only updated when the guest is started and while it is running (i.e., hot plug of new adapters/domains). Making this change would require changing that entire design concept which I am reluctant to do at this point in the game. > > >> + >> + mutex_lock(&matrix_dev->lock); >> + nchars = vfio_ap_mdev_matrix_show(&matrix_mdev->shadow_apcb, buf); >> + mutex_unlock(&matrix_dev->lock); >> + >> + return nchars; >> +} >> +static DEVICE_ATTR_RO(guest_matrix);
On Fri, 13 Nov 2020 12:27:32 -0500 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > > > On 10/28/20 4:17 AM, Halil Pasic wrote: > > On Thu, 22 Oct 2020 13:12:02 -0400 > > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > > > >> +static ssize_t guest_matrix_show(struct device *dev, > >> + struct device_attribute *attr, char *buf) > >> +{ > >> + ssize_t nchars; > >> + struct mdev_device *mdev = mdev_from_dev(dev); > >> + struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); > >> + > >> + if (!vfio_ap_mdev_has_crycb(matrix_mdev)) > >> + return -ENODEV; > > I'm wondering, would it make sense to have guest_matrix display the would > > be guest matrix when we don't have a KVM? With the filtering in > > place, the question in what guest_matrix would my (assign) matrix result > > right now if I were to hook up my vfio_ap_mdev to a guest seems a > > legitimate one. > > A couple of thoughts here: > * The ENODEV informs the user that there is no guest running > which makes sense to me given this interface displays the > guest matrix. The alternative, which I considered, was to > display an empty matrix (i.e., nothing). > * This would be a pretty drastic change to the design because > the shadow_apcb - which is what is displayed via this interface - is > only updated when the guest is started and while it is running (i.e., > hot plug of new adapters/domains). Making this change would > require changing that entire design concept which I am reluctant > to do at this point in the game. > > No problem. My thinking was, that, because we can do the assign/unassing ops also for the running guest, that we also have the code to do the maintenance on the shadow_apcb. In this series this code is conditional with respect to vfio_ap_mdev_has_crycb(). E.g. static ssize_t assign_adapter_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { [..] if (vfio_ap_mdev_has_crycb(matrix_mdev)) if (vfio_ap_mdev_filter_guest_matrix(matrix_mdev, true)) vfio_ap_mdev_commit_shadow_apcb(matrix_mdev); If one were to move the vfio_ap_mdev_has_crycb() check into vfio_ap_mdev_commit_shadow_apcb() then we would have an always up to date shatdow_apcb, we could display. I don't feel strongly about this. Was just an idea, because if the result of the filtering is surprising, currently the only to see, without knowing the algorithm, and possibly the state, and the history of the system, is to actually start a guest. Regards, Halil
On 11/13/20 6:12 PM, Halil Pasic wrote: > On Fri, 13 Nov 2020 12:27:32 -0500 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> >> On 10/28/20 4:17 AM, Halil Pasic wrote: >>> On Thu, 22 Oct 2020 13:12:02 -0400 >>> Tony Krowiak <akrowiak@linux.ibm.com> wrote: >>> >>>> +static ssize_t guest_matrix_show(struct device *dev, >>>> + struct device_attribute *attr, char *buf) >>>> +{ >>>> + ssize_t nchars; >>>> + struct mdev_device *mdev = mdev_from_dev(dev); >>>> + struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); >>>> + >>>> + if (!vfio_ap_mdev_has_crycb(matrix_mdev)) >>>> + return -ENODEV; >>> I'm wondering, would it make sense to have guest_matrix display the would >>> be guest matrix when we don't have a KVM? With the filtering in >>> place, the question in what guest_matrix would my (assign) matrix result >>> right now if I were to hook up my vfio_ap_mdev to a guest seems a >>> legitimate one. >> A couple of thoughts here: >> * The ENODEV informs the user that there is no guest running >> which makes sense to me given this interface displays the >> guest matrix. The alternative, which I considered, was to >> display an empty matrix (i.e., nothing). >> * This would be a pretty drastic change to the design because >> the shadow_apcb - which is what is displayed via this interface - is >> only updated when the guest is started and while it is running (i.e., >> hot plug of new adapters/domains). Making this change would >> require changing that entire design concept which I am reluctant >> to do at this point in the game. >> >> > No problem. My thinking was, that, because we can do the > assign/unassing ops also for the running guest, that we also have > the code to do the maintenance on the shadow_apcb. In this > series this code is conditional with respect to vfio_ap_mdev_has_crycb(). > E.g. > > static ssize_t assign_adapter_store(struct device *dev, > struct device_attribute *attr, > const char *buf, size_t count) > { > [..] > if (vfio_ap_mdev_has_crycb(matrix_mdev)) > if (vfio_ap_mdev_filter_guest_matrix(matrix_mdev, true)) > vfio_ap_mdev_commit_shadow_apcb(matrix_mdev); > > If one were to move the > vfio_ap_mdev_has_crycb() check into vfio_ap_mdev_commit_shadow_apcb() > then we would have an always up to date shatdow_apcb, we could display. > > I don't feel strongly about this. Was just an idea, because if the result > of the filtering is surprising, currently the only to see, without > knowing the algorithm, and possibly the state, and the history of the > system, is to actually start a guest. Okay, I can buy this and will make the change. > > Regards, > Halil >
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index 9791761aa7fd..7bad70d7bcef 100644 --- a/drivers/s390/crypto/vfio_ap_ops.c +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -1073,29 +1073,24 @@ static ssize_t control_domains_show(struct device *dev, } static DEVICE_ATTR_RO(control_domains); -static ssize_t matrix_show(struct device *dev, struct device_attribute *attr, - char *buf) +static ssize_t vfio_ap_mdev_matrix_show(struct ap_matrix *matrix, 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; + unsigned long napm_bits = matrix->apm_max + 1; + unsigned long naqm_bits = 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); + apid1 = find_first_bit_inv(matrix->apm, napm_bits); + apqi1 = find_first_bit_inv(matrix->aqm, naqm_bits); 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, + for_each_set_bit_inv(apid, matrix->apm, napm_bits) { + for_each_set_bit_inv(apqi, matrix->aqm, naqm_bits) { n = sprintf(bufpos, "%02lx.%04lx\n", apid, apqi); @@ -1104,25 +1099,55 @@ static ssize_t matrix_show(struct device *dev, struct device_attribute *attr, } } } else if (apid1 < napm_bits) { - for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, napm_bits) { + for_each_set_bit_inv(apid, 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) { + for_each_set_bit_inv(apqi, matrix->aqm, naqm_bits) { n = sprintf(bufpos, ".%04lx\n", apqi); bufpos += n; nchars += n; } } + return nchars; +} + +static ssize_t matrix_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + ssize_t nchars; + struct mdev_device *mdev = mdev_from_dev(dev); + struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); + + mutex_lock(&matrix_dev->lock); + nchars = vfio_ap_mdev_matrix_show(&matrix_mdev->matrix, buf); mutex_unlock(&matrix_dev->lock); return nchars; } static DEVICE_ATTR_RO(matrix); +static ssize_t guest_matrix_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + ssize_t nchars; + struct mdev_device *mdev = mdev_from_dev(dev); + struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev); + + if (!vfio_ap_mdev_has_crycb(matrix_mdev)) + return -ENODEV; + + mutex_lock(&matrix_dev->lock); + nchars = vfio_ap_mdev_matrix_show(&matrix_mdev->shadow_apcb, buf); + mutex_unlock(&matrix_dev->lock); + + return nchars; +} +static DEVICE_ATTR_RO(guest_matrix); + static struct attribute *vfio_ap_mdev_attrs[] = { &dev_attr_assign_adapter.attr, &dev_attr_unassign_adapter.attr, @@ -1132,6 +1157,7 @@ static struct attribute *vfio_ap_mdev_attrs[] = { &dev_attr_unassign_control_domain.attr, &dev_attr_control_domains.attr, &dev_attr_matrix.attr, + &dev_attr_guest_matrix.attr, NULL, };
The matrix of adapters and domains configured in a guest's APCB may differ from the matrix of adapters and domains assigned to the matrix mdev, so this patch introduces a sysfs attribute to display the matrix of a guest using the matrix mdev. For a matrix mdev denoted by $uuid, the crycb for a guest using the matrix mdev can be displayed as follows: cat /sys/devices/vfio_ap/matrix/$uuid/guest_matrix If a guest is not using the matrix mdev at the time the crycb is displayed, an error (ENODEV) will be returned. Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> --- drivers/s390/crypto/vfio_ap_ops.c | 54 +++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 14 deletions(-)