On Fri, 21 Aug 2020 15:56:14 -0400
Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
> Implements the driver callback invoked by the AP bus when the AP bus
> scan has completed. Since this callback is invoked after binding the newly
> added devices to their respective device drivers, the vfio_ap driver will
> attempt to plug the adapters, domains and control domains into each guest
> using a matrix mdev to which they are assigned. Keep in mind that an
> adapter or domain can be plugged in only if each APQN with the APID of the
> adapter or the APQI of the domain references a queue device bound to the
> vfio_ap device driver. Consequently, not all newly added adapters and
> domains will necessarily get hot plugged.
>
> Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxx>
> ---
> drivers/s390/crypto/vfio_ap_drv.c | 1 +
> drivers/s390/crypto/vfio_ap_ops.c | 110 +++++++++++++++++++++++++-
> drivers/s390/crypto/vfio_ap_private.h | 2 +
> 3 files changed, 110 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index ea0a7603e886..21bfae928be5 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -180,6 +180,7 @@ static int __init vfio_ap_init(void)
> vfio_ap_drv.in_use = vfio_ap_mdev_resource_in_use;
> vfio_ap_drv.ids = ap_queue_ids;
> vfio_ap_drv.on_config_changed = vfio_ap_on_cfg_changed;
> + vfio_ap_drv.on_scan_complete = vfio_ap_on_scan_complete;
>
> ret = ap_driver_register(&vfio_ap_drv, THIS_MODULE, VFIO_AP_DRV_NAME);
> if (ret) {
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index e002d556abab..e6480f31a42b 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -616,14 +616,13 @@ static bool vfio_ap_mdev_config_shadow_apcb(struct ap_matrix_mdev *matrix_mdev)
> * CRYCB after filtering, then try filtering the APQIs.
> */
> if (napm == 0) {
> - naqm = vfio_ap_mdev_filter_matrix(matrix_mdev,
> - &shadow_apcb, false);
> -
> /*
> * If there are no APQNs that can be assigned to the
> * matrix mdev after filtering the APQIs, then no APQNs
> * shall be assigned to the guest's CRYCB.
> */
> + naqm = vfio_ap_mdev_filter_matrix(matrix_mdev,
> + &shadow_apcb, false);
Here you just moved the thing around the comment, or?
> if (naqm == 0) {
> bitmap_clear(shadow_apcb.apm, 0, AP_DEVICES);
> bitmap_clear(shadow_apcb.aqm, 0, AP_DOMAINS);
> @@ -1758,6 +1757,16 @@ static bool vfio_ap_mdev_unassign_apids(struct ap_matrix_mdev *matrix_mdev,
> for_each_set_bit_inv(apid, apm_unassign, AP_DEVICES) {
> unassigned |= vfio_ap_mdev_unassign_guest_apid(matrix_mdev,
> apid);
> + /*
> + * If the APID is not assigned to the matrix mdev's shadow
> + * CRYCB, continue with the next APID.
> + */
> + if (!test_bit_inv(apid, matrix_mdev->shadow_apcb.apm))
> + continue;
> +
> + /* Unassign the APID from the matrix mdev's shadow CRYCB */
> + clear_bit_inv(apid, matrix_mdev->shadow_apcb.apm);
> + unassigned = true;
I don't understand this at all. This patch is supposed to be about
assign and not unassign, or?
> }
>
> return unassigned;
> @@ -1791,6 +1800,17 @@ static bool vfio_ap_mdev_unassign_apqis(struct ap_matrix_mdev *matrix_mdev,
> for_each_set_bit_inv(apqi, aqm_unassign, AP_DOMAINS) {
> unassigned |= vfio_ap_mdev_unassign_guest_apqi(matrix_mdev,
> apqi);
> +
> + /*
> + * If the APQI is not assigned to the matrix mdev's shadow
> + * CRYCB, continue with the next APQI
> + */
> + if (!test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm))
> + continue;
> +
> + /* Unassign the APQI from the matrix mdev's shadow CRYCB */
> + clear_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm);
> + unassigned = true;
> }
>
> return unassigned;
> @@ -1852,3 +1872,87 @@ void vfio_ap_on_cfg_changed(struct ap_config_info *new_config_info,
> }
> mutex_unlock(&matrix_dev->lock);
> }
> +
> +bool vfio_ap_mdev_assign_apids(struct ap_matrix_mdev *matrix_mdev,
> + unsigned long *apm_assign)
> +{
> + unsigned long apid;
> + bool assigned = false;
> +
> + for_each_set_bit_inv(apid, apm_assign, AP_DEVICES)
> + if (test_bit_inv(apid, matrix_mdev->matrix.apm))
> + if (vfio_ap_mdev_assign_guest_apid(matrix_mdev, apid))
> + assigned = true;
> +
> + return assigned;
> +}
> +
> +bool vfio_ap_mdev_assign_apqis(struct ap_matrix_mdev *matrix_mdev,
> + unsigned long *aqm_assign)
> +{
> + unsigned long apqi;
> + bool assigned = false;
> +
> + for_each_set_bit_inv(apqi, aqm_assign, AP_DOMAINS)
> + if (test_bit_inv(apqi, matrix_mdev->matrix.aqm))
> + if (vfio_ap_mdev_assign_guest_apqi(matrix_mdev, apqi))
> + assigned = true;
> +
> + return assigned;
> +}
> +
> +void vfio_ap_on_scan_complete(struct ap_config_info *new_config_info,
> + struct ap_config_info *old_config_info)
> +{
> + struct ap_matrix_mdev *matrix_mdev;
> + DECLARE_BITMAP(apm_assign, AP_DEVICES);
> + DECLARE_BITMAP(aqm_assign, AP_DOMAINS);
> + int ap_add, aq_add;
> + bool assign;
> + unsigned long *cur_apm, *cur_aqm, *prev_apm, *prev_aqm;
> +
> + /*
> + * If we are not in the middle of a host configuration change scan it is
> + * likely that the vfio_ap driver was loaded mid-scan, so let's handle
> + * this scenario by calling the vfio_ap_on_cfg_changed function which
> + * gets called at the start of an AP bus scan when the host AP
> + * configuration has changed.
> + */
> + if (!(matrix_dev->flags & AP_MATRIX_CFG_CHG))
> + vfio_ap_on_cfg_changed(new_config_info, old_config_info);
Or we could just let the not-optimized variant handle it. Patch 15 has
to take care of single queues anyway, and 13 and 14 are about avoiding
having a bunch of updates to the CRYCB in short succession. But if we
just loaded the module in a middle of a config changing scan, then I
guess having a bunch of populated mdevs attached to guests is not very
likely.
> +
> + cur_apm = (unsigned long *)matrix_dev->config_info.apm;
> + cur_aqm = (unsigned long *)matrix_dev->config_info.aqm;
> +
> + prev_apm = (unsigned long *)matrix_dev->config_info_prev.apm;
> + prev_aqm = (unsigned long *)matrix_dev->config_info_prev.aqm;
> +
> + ap_add = bitmap_andnot(apm_assign, cur_apm, prev_apm, AP_DEVICES);
> + aq_add = bitmap_andnot(aqm_assign, cur_aqm, prev_aqm, AP_DOMAINS);
> +
> + mutex_lock(&matrix_dev->lock);
> + list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
> + if (!vfio_ap_mdev_has_crycb(matrix_mdev))
> + continue;
> +
> + assign = false;
> +
> + if (ap_add)
> + if (bitmap_intersects(matrix_mdev->matrix.apm,
> + apm_assign, AP_DEVICES))
> + assign |= vfio_ap_mdev_assign_apids(matrix_mdev,
> + apm_assign);
> +
> + if (aq_add)
> + if (bitmap_intersects(matrix_mdev->matrix.aqm,
> + aqm_assign, AP_DOMAINS))
> + assign |= vfio_ap_mdev_assign_apqis(matrix_mdev,
> + aqm_assign);
> +
> + if (assign)
> + vfio_ap_mdev_commit_shadow_apcb(matrix_mdev);
> + }
> +
> + matrix_dev->flags &= ~AP_MATRIX_CFG_CHG;
> + mutex_unlock(&matrix_dev->lock);
> +}
There may be a simpler and more concise way to accomplish this logic,
but at this point I don't want to think about that. We can do
refactoring any time we want. I'm more worried about the points I
addressed in reference to the previous patches.
Regards,
Halil
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index fc8629e28ad3..da1754fd4f66 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -113,5 +113,7 @@ void vfio_ap_mdev_remove_queue(struct ap_queue *queue);
> bool vfio_ap_mdev_resource_in_use(unsigned long *apm, unsigned long *aqm);
> void vfio_ap_on_cfg_changed(struct ap_config_info *new_config_info,
> struct ap_config_info *old_config_info);
> +void vfio_ap_on_scan_complete(struct ap_config_info *new_config_info,
> + struct ap_config_info *old_config_info);
>
> #endif /* _VFIO_AP_PRIVATE_H_ */
