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

[Pierre Morel]

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