Re: [Xen-devel] [PATCH] xen: reuse the same pirq allocated when driverload first time

From: DuanZhenzhong
Date: Tue Jun 25 2013 - 01:33:35 EST

Stefano Stabellini wrote:
Trimming some of the people in CC

On Mon, 24 Jun 2013, Zhenzhong Duan wrote:
On 2013-06-20 22:21, Stefano Stabellini wrote:
On Thu, 20 Jun 2013, Zhenzhong Duan wrote:
On 2013-06-05 20:50, Stefano Stabellini wrote:
On Wed, 5 Jun 2013, Zhenzhong Duan wrote:
Stefano Stabellini wrote:

On Tue, 21 May 2013, Stefano Stabellini wrote:
On Tue, 21 May 2013, Konrad Rzeszutek Wilk wrote:
Looking at the hypervisor code I couldn't see anything obviously
wrong.
I think the culprit is "physdev_unmap_pirq":

if ( is_hvm_domain(d) )
{
spin_lock(&d->event_lock);
gdprintk(XENLOG_WARNING,"d%d, pirq: %d is %x %s, irq: %d\n",
d->domain_id, pirq, domain_pirq_to_emuirq(d, pirq),
domain_pirq_to_emuirq(d, pirq) == IRQ_UNBOUND ?
"unbound" :
"",
domain_pirq_to_irq(d, pirq));
if
( domain_pirq_to_emuirq(d, pirq) != IRQ_UNBOUND )
ret = unmap_domain_pirq_emuirq(d, pirq);
spin_unlock(&d->event_lock);
if ( domid == DOMID_SELF || ret )
goto free_domain;

It always tells me unbound:

(XEN) physdev.c:237:d14 14, pirq: 54 is ffffffff
(XEN) irq.c:1873:d14 14, nr_pirqs: 56
(XEN) physdev.c:237:d14 14, pirq: 53 is ffffffff
(XEN) irq.c:1873:d14 14, nr_pirqs: 56
(XEN) physdev.c:237:d14 14, pirq: 52 is ffffffff
(XEN) irq.c:1873:d14 14, nr_pirqs: 56
(XEN) physdev.c:237:d14 14, pirq: 51 is ffffffff
(XEN) irq.c:1873:d14 14, nr_pirqs: 56
(XEN) physdev.c:237:d14 14, pirq: 50 is ffffffff
(XEN) irq.c:1873:d14 14, nr_pirqs: 56
(a bit older debug code, so the 'unbound' does not show up here).

Which means that the call to unmap_domain_pirq_emuirq does not happen.
The checks in unmap_domain_pirq_emuirq also look to be depend
on the code being IRQ_UNBOUND.

In other words, all of that code looks to only clear things when
they are !IRQ_UNBOUND.

But the other logic (IRQ_UNBOUND) looks to be missing a removal
in the radix tree:

if ( emuirq != IRQ_PT )
radix_tree_delete(&d->arch.hvm_domain.emuirq_pirq, emuirq);
And
I think that is what is causing the leak - the radix tree
needs to be pruned? Or perhaps the allocate_pirq should check
the radix tree for IRQ_UNBOUND ones and re-use them?
I think that you are looking in the wrong place.
The issue is that QEMU doesn't call pt_msi_disable in
pt_msgctrl_reg_write if (!val & PCI_MSI_FLAGS_ENABLE).

The code above is correct as is because it is trying to handle
emulated
IRQs and MSIs, not real passthrough MSIs. They latter are not added to
that radix tree, see physdev_hvm_map_pirq and physdev_map_pirq.
This patch fixes the issue, I have only tested MSI (MSI-X completely
untested).


diff --git a/hw/pass-through.c b/hw/pass-through.c
index 304c438..079e465 100644
--- a/hw/pass-through.c
+++ b/hw/pass-through.c
@@ -3866,7 +3866,11 @@ static int pt_msgctrl_reg_write(struct pt_dev
*ptdev,
ptdev->msi->flags |= PCI_MSI_FLAGS_ENABLE;
}
else
- ptdev->msi->flags &= ~PCI_MSI_FLAGS_ENABLE;
+ {
+ if (ptdev->msi->flags & PT_MSI_MAPPED) {
+ pt_msi_disable(ptdev);
+ }
+ }
/* pass through MSI_ENABLE bit when no MSI-INTx translation
*/
if (!ptdev->msi_trans_en) {
@@ -4013,6 +4017,8 @@ static int pt_msixctrl_reg_write(struct pt_dev
*ptdev,
pt_disable_msi_translate(ptdev);
}
pt_msix_update(ptdev);
+ } else if (!(*value & PCI_MSIX_ENABLE) && ptdev->msix->enabled) {
+ pt_msix_delete(ptdev);
Hi Stefano,
I made a test with this patch, os reboot when driver reload. If use
pt_msix_disable
instead of pt_msix_delete, driver could be reloaded.
But I still see some error in qemu.log and xen console. Seems four
IRQs
are not freed
when unmap.
--------------first load---------------------------
pt_msix_update_one: pt_msix_update_one requested pirq = 103
pt_msix_update_one: Update msix entry 0 with pirq 67 gvec 0
pt_msix_update_one: pt_msix_update_one requested pirq = 102
pt_msix_update_one: Update msix entry 1 with pirq 66 gvec 0
pt_msix_update_one: pt_msix_update_one requested pirq = 101
pt_msix_update_one: Update msix entry 2 with pirq 65 gvec 0
pt_msix_update_one: pt_msix_update_one requested pirq = 100
pt_msix_update_one: Update msix entry 3 with pirq 64 gvec 0
------------- first unload---------------------------
pt_msix_disable: Unbind msix with pirq 67, gvec 0
pt_msix_disable: Unmap msix with pirq 67
pt_msix_disable: Error: Unmapping of MSI-X failed. [00:04.0]
pt_msix_disable: Unbind msix with pirq 66, gvec 0
pt_msix_disable: Unmap msix with pirq 66
pt_msix_disable: Error: Unmapping of MSI-X failed. [00:04.0]
pt_msix_disable: Unbind msix with pirq 65, gvec 0
pt_msix_disable: Unmap msix with pirq 65
pt_msix_disable: Error: Unmapping of MSI-X failed. [00:04.0]
pt_msix_disable: Unbind msix with pirq 64, gvec 0
pt_msix_disable: Unmap msix with pirq 64
pt_msix_disable: Error: Unmapping of MSI-X failed. [00:04.0]
Can you add some printks in Xen (the hypercall name is
PHYSDEVOP_unmap_pirq) to figure out exactly why they are failing?
Did some test, domain_pirq_to_emuirq(d, unmap->pirq) = IRQ_UNBOUND in
physdev_unmap_pirq.
That means that Linux didn't call irq_enable on the MSI-X in question:

irq_enable -> __startup_pirq -> EVTCHNOP_bind_pirq

EVTCHNOP_bind_pirq is implemented by evtchn_bind_pirq in Xen and calls
map_domain_emuirq_pirq, so domain_pirq_to_emuirq(d, unmap->pirq) should
be IRQ_PT.

I don't know if that's a normal condition, but in any case it should
not create any problems to physdev_unmap_pirq, in fact the folloing
check:

if ( domid == DOMID_SELF || ret )
goto free_domain;

should fail so Xen should continue and execute unmap_domain_pirq. That's
what we want.
From linux side, request_irq-> request_threaded_irq-> __setup_irq->
irq_startup-> startup_pirq-> EVTCHNOP_bind_pirq
If irq_enable isn't called, how does the driver receive interrupt, I did see
four interrupts in /proc/interrupt and driver works ok.

Good to know

Could you have a look if there is something wrong in xen side of clearing the
mapping?

What I am saying is that the error you are getting:

pt_msix_disable: Unbind msix with pirq 67, gvec 0
pt_msix_disable: Unmap msix with pirq 67
pt_msix_disable: Error: Unmapping of MSI-X failed. [00:04.0]

cannot be caused by domain_pirq_to_emuirq(d, pirq) returning
IRQ_UNBOUND.
So, why are you getting this error? What is failing?
I am ready to believe the problem is in Xen but Without understanding
why you are getting the error it's hard to find a solution.
I found the reason, you are looking at xen-unstable, I was working with 4.1.30-OVM, it has patch of CVE-2012-4536 / XSA-21.
That patch set ret to -EINVAL initially. After remove that line, unmap succeed.
But we still need below patch to let driver reload succeed everytime. Without that, 1st reload failed, 2nd succeed, 3 failed, ...

diff -up --new-file ./hw/pt-msi.c.old1 ./hw/pt-msi.c
--- ./hw/pt-msi.c.old1 2013-06-26 01:36:08.000000000 +0800
+++ ./hw/pt-msi.c 2013-06-26 01:37:41.000000000 +0800
@@ -469,7 +469,7 @@ static void pci_msix_writel(void *opaque
return;
}

- if ( offset != 3 && entry->io_mem[offset] != val )
+ if ( offset != 3 && (entry->io_mem[offset] != val || entry->pirq == -1))
entry->flags = 1;
entry->io_mem[offset] = val;

zduan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/