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

From: Pierre Morel
Date: Fri May 10 2019 - 12:23:21 EST

On 08/05/2019 22:17, Tony Krowiak wrote:
On 5/2/19 1:34 PM, Pierre Morel wrote:
We register a AP PQAP instruction hook during the open
of the mediated device. And unregister it on release.

During the probe of the AP device, we allocate a vfio_ap_queue
structure to keep track of the information we need for the
PQAP/AQIC instruction interception.

In the AP PQAP instruction hook, if we receive a demand to
enable IRQs,
- we retrieve the vfio_ap_queue based on the APQN we receive
ÂÂ in REG1,
- we retrieve the page of the guest address, (NIB), from
ÂÂ register REG2
- we retrieve the mediated device to use the VFIO pinning
ÂÂ infrastructure to pin the page of the guest address,
- we retrieve the pointer to KVM to register the guest ISC
ÂÂ and retrieve the host ISC
- finaly we activate GISA

If we receive a demand to disable IRQs,
- we deactivate GISA
- unregister from the GIB
- unping the NIB

s/unping/unpin

yes, thanks,



...snip...

 static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
 {
-ÂÂÂ /* Nothing to do yet */
+ÂÂÂ struct vfio_ap_queue *q;
+ÂÂÂ int apid, apqi;
+
+ÂÂÂ mutex_lock(&matrix_dev->lock);
+ÂÂÂ q = dev_get_drvdata(&apdev->device);
+ÂÂÂ dev_set_drvdata(&apdev->device, NULL);
+ÂÂÂ if (q) {
+ÂÂÂÂÂÂÂ apid = AP_QID_CARD(q->apqn);
+ÂÂÂÂÂÂÂ apqi = AP_QID_QUEUE(q->apqn);
+ÂÂÂÂÂÂÂ vfio_ap_mdev_reset_queue(apid, apqi, 1);

As you know, there is another patch series (s390: vfio-ap: dynamic
configuration support) posted concurrently with this series. That series
handles reset on remove of an AP queue device. It looks like your
choices here will greatly conflict with the reset processing in the
other patch series and create a nasty merge conflict. My suggestion is
that you build this patch series on top of the other series and do
the following:

There are two new functions introduced in vfio_ap_private.h:
void vfio_ap_mdev_remove_queue(struct ap_queue *queue);
void vfio_ap_mdev_probe_queue(struct ap_queue *queue);

These are called from the probe and remove callbacks in vfio_ap_drv.c.
If you embed your changes to the probe and remove functions above into
those new functions, that will make merging the two much easier and
the code cleaner IMHO.

The merging is really limited
The dynamic configuration patches series is new and I am not sure it will satisfy Harald and Reinhard, doing so would delay the IRQ patch series for an unknown among of time.
I am not sure it is so wise.

May be an other opinion?


Whatever, we can share the reset function, it is independent of the series, for my opinion it could go its own way.
I can take your patch.

...snip...

+struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
+{
+ÂÂÂ struct ap_qirq_ctrl aqic_gisa;
+ÂÂÂ struct ap_queue_status status = {
+ÂÂÂÂÂÂÂÂÂÂÂ .response_code = AP_RESPONSE_Q_NOT_AVAIL,
+ÂÂÂÂÂÂÂÂÂÂÂ };
+ÂÂÂ int retry_tapq = 5;
+ÂÂÂ int retry_aqic = 5;
+
+ÂÂÂ if (!q)

When will q ever be NULL? I checked all places where this is called and
it looks to me like this will never happen.


OK, I check, may be too carefull.

+ÂÂÂÂÂÂÂ return status;
+
+again:

I'm not crazy about using a label, why not a do/while
loop or something of that nature?

I will try this way.


+ÂÂÂ status = ap_aqic(q->apqn, aqic_gisa, NULL);
+ÂÂÂ switch (status.response_code) {
+ÂÂÂ case AP_RESPONSE_OTHERWISE_CHANGED:
+ÂÂÂ case AP_RESPONSE_RESET_IN_PROGRESS:
+ÂÂÂ case AP_RESPONSE_NORMAL: /* Fall through */
+ÂÂÂÂÂÂÂ while (status.irq_enabled && retry_tapq--) {
+ÂÂÂÂÂÂÂÂÂÂÂ msleep(20);
+ÂÂÂÂÂÂÂÂÂÂÂ status = ap_tapq(q->apqn, NULL);

Shouldn't you be checking response codes from the TAPQ
function? Maybe there should be a function call here to
with for IRQ disabled?

OK

Thanks,
Pierre



--
Pierre Morel
Linux/KVM/QEMU in BÃblingen - Germany