Re: [Xen-devel] [PATCH] xen: reuse the same pirq allocated when driverload first time
From: Zhenzhong Duan
Date: Thu Jun 27 2013 - 22:33:14 EST
On 2013-06-27 19:52, Stefano Stabellini wrote:
On Thu, 27 Jun 2013, Zhenzhong Duan wrote:
On 2013-06-27 02:08, Stefano Stabellini wrote:
On Wed, 26 Jun 2013, Zhenzhong Duan wrote:
On 2013-06-26 01:51, Stefano Stabellini wrote:
On Tue, 25 Jun 2013, DuanZhenzhong wrote:
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;
Interesting. I don't think this is the proper fix though.
Does the appended patch change anything?
diff --git a/hw/pt-msi.c b/hw/pt-msi.c
index 71fa6f0..cd5d9c7 100644
--- a/hw/pt-msi.c
+++ b/hw/pt-msi.c
@@ -302,7 +302,7 @@ static int pt_msix_update_one(struct pt_dev *dev,
int
entry_nr)
uint32_t gflags = __get_msi_gflags(entry->io_mem[2], gaddr);
int ret;
- if ( !entry->flags )
+ if ( !entry->flags && ptdev->msix->enabled )
return 0;
if (!gvec) {
Tested, not work.
If you look at msix_capability_init in kernel, line 707,722,
dev->msix->enabled is already set when pt_msix_update is called.
Yeah, but it shouldn't be already set in QEMU. In fact in QEMU
dev->msix->enabled is modified in pt_msixctrl_reg_write after calling to
pt_msix_update.
It does.
line 707, PCI_MSIX_FLAGS_MASKALL | PCI_MSIX_FLAGS_ENABLE is set
this will set dev->msix->enabled first time
line 722, PCI_MSIX_FLAGS_ENABLE set
this trigger call of pt_msix_update
then dev->msix->enabled was set a second time
I was assuming that you needed to add "|| entry->pirq == -1" because you
needed to pass the check:
if ( !entry->flags )
return 0;
at the beginning of pt_msix_update_one. Am I getting it right?
Right, as entry->pirq is set to -1 when driver unload.
If that is case that I thought that we just needed to make sure that
when ptdev->msix->enabled is still zero then we go through the test in
pt_msix_update_one. Where is the mistake?
If you want to use dev->msix->enabled for checking, below patch could work.
But in this case, dev->msix->enabled doesn't represent PCI_MSIX_ENABLE any
more,
but a combination of PCI_MSIX_ENABLE and ~PCI_MSIX_MASK
zduan
***********************************************************
diff -up ./hw/pass-through.c.old2 ./hw/pass-through.c
--- ./hw/pass-through.c.old2 2013-06-27 11:05:30.000000000 +0800
+++ ./hw/pass-through.c 2013-06-27 11:07:35.000000000 +0800
@@ -4027,7 +4027,7 @@ static int pt_msixctrl_reg_write(struct
pt_msix_disable(ptdev);
}
- ptdev->msix->enabled = !!(*value & PCI_MSIX_ENABLE);
+ ptdev->msix->enabled = (*value & PCI_MSIX_ENABLE) && !(*value &
PCI_MSIX_MASK);
return 0;
}
diff -up ./hw/pt-msi.c.old2 ./hw/pt-msi.c
--- ./hw/pt-msi.c.old2 2013-06-27 11:26:12.000000000 +0800
+++ ./hw/pt-msi.c 2013-06-27 11:27:13.000000000 +0800
@@ -294,7 +294,7 @@ static int pt_msix_update_one(struct pt_
uint32_t gflags = __get_msi_gflags(entry->io_mem[2], gaddr);
int ret;
- if ( !entry->flags )
+ if ( !entry->flags && dev->msix->enabled )
return 0;
if (!gvec) {
I understand now, thanks for the explanation.
Among the two alternatives, I think that your first change is actually
better.
However we can probably still improve it a little bit by setting
entry->flags to 1 directly in pt_msix_disable? So that we don't confuse
the driver reload case from the first driver initialization.
diff --git a/hw/pt-msi.c b/hw/pt-msi.c
index 71fa6f0..cc4e280 100644
--- a/hw/pt-msi.c
+++ b/hw/pt-msi.c
@@ -408,7 +408,7 @@ void pt_msix_disable(struct pt_dev *dev)
}
/* clear msi-x info */
entry->pirq = -1;
- entry->flags = 0;
+ entry->flags = 1;
}
}
Test passed.
But this change set entry->flags in all entrys, dev->msix->total_entries
count of pirqs are
mapped when driver reload, no matter how many msix entrys driver is
initializing.
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/