Re: [PATCH v7 3/4] s390: ap: implement PAPQ AQIC interception in kernel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxx> 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




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux