| Message ID | 20250220000742.2930832-1-akrowiak@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | s390/vio-ap: Fix no AP queue sharing allowed message written to kernel log | expand |
On 2025-02-20 01:07, Anthony Krowiak wrote: > An erroneous message is written to the kernel log when either of the > following actions are taken by a user: > > 1. Assign an adapter or domain to a vfio_ap mediated device via its > sysfs > assign_adapter or assign_domain attributes that would result in one > or > more AP queues being assigned that are already assigned to a > different > mediated device. Sharing of queues between mdevs is not allowed. > > 2. Reserve an adapter or domain for the host device driver via the AP > bus > driver's sysfs apmask or aqmask attribute that would result in > providing > host access to an AP queue that is in use by a vfio_ap mediated > device. > Reserving a queue for a host driver that is in use by an mdev is not > allowed. > > In both cases, the assignment will return an error; however, a message > like > the following is written to the kernel log: > > vfio_ap_mdev e1839397-51a0-4e3c-91e0-c3b9c3d3047d: Userspace may not > re-assign queue 00.0028 already assigned to \ > e1839397-51a0-4e3c-91e0-c3b9c3d3047d > > Notice the mdev reporting the error is the same as the mdev identified > in the message as the one to which the queue is being assigned. > It is perfectly okay to assign a queue to an mdev to which it is > already assigned; the assignment is simply ignored by the vfio_ap > device > driver. > > This patch logs more descriptive and accurate messages for both 1 and 2 > above to the kernel log: > > Example for 1: > vfio_ap_mdev 0fe903a0-a323-44db-9daf-134c68627d61: Userspace may not > assign > queue 00.0033 to mdev: already assigned to \ > 62177883-f1bb-47f0-914d-32a22e3a8804 > > Example for 2: > vfio_ap_mdev 62177883-f1bb-47f0-914d-32a22e3a8804: Can not reserve > queue > 00.0033 for host driver: in use by mdev > > Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com> > --- > drivers/s390/crypto/vfio_ap_ops.c | 73 ++++++++++++++++++++----------- > 1 file changed, 48 insertions(+), 25 deletions(-) > > diff --git a/drivers/s390/crypto/vfio_ap_ops.c > b/drivers/s390/crypto/vfio_ap_ops.c > index a52c2690933f..2ce52b491f8a 100644 > --- a/drivers/s390/crypto/vfio_ap_ops.c > +++ b/drivers/s390/crypto/vfio_ap_ops.c > @@ -863,48 +863,66 @@ static void vfio_ap_mdev_remove(struct > mdev_device *mdev) > vfio_put_device(&matrix_mdev->vdev); > } > > -#define MDEV_SHARING_ERR "Userspace may not re-assign queue > %02lx.%04lx " \ > - "already assigned to %s" > +#define MDEV_SHARING_ERR "Userspace may not assign queue %02lx.%04lx " > \ > + "to mdev: already assigned to %s" > > -static void vfio_ap_mdev_log_sharing_err(struct ap_matrix_mdev > *matrix_mdev, > - unsigned long *apm, > - unsigned long *aqm) > +#define MDEV_IN_USE_ERR "Can not reserve queue %02lx.%04lx for host > driver: " \ > + "in use by mdev" > + > +static void vfio_ap_mdev_log_sharing_err(struct ap_matrix_mdev > *assignee, > + struct ap_matrix_mdev *assigned_to, > + unsigned long *apm, unsigned long *aqm) > { > unsigned long apid, apqi; > - const struct device *dev = mdev_dev(matrix_mdev->mdev); > - const char *mdev_name = dev_name(dev); > > for_each_set_bit_inv(apid, apm, AP_DEVICES) > for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) > - dev_warn(dev, MDEV_SHARING_ERR, apid, apqi, mdev_name); > + dev_warn(mdev_dev(assignee->mdev), MDEV_SHARING_ERR, > + apid, apqi, dev_name(mdev_dev(assigned_to->mdev))); > } > > -/** > +static void vfio_ap_mdev_log_in_use_err(struct ap_matrix_mdev > *assignee, > + unsigned long *apm, unsigned long *aqm) > +{ > + unsigned long apid, apqi; > + > + for_each_set_bit_inv(apid, apm, AP_DEVICES) > + for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) > + dev_warn(mdev_dev(assignee->mdev), MDEV_IN_USE_ERR, > + apid, apqi); > +} > + > +/**assigned > * vfio_ap_mdev_verify_no_sharing - verify APQNs are not shared by > matrix mdevs > * > + * @assignee the matrix mdev to which @mdev_apm and @mdev_aqm are > being > + * assigned; or, NULL if this function was called by the AP > bus driver > + * in_use callback to verify none of the APQNs being > reserved for the > + * host device driver are in use by a vfio_ap mediated > device > * @mdev_apm: mask indicating the APIDs of the APQNs to be verified > * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified > * > - * Verifies that each APQN derived from the Cartesian product of a > bitmap of > - * AP adapter IDs and AP queue indexes is not configured for any > matrix > - * mediated device. AP queue sharing is not allowed. > + * Verifies that each APQN derived from the Cartesian product of APIDs > + * represented by the bits set in @mdev_apm and the APQIs of the bits > set in > + * @mdev_aqm is not assigned to a mediated device other than the mdev > to which > + * the APQN is being assigned (@assignee). AP queue sharing is not > allowed. > * > * Return: 0 if the APQNs are not shared; otherwise return > -EADDRINUSE. > */ > -static int vfio_ap_mdev_verify_no_sharing(unsigned long *mdev_apm, > +static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev > *assignee, > + unsigned long *mdev_apm, > unsigned long *mdev_aqm) > { > - struct ap_matrix_mdev *matrix_mdev; > + struct ap_matrix_mdev *assigned_to; > DECLARE_BITMAP(apm, AP_DEVICES); > DECLARE_BITMAP(aqm, AP_DOMAINS); > > - list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) { > + list_for_each_entry(assigned_to, &matrix_dev->mdev_list, node) { > /* > - * If the input apm and aqm are fields of the matrix_mdev > - * object, then move on to the next matrix_mdev. > + * If the mdev to which the mdev_apm and mdev_aqm is being > + * assigned is the same as the mdev being verified > */ > - if (mdev_apm == matrix_mdev->matrix.apm && > - mdev_aqm == matrix_mdev->matrix.aqm) > + if (assignee == assigned_to) > continue; > > memset(apm, 0, sizeof(apm)); > @@ -912,17 +930,21 @@ static int > vfio_ap_mdev_verify_no_sharing(unsigned long *mdev_apm, > > /* > * We work on full longs, as we can only exclude the leftover > - * bits in non-inverse order. The leftover is all zeros. > + * bits in non-inverse order. The leftover is all zeros.assigned > */ > - if (!bitmap_and(apm, mdev_apm, matrix_mdev->matrix.apm, > + if (!bitmap_and(apm, mdev_apm, assigned_to->matrix.apm, > AP_DEVICES)) > continue; > > - if (!bitmap_and(aqm, mdev_aqm, matrix_mdev->matrix.aqm, > + if (!bitmap_and(aqm, mdev_aqm, assigned_to->matrix.aqm, > AP_DOMAINS)) > continue; > > - vfio_ap_mdev_log_sharing_err(matrix_mdev, apm, aqm); > + if (assignee) > + vfio_ap_mdev_log_sharing_err(assignee, assigned_to, > + apm, aqm); > + else > + vfio_ap_mdev_log_in_use_err(assigned_to, apm, aqm); > > return -EADDRINUSE; > } > @@ -951,7 +973,8 @@ static int vfio_ap_mdev_validate_masks(struct > ap_matrix_mdev *matrix_mdev) > matrix_mdev->matrix.aqm)) > return -EADDRNOTAVAIL; > > - return vfio_ap_mdev_verify_no_sharing(matrix_mdev->matrix.apm, > + return vfio_ap_mdev_verify_no_sharing(matrix_mdev, > + matrix_mdev->matrix.apm, > matrix_mdev->matrix.aqm); > } > > @@ -2458,7 +2481,7 @@ int vfio_ap_mdev_resource_in_use(unsigned long > *apm, unsigned long *aqm) > > mutex_lock(&matrix_dev->guests_lock); > mutex_lock(&matrix_dev->mdevs_lock); > - ret = vfio_ap_mdev_verify_no_sharing(apm, aqm); > + ret = vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm); > mutex_unlock(&matrix_dev->mdevs_lock); > mutex_unlock(&matrix_dev->guests_lock); I don't see exactly where you do the printk but according to your description you do an error log. I would suggest to lower this to a warning instead.
On Wed, Feb 19, 2025 at 07:07:38PM -0500, Anthony Krowiak wrote: > -#define MDEV_SHARING_ERR "Userspace may not re-assign queue %02lx.%04lx " \ > - "already assigned to %s" > +#define MDEV_SHARING_ERR "Userspace may not assign queue %02lx.%04lx " \ > + "to mdev: already assigned to %s" Please do not split error messages across several lines, so it is easy to grep such for messages. If this would have been used for printk directly checkpatch would have emitted a message. > +#define MDEV_IN_USE_ERR "Can not reserve queue %02lx.%04lx for host driver: " \ > + "in use by mdev" Same here. > for_each_set_bit_inv(apid, apm, AP_DEVICES) > for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) > - dev_warn(dev, MDEV_SHARING_ERR, apid, apqi, mdev_name); > + dev_warn(mdev_dev(assignee->mdev), MDEV_SHARING_ERR, > + apid, apqi, dev_name(mdev_dev(assigned_to->mdev))); Braces are missing. Even it the above is not a bug: bodies of for statements must be enclosed with braces if they have more than one line: for_each_set_bit_inv(apid, apm, AP_DEVICES) { for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) { dev_warn(mdev_dev(assignee->mdev), MDEV_SHARING_ERR, apid, apqi, dev_name(mdev_dev(assigned_to->mdev)) } } > +static void vfio_ap_mdev_log_in_use_err(struct ap_matrix_mdev *assignee, > + unsigned long *apm, unsigned long *aqm) > +{ > + unsigned long apid, apqi; > + > + for_each_set_bit_inv(apid, apm, AP_DEVICES) > + for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) > + dev_warn(mdev_dev(assignee->mdev), MDEV_IN_USE_ERR, > + apid, apqi); > +} Same here. > + > +/**assigned > * vfio_ap_mdev_verify_no_sharing - verify APQNs are not shared by matrix mdevs Stray "assigned" - as a result this is not kernel doc anymore. > + * @assignee the matrix mdev to which @mdev_apm and @mdev_aqm are being > + * assigned; or, NULL if this function was called by the AP bus driver > + * in_use callback to verify none of the APQNs being reserved for the > + * host device driver are in use by a vfio_ap mediated device > * @mdev_apm: mask indicating the APIDs of the APQNs to be verified > * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified Missing ":" behind @assignee. Please keep this consistent. > @@ -912,17 +930,21 @@ static int vfio_ap_mdev_verify_no_sharing(unsigned long *mdev_apm, > > /* > * We work on full longs, as we can only exclude the leftover > - * bits in non-inverse order. The leftover is all zeros. > + * bits in non-inverse order. The leftover is all zeros.assigned > */ Another random "assigned" word. > + if (assignee) > + vfio_ap_mdev_log_sharing_err(assignee, assigned_to, > + apm, aqm); > + else > + vfio_ap_mdev_log_in_use_err(assigned_to, apm, aqm); if body with multiple lines -> braces. Or better make that vfio_ap_mdev_log_sharing_err() call a long line. If you want to keep the line-break add braces to both the if and else branch.
On 2/21/25 2:56 AM, Harald Freudenberger wrote: > On 2025-02-20 01:07, Anthony Krowiak wrote: >> An erroneous message is written to the kernel log when either of the >> following actions are taken by a user: >> >> 1. Assign an adapter or domain to a vfio_ap mediated device via its >> sysfs >> assign_adapter or assign_domain attributes that would result in >> one or >> more AP queues being assigned that are already assigned to a >> different >> mediated device. Sharing of queues between mdevs is not allowed. >> >> 2. Reserve an adapter or domain for the host device driver via the AP >> bus >> driver's sysfs apmask or aqmask attribute that would result in >> providing >> host access to an AP queue that is in use by a vfio_ap mediated >> device. >> Reserving a queue for a host driver that is in use by an mdev is not >> allowed. >> >> In both cases, the assignment will return an error; however, a >> message like >> the following is written to the kernel log: >> vfio_ap_mdev_log_sharing_err >> vfio_ap_mdev e1839397-51a0-4e3c-91e0-c3b9c3d3047d: Userspace may not >> re-assign queue 00.0028 already assigned to \ >> e1839397-51a0-4e3c-91e0-c3b9c3d3047d >> >> Notice the mdev reporting the error is the same as the mdev identified >> in the message as the one to which the queue is being assigned. >> It is perfectly okay to assign a queue to an mdev to which it is >> already assigned; the assignment is simply ignored by the vfio_ap device >> driver. >> >> This patch logs more descriptive and accurate messages for both 1 and 2 >> above to the kernel log: >> >> Example for 1: >> vfio_ap_mdev 0fe903a0-a323-44db-9daf-134c68627d61: Userspace may not >> assign >> queue 00.0033 to mdev: already assigned to \ >> 62177883-f1bb-47f0-914d-32a22e3a8804 >> >> Example for 2: >> vfio_ap_mdev 62177883-f1bb-47f0-914d-32a22e3a8804: Can not reserve queue >> 00.0033 for host driver: in use by mdev >> >> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com> >> --- >> drivers/s390/crypto/vfio_ap_ops.c | 73 ++++++++++++++++++++----------- >> 1 file changed, 48 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/s390/crypto/vfio_ap_ops.c >> b/drivers/s390/crypto/vfio_ap_ops.c >> index a52c2690933f..2ce52b491f8a 100644 >> --- a/drivers/s390/crypto/vfio_ap_ops.c >> +++ b/drivers/s390/crypto/vfio_ap_ops.c >> @@ -863,48 +863,66 @@ static void vfio_ap_mdev_remove(struct >> mdev_device *mdev) >> vfio_put_device(&matrix_mdev->vdev); >> } >> >> -#define MDEV_SHARING_ERR "Userspace may not re-assign queue >> %02lx.%04lx " \ >> - "already assigned to %s" >> +#define MDEV_SHARING_ERR "Userspace may not assign queue %02lx.%04lx >> " \ >> + "to mdev: already assigned to %s" >> >> -static void vfio_ap_mdev_log_sharing_err(struct ap_matrix_mdev >> *matrix_mdev, >> - unsigned long *apm, >> - unsigned long *aqm) >> +#define MDEV_IN_USE_ERR "Can not reserve queue %02lx.%04lx for host >> driver: " \ >> + "in use by mdev" >> + >> +static void vfio_ap_mdev_log_sharing_err(struct ap_matrix_mdev >> *assignee, >> + struct ap_matrix_mdev *assigned_to, >> + unsigned long *apm, unsigned long *aqm) >> { >> unsigned long apid, apqi; >> - const struct device *dev = mdev_dev(matrix_mdev->mdev); >> - const char *mdev_name = dev_name(dev); >> >> for_each_set_bit_inv(apid, apm, AP_DEVICES) >> for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) >> - dev_warn(dev, MDEV_SHARING_ERR, apid, apqi, mdev_name); >> + dev_warn(mdev_dev(assignee->mdev), MDEV_SHARING_ERR, >> + apid, apqi, dev_name(mdev_dev(assigned_to->mdev))); >> } >> >> -/** >> +static void vfio_ap_mdev_log_in_use_err(struct ap_matrix_mdev >> *assignee, >> + unsigned long *apm, unsigned long *aqm) >> +{ >> + unsigned long apid, apqi; >> + >> + for_each_set_bit_inv(apid, apm, AP_DEVICES) >> + for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) >> + dev_warn(mdev_dev(assignee->mdev), MDEV_IN_USE_ERR, >> + apid, apqi); >> +} >> + >> +/**assigned >> * vfio_ap_mdev_verify_no_sharing - verify APQNs are not shared by >> matrix mdevs >> * >> + * @assignee the matrix mdev to which @mdev_apm and @mdev_aqm are being >> + * assigned; or, NULL if this function was called by the AP >> bus driver >> + * in_use callback to verify none of the APQNs being >> reserved for the >> + * host device driver are in use by a vfio_ap mediated device >> * @mdev_apm: mask indicating the APIDs of the APQNs to be verified >> * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified >> * >> - * Verifies that each APQN derived from the Cartesian product of a >> bitmap of >> - * AP adapter IDs and AP queue indexes is not configured for any matrix >> - * mediated device. AP queue sharing is not allowed. >> + * Verifies that each APQN derived from the Cartesian product of APIDs >> + * represented by the bits set in @mdev_apm and the APQIs of the >> bits set in >> + * @mdev_aqm is not assigned to a mediated device other than the >> mdev to which >> + * the APQN is being assigned (@assignee). AP queue sharing is not >> allowed. >> * >> * Return: 0 if the APQNs are not shared; otherwise return -EADDRINUSE. >> */ >> -static int vfio_ap_mdev_verify_no_sharing(unsigned long *mdev_apm, >> +static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev >> *assignee, >> + unsigned long *mdev_apm, >> unsigned long *mdev_aqm) >> { >> - struct ap_matrix_mdev *matrix_mdev; >> + struct ap_matrix_mdev *assigned_to; >> DECLARE_BITMAP(apm, AP_DEVICES); >> DECLARE_BITMAP(aqm, AP_DOMAINS); >> >> - list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) { >> + list_for_each_entry(assigned_to, &matrix_dev->mdev_list, node) { >> /* >> - * If the input apm and aqm are fields of the matrix_mdev >> - * object, then move on to the next matrix_mdev. >> + * If the mdev to which the mdev_apm and mdev_aqm is being >> + * assigned is the same as the mdev being verified >> */ >> - if (mdev_apm == matrix_mdev->matrix.apm && >> - mdev_aqm == matrix_mdev->matrix.aqm) >> + if (assignee == assigned_to) >> continue; >> >> memset(apm, 0, sizeof(apm)); >> @@ -912,17 +930,21 @@ static int >> vfio_ap_mdev_verify_no_sharing(unsigned long *mdev_apm, >> >> /* >> * We work on full longs, as we can only exclude the leftover >> - * bits in non-inverse order. The leftover is all zeros. >> + * bits in non-inverse order. The leftover is all >> zeros.assigned >> */ >> - if (!bitmap_and(apm, mdev_apm, matrix_mdev->matrix.apm, >> + if (!bitmap_and(apm, mdev_apm, assigned_to->matrix.apm, >> AP_DEVICES)) >> continue; >> >> - if (!bitmap_and(aqm, mdev_aqm, matrix_mdev->matrix.aqm, >> + if (!bitmap_and(aqm, mdev_aqm, assigned_to->matrix.aqm, >> AP_DOMAINS)) >> continue; >> >> - vfio_ap_mdev_log_sharing_err(matrix_mdev, apm, aqm); >> + if (assignee) >> + vfio_ap_mdev_log_sharing_err(assignee, assigned_to, >> + apm, aqm); >> + else >> + vfio_ap_mdev_log_in_use_err(assigned_to, apm, aqm); >> >> return -EADDRINUSE; >> } >> @@ -951,7 +973,8 @@ static int vfio_ap_mdev_validate_masks(struct >> ap_matrix_mdev *matrix_mdev) >> matrix_mdev->matrix.aqm)) >> return -EADDRNOTAVAIL; >> >> - return vfio_ap_mdev_verify_no_sharing(matrix_mdev->matrix.apm, >> + return vfio_ap_mdev_verify_no_sharing(matrix_mdev, >> + matrix_mdev->matrix.apm, >> matrix_mdev->matrix.aqm); >> } >> >> @@ -2458,7 +2481,7 @@ int vfio_ap_mdev_resource_in_use(unsigned long >> *apm, unsigned long *aqm) >> >> mutex_lock(&matrix_dev->guests_lock); >> mutex_lock(&matrix_dev->mdevs_lock); >> - ret = vfio_ap_mdev_verify_no_sharing(apm, aqm); >> + ret = vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm); >> mutex_unlock(&matrix_dev->mdevs_lock); >> mutex_unlock(&matrix_dev->guests_lock); > > I don't see exactly where you do the printk but according to your > description you do an error log. I would suggest to lower this > to a warning instead. The messages are written via the two functions above using the 'dev_warn' function to post the messages to the log. See: vfio_ap_mdev_log_sharing_err vfio_ap_mdev_log_in_use_err
On 2/21/25 4:57 AM, Heiko Carstens wrote: > On Wed, Feb 19, 2025 at 07:07:38PM -0500, Anthony Krowiak wrote: >> -#define MDEV_SHARING_ERR "Userspace may not re-assign queue %02lx.%04lx " \ >> - "already assigned to %s" >> +#define MDEV_SHARING_ERR "Userspace may not assign queue %02lx.%04lx " \ >> + "to mdev: already assigned to %s" > Please do not split error messages across several lines, so it is easy > to grep such for messages. If this would have been used for printk > directly checkpatch would have emitted a message. fixed > >> +#define MDEV_IN_USE_ERR "Can not reserve queue %02lx.%04lx for host driver: " \ >> + "in use by mdev" > Same here. fixed > >> for_each_set_bit_inv(apid, apm, AP_DEVICES) >> for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) >> - dev_warn(dev, MDEV_SHARING_ERR, apid, apqi, mdev_name); >> + dev_warn(mdev_dev(assignee->mdev), MDEV_SHARING_ERR, >> + apid, apqi, dev_name(mdev_dev(assigned_to->mdev))); > Braces are missing. Even it the above is not a bug: bodies of for > statements must be enclosed with braces if they have more than one > line: fixed > > for_each_set_bit_inv(apid, apm, AP_DEVICES) { > for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) { > dev_warn(mdev_dev(assignee->mdev), MDEV_SHARING_ERR, > apid, apqi, dev_name(mdev_dev(assigned_to->mdev)) > } > } > >> +static void vfio_ap_mdev_log_in_use_err(struct ap_matrix_mdev *assignee, >> + unsigned long *apm, unsigned long *aqm) >> +{ >> + unsigned long apid, apqi; >> + >> + for_each_set_bit_inv(apid, apm, AP_DEVICES) >> + for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) >> + dev_warn(mdev_dev(assignee->mdev), MDEV_IN_USE_ERR, >> + apid, apqi); >> +} > Same here. fixed > >> + >> +/**assigned >> * vfio_ap_mdev_verify_no_sharing - verify APQNs are not shared by matrix mdevs > Stray "assigned" - as a result this is not kernel doc anymore. fixed > >> + * @assignee the matrix mdev to which @mdev_apm and @mdev_aqm are being >> + * assigned; or, NULL if this function was called by the AP bus driver >> + * in_use callback to verify none of the APQNs being reserved for the >> + * host device driver are in use by a vfio_ap mediated device >> * @mdev_apm: mask indicating the APIDs of the APQNs to be verified >> * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified > Missing ":" behind @assignee. Please keep this consistent. fixed > >> @@ -912,17 +930,21 @@ static int vfio_ap_mdev_verify_no_sharing(unsigned long *mdev_apm, >> >> /* >> * We work on full longs, as we can only exclude the leftover >> - * bits in non-inverse order. The leftover is all zeros. >> + * bits in non-inverse order. The leftover is all zeros.assigned >> */ > Another random "assigned" word. My IDE sometimes randomly pastes things in the clipboard. fixed > >> + if (assignee) >> + vfio_ap_mdev_log_sharing_err(assignee, assigned_to, >> + apm, aqm); >> + else >> + vfio_ap_mdev_log_in_use_err(assigned_to, apm, aqm); > if body with multiple lines -> braces. Or better make that > vfio_ap_mdev_log_sharing_err() call a long line. If you want to keep > the line-break add braces to both the if and else branch. fixed
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index a52c2690933f..2ce52b491f8a 100644 --- a/drivers/s390/crypto/vfio_ap_ops.c +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -863,48 +863,66 @@ static void vfio_ap_mdev_remove(struct mdev_device *mdev) vfio_put_device(&matrix_mdev->vdev); } -#define MDEV_SHARING_ERR "Userspace may not re-assign queue %02lx.%04lx " \ - "already assigned to %s" +#define MDEV_SHARING_ERR "Userspace may not assign queue %02lx.%04lx " \ + "to mdev: already assigned to %s" -static void vfio_ap_mdev_log_sharing_err(struct ap_matrix_mdev *matrix_mdev, - unsigned long *apm, - unsigned long *aqm) +#define MDEV_IN_USE_ERR "Can not reserve queue %02lx.%04lx for host driver: " \ + "in use by mdev" + +static void vfio_ap_mdev_log_sharing_err(struct ap_matrix_mdev *assignee, + struct ap_matrix_mdev *assigned_to, + unsigned long *apm, unsigned long *aqm) { unsigned long apid, apqi; - const struct device *dev = mdev_dev(matrix_mdev->mdev); - const char *mdev_name = dev_name(dev); for_each_set_bit_inv(apid, apm, AP_DEVICES) for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) - dev_warn(dev, MDEV_SHARING_ERR, apid, apqi, mdev_name); + dev_warn(mdev_dev(assignee->mdev), MDEV_SHARING_ERR, + apid, apqi, dev_name(mdev_dev(assigned_to->mdev))); } -/** +static void vfio_ap_mdev_log_in_use_err(struct ap_matrix_mdev *assignee, + unsigned long *apm, unsigned long *aqm) +{ + unsigned long apid, apqi; + + for_each_set_bit_inv(apid, apm, AP_DEVICES) + for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) + dev_warn(mdev_dev(assignee->mdev), MDEV_IN_USE_ERR, + apid, apqi); +} + +/**assigned * vfio_ap_mdev_verify_no_sharing - verify APQNs are not shared by matrix mdevs * + * @assignee the matrix mdev to which @mdev_apm and @mdev_aqm are being + * assigned; or, NULL if this function was called by the AP bus driver + * in_use callback to verify none of the APQNs being reserved for the + * host device driver are in use by a vfio_ap mediated device * @mdev_apm: mask indicating the APIDs of the APQNs to be verified * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified * - * Verifies that each APQN derived from the Cartesian product of a bitmap of - * AP adapter IDs and AP queue indexes is not configured for any matrix - * mediated device. AP queue sharing is not allowed. + * Verifies that each APQN derived from the Cartesian product of APIDs + * represented by the bits set in @mdev_apm and the APQIs of the bits set in + * @mdev_aqm is not assigned to a mediated device other than the mdev to which + * the APQN is being assigned (@assignee). AP queue sharing is not allowed. * * Return: 0 if the APQNs are not shared; otherwise return -EADDRINUSE. */ -static int vfio_ap_mdev_verify_no_sharing(unsigned long *mdev_apm, +static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *assignee, + unsigned long *mdev_apm, unsigned long *mdev_aqm) { - struct ap_matrix_mdev *matrix_mdev; + struct ap_matrix_mdev *assigned_to; DECLARE_BITMAP(apm, AP_DEVICES); DECLARE_BITMAP(aqm, AP_DOMAINS); - list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) { + list_for_each_entry(assigned_to, &matrix_dev->mdev_list, node) { /* - * If the input apm and aqm are fields of the matrix_mdev - * object, then move on to the next matrix_mdev. + * If the mdev to which the mdev_apm and mdev_aqm is being + * assigned is the same as the mdev being verified */ - if (mdev_apm == matrix_mdev->matrix.apm && - mdev_aqm == matrix_mdev->matrix.aqm) + if (assignee == assigned_to) continue; memset(apm, 0, sizeof(apm)); @@ -912,17 +930,21 @@ static int vfio_ap_mdev_verify_no_sharing(unsigned long *mdev_apm, /* * We work on full longs, as we can only exclude the leftover - * bits in non-inverse order. The leftover is all zeros. + * bits in non-inverse order. The leftover is all zeros.assigned */ - if (!bitmap_and(apm, mdev_apm, matrix_mdev->matrix.apm, + if (!bitmap_and(apm, mdev_apm, assigned_to->matrix.apm, AP_DEVICES)) continue; - if (!bitmap_and(aqm, mdev_aqm, matrix_mdev->matrix.aqm, + if (!bitmap_and(aqm, mdev_aqm, assigned_to->matrix.aqm, AP_DOMAINS)) continue; - vfio_ap_mdev_log_sharing_err(matrix_mdev, apm, aqm); + if (assignee) + vfio_ap_mdev_log_sharing_err(assignee, assigned_to, + apm, aqm); + else + vfio_ap_mdev_log_in_use_err(assigned_to, apm, aqm); return -EADDRINUSE; } @@ -951,7 +973,8 @@ static int vfio_ap_mdev_validate_masks(struct ap_matrix_mdev *matrix_mdev) matrix_mdev->matrix.aqm)) return -EADDRNOTAVAIL; - return vfio_ap_mdev_verify_no_sharing(matrix_mdev->matrix.apm, + return vfio_ap_mdev_verify_no_sharing(matrix_mdev, + matrix_mdev->matrix.apm, matrix_mdev->matrix.aqm); } @@ -2458,7 +2481,7 @@ int vfio_ap_mdev_resource_in_use(unsigned long *apm, unsigned long *aqm) mutex_lock(&matrix_dev->guests_lock); mutex_lock(&matrix_dev->mdevs_lock); - ret = vfio_ap_mdev_verify_no_sharing(apm, aqm); + ret = vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm); mutex_unlock(&matrix_dev->mdevs_lock); mutex_unlock(&matrix_dev->guests_lock);
An erroneous message is written to the kernel log when either of the following actions are taken by a user: 1. Assign an adapter or domain to a vfio_ap mediated device via its sysfs assign_adapter or assign_domain attributes that would result in one or more AP queues being assigned that are already assigned to a different mediated device. Sharing of queues between mdevs is not allowed. 2. Reserve an adapter or domain for the host device driver via the AP bus driver's sysfs apmask or aqmask attribute that would result in providing host access to an AP queue that is in use by a vfio_ap mediated device. Reserving a queue for a host driver that is in use by an mdev is not allowed. In both cases, the assignment will return an error; however, a message like the following is written to the kernel log: vfio_ap_mdev e1839397-51a0-4e3c-91e0-c3b9c3d3047d: Userspace may not re-assign queue 00.0028 already assigned to \ e1839397-51a0-4e3c-91e0-c3b9c3d3047d Notice the mdev reporting the error is the same as the mdev identified in the message as the one to which the queue is being assigned. It is perfectly okay to assign a queue to an mdev to which it is already assigned; the assignment is simply ignored by the vfio_ap device driver. This patch logs more descriptive and accurate messages for both 1 and 2 above to the kernel log: Example for 1: vfio_ap_mdev 0fe903a0-a323-44db-9daf-134c68627d61: Userspace may not assign queue 00.0033 to mdev: already assigned to \ 62177883-f1bb-47f0-914d-32a22e3a8804 Example for 2: vfio_ap_mdev 62177883-f1bb-47f0-914d-32a22e3a8804: Can not reserve queue 00.0033 for host driver: in use by mdev Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com> --- drivers/s390/crypto/vfio_ap_ops.c | 73 ++++++++++++++++++++----------- 1 file changed, 48 insertions(+), 25 deletions(-)