Messages in this thread | | | Subject | Re: [PATCH v7 3/4] s390: ap: implement PAPQ AQIC interception in kernel | From | Pierre Morel <> | Date | Tue, 30 Apr 2019 10:32:52 +0200 |
| |
On 30/04/2019 10:18, Pierre Morel wrote: > On 29/04/2019 18:50, Halil Pasic wrote: >> On Fri, 26 Apr 2019 15:01:27 +0200 >> Pierre Morel <pmorel@linux.ibm.com> wrote: >> >>> +static struct ap_queue_status vfio_ap_setirq(struct vfio_ap_queue *q) >>> +{ >>> + struct ap_qirq_ctrl aqic_gisa = {}; >>> + struct ap_queue_status status = {}; >>> + struct kvm_s390_gisa *gisa; >>> + struct kvm *kvm; >>> + unsigned long h_nib, h_pfn; >>> + int ret; >>> + >>> + q->a_pfn = q->a_nib >> PAGE_SHIFT; >>> + ret = vfio_pin_pages(mdev_dev(q->matrix_mdev->mdev), &q->a_pfn, 1, >>> + IOMMU_READ | IOMMU_WRITE, &h_pfn); >>> + switch (ret) { >>> + case 1: >>> + break; >>> + case -EINVAL: >>> + case -E2BIG: >>> + status.response_code = AP_RESPONSE_INVALID_ADDRESS; >>> + /* Fallthrough */ >>> + default: >>> + return status; >> >> Can we actually hit the default label? AFICT you would return an >> all-zero status, i.e. status.response_code == 0 'Normal completion'. > > hum right, the setting of AP_INVALID_ADDRESS should be in the default > and there is no need for the two error cases, they will match the default. > > >> >>> + } >>> + >>> + kvm = q->matrix_mdev->kvm; >>> + gisa = kvm->arch.gisa_int.origin; >>> + >>> + h_nib = (h_pfn << PAGE_SHIFT) | (q->a_nib & ~PAGE_MASK); >>> + aqic_gisa.gisc = q->a_isc; >>> + aqic_gisa.isc = kvm_s390_gisc_register(kvm, q->a_isc); >>> + aqic_gisa.ir = 1; >>> + aqic_gisa.gisa = gisa->next_alert >> 4; >> >> Why gisa->next_alert? Isn't this supposed to get set to gisa origin >> (without some bits on the left)? > > Someone already asked this question. > The answer is: look at the ap_qirq_ctrl structure, you will see that the > gisa field is 27 bits wide. > >> >>> + >>> + status = ap_aqic(q->apqn, aqic_gisa, (void *)h_nib); >>> + switch (status.response_code) { >>> + case AP_RESPONSE_NORMAL: >>> + /* See if we did clear older IRQ configuration */ >>> + if (q->p_pfn) >>> + vfio_unpin_pages(mdev_dev(q->matrix_mdev->mdev), >>> + &q->p_pfn, 1); >>> + if (q->p_isc != VFIO_AP_ISC_INVALID) >>> + kvm_s390_gisc_unregister(kvm, q->p_isc); >>> + q->p_pfn = q->a_pfn; >>> + q->p_isc = q->a_isc; >>> + break; >>> + case AP_RESPONSE_OTHERWISE_CHANGED: >>> + /* We could not modify IRQ setings: clear new configuration */ >>> + vfio_unpin_pages(mdev_dev(q->matrix_mdev->mdev), &q->a_pfn, 1); >>> + kvm_s390_gisc_unregister(kvm, q->a_isc); >> >> Hm, see below. Wouldn't you want to set a_isc to VFIO_AP_ISC_INVALID? > > grrr!!! when did I insert these 3 lines, it was OK in previous series! > all 3 lines, vfio_unpin() / gisc_unregister / break must go away.
No it wasn't, I will correct this.
> >> >>> + break; >>> + default: /* Fall Through */ >> >> Is it 'break' or is it 'Fall Through'? > > it is a fall through > >> >>> + pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn, >>> + status.response_code); >>> + vfio_ap_free_irq_data(q); >>> + break; >>> + } >>> + >>> + return status; >>> +} > >
-- Pierre Morel Linux/KVM/QEMU in Böblingen - Germany
| |