Messages in this thread | | | Date | Tue, 11 Jan 2022 16:19:06 -0500 | Subject | Re: [PATCH v17 06/15] s390/vfio-ap: refresh guest's APCB by filtering APQNs assigned to mdev | From | Tony Krowiak <> |
| |
On 12/27/21 03:53, Halil Pasic wrote: > On Thu, 21 Oct 2021 11:23:23 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> Refresh the guest's APCB by filtering the APQNs assigned to the matrix mdev >> that do not reference an AP queue device bound to the vfio_ap device >> driver. The mdev's APQNs will be filtered according to the following rules: >> >> * The APID of each adapter and the APQI of each domain that is not in the >> host's AP configuration is filtered out. >> >> * The APID of each adapter comprising an APQN that does not reference a >> queue device bound to the vfio_ap device driver is filtered. The APQNs >> are derived from the Cartesian product of the APID of each adapter and >> APQI of each domain assigned to the mdev. >> >> The control domains that are not assigned to the host's AP configuration >> will also be filtered before assigning them to the guest's APCB. > The v16 version used to filer on queue removal from vfio_ap, which makes > a ton of sense. > > This version will "filter back" the queues once these become bound, but > if a queue is removed form vfio_ap, we don't seem to care to filter. Is > this intentional?
See patch the changes to the vfio_ap_mdev_remove_queue function in patch 09/15, s390/vfio-ap: allow hot plug/unplug of AP resources using mdev device
Now that I look at that patch, I should probably rearrange this patch to also do filtering on queue removal, but only do the hotplug stuff in patch 09.
> > Also we could probably do the filtering incrementally. In a sense that > at a time only so much changes, and we know that the invariant was > preserved without that change. But that would probably end up trading > complexity for cycles. I will trust your judgment and your tests on this > matter.
I am not entirely clear on what you are suggesting. I think you are suggesting that there may not be a need to look at every APQN assigned to the mdev when an adapter or domain is assigned or unassigned or a queue is probed or removed. Maybe you can clarify what you are suggesting here.
> >> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >> --- >> drivers/s390/crypto/vfio_ap_ops.c | 66 ++++++++++++++++++++++++++++++- >> 1 file changed, 64 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c >> index 4305177029bf..46c179363aca 100644 >> --- a/drivers/s390/crypto/vfio_ap_ops.c >> +++ b/drivers/s390/crypto/vfio_ap_ops.c >> @@ -314,6 +314,62 @@ static void vfio_ap_matrix_init(struct ap_config_info *info, >> matrix->adm_max = info->apxa ? info->Nd : 15; >> } >> >> +static void vfio_ap_mdev_filter_cdoms(struct ap_matrix_mdev *matrix_mdev) >> +{ >> + bitmap_and(matrix_mdev->shadow_apcb.adm, matrix_mdev->matrix.adm, >> + (unsigned long *)matrix_dev->info.adm, AP_DOMAINS); >> +} >> + >> +/* >> + * vfio_ap_mdev_filter_matrix - copy the mdev's AP configuration to the KVM >> + * guest's APCB then filter the APIDs that do not >> + * comprise at least one APQN that references a >> + * queue device bound to the vfio_ap device driver. >> + * >> + * @matrix_mdev: the mdev whose AP configuration is to be filtered. >> + */ >> +static void vfio_ap_mdev_filter_matrix(struct ap_matrix_mdev *matrix_mdev) >> +{ >> + int ret; >> + unsigned long apid, apqi, apqn; >> + >> + ret = ap_qci(&matrix_dev->info); >> + if (ret) >> + return; >> + >> + vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->shadow_apcb); >> + >> + /* >> + * Copy the adapters, domains and control domains to the shadow_apcb >> + * from the matrix mdev, but only those that are assigned to the host's >> + * AP configuration. >> + */ >> + bitmap_and(matrix_mdev->shadow_apcb.apm, matrix_mdev->matrix.apm, >> + (unsigned long *)matrix_dev->info.apm, AP_DEVICES); >> + bitmap_and(matrix_mdev->shadow_apcb.aqm, matrix_mdev->matrix.aqm, >> + (unsigned long *)matrix_dev->info.aqm, AP_DOMAINS); >> + >> + for_each_set_bit_inv(apid, matrix_mdev->shadow_apcb.apm, AP_DEVICES) { >> + for_each_set_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm, >> + AP_DOMAINS) { >> + /* >> + * If the APQN is not bound to the vfio_ap device >> + * driver, then we can't assign it to the guest's >> + * AP configuration. The AP architecture won't >> + * allow filtering of a single APQN, so if we're >> + * filtering APIDs, then filter the APID; otherwise, >> + * filter the APQI. >> + */ >> + apqn = AP_MKQID(apid, apqi); >> + if (!vfio_ap_mdev_get_queue(matrix_mdev, apqn)) { >> + clear_bit_inv(apid, >> + matrix_mdev->shadow_apcb.apm); >> + break; >> + } >> + } >> + } >> +} >> + >> static int vfio_ap_mdev_probe(struct mdev_device *mdev) >> { >> struct ap_matrix_mdev *matrix_mdev; >> @@ -703,6 +759,7 @@ static ssize_t assign_adapter_store(struct device *dev, >> goto share_err; >> >> vfio_ap_mdev_link_adapter(matrix_mdev, apid); >> + vfio_ap_mdev_filter_matrix(matrix_mdev); >> ret = count; >> goto done; >> >> @@ -771,6 +828,7 @@ static ssize_t unassign_adapter_store(struct device *dev, >> >> clear_bit_inv((unsigned long)apid, matrix_mdev->matrix.apm); >> vfio_ap_mdev_unlink_adapter(matrix_mdev, apid); >> + vfio_ap_mdev_filter_matrix(matrix_mdev); >> ret = count; >> done: >> mutex_unlock(&matrix_dev->lock); >> @@ -874,6 +932,7 @@ static ssize_t assign_domain_store(struct device *dev, >> goto share_err; >> >> vfio_ap_mdev_link_domain(matrix_mdev, apqi); >> + vfio_ap_mdev_filter_matrix(matrix_mdev); >> ret = count; >> goto done; >> >> @@ -942,6 +1001,7 @@ static ssize_t unassign_domain_store(struct device *dev, >> >> clear_bit_inv((unsigned long)apqi, matrix_mdev->matrix.aqm); >> vfio_ap_mdev_unlink_domain(matrix_mdev, apqi); >> + vfio_ap_mdev_filter_matrix(matrix_mdev); >> ret = count; >> >> done: >> @@ -995,6 +1055,7 @@ static ssize_t assign_control_domain_store(struct device *dev, >> * number of control domains that can be assigned. >> */ >> set_bit_inv(id, matrix_mdev->matrix.adm); >> + vfio_ap_mdev_filter_cdoms(matrix_mdev); >> ret = count; >> done: >> mutex_unlock(&matrix_dev->lock); >> @@ -1042,6 +1103,7 @@ static ssize_t unassign_control_domain_store(struct device *dev, >> } >> >> clear_bit_inv(domid, matrix_mdev->matrix.adm); >> + clear_bit_inv(domid, matrix_mdev->shadow_apcb.adm); >> ret = count; >> done: >> mutex_unlock(&matrix_dev->lock); >> @@ -1179,8 +1241,6 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev, >> kvm_get_kvm(kvm); >> matrix_mdev->kvm = kvm; >> kvm->arch.crypto.data = matrix_mdev; >> - memcpy(&matrix_mdev->shadow_apcb, &matrix_mdev->matrix, >> - sizeof(struct ap_matrix)); >> kvm_arch_crypto_set_masks(kvm, matrix_mdev->shadow_apcb.apm, >> matrix_mdev->shadow_apcb.aqm, >> matrix_mdev->shadow_apcb.adm); >> @@ -1536,6 +1596,8 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev) >> q->apqn = to_ap_queue(&apdev->device)->qid; >> q->saved_isc = VFIO_AP_ISC_INVALID; >> vfio_ap_queue_link_mdev(q); >> + if (q->matrix_mdev) >> + vfio_ap_mdev_filter_matrix(q->matrix_mdev); >> dev_set_drvdata(&apdev->device, q); >> mutex_unlock(&matrix_dev->lock); >>
| |