* [PATCH] kvm: device-assignment: Catch GSI overflow
@ 2009-05-07 17:09 Alex Williamson
2009-05-07 17:16 ` Alex Williamson
2009-05-12 22:14 ` [PATCH] kvm: device-assignment: Fix kvm_get_irq_route_gsi() return check Alex Williamson
0 siblings, 2 replies; 9+ messages in thread
From: Alex Williamson @ 2009-05-07 17:09 UTC (permalink / raw)
To: kvm; +Cc: alex.williamson
Fix the index at which we return -ENOSPC since the kernel side will
reject a GSI >= KVM_MAX_IRQ_ROUTES. Also, mask as a signed int before
testing for error.
Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---
hw/device-assignment.c | 2 +-
kvm/libkvm/libkvm.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index a7365c8..e06dd08 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -796,7 +796,7 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev, unsigned int ctrl_pos)
pci_dev->cap.start + PCI_MSI_DATA_32);
assigned_dev->entry->type = KVM_IRQ_ROUTING_MSI;
assigned_dev->entry->gsi = kvm_get_irq_route_gsi(kvm_context);
- if (assigned_dev->entry->gsi < 0) {
+ if ((int)(assigned_dev->entry->gsi) < 0) {
perror("assigned_dev_update_msi: kvm_get_irq_route_gsi");
return;
}
diff --git a/kvm/libkvm/libkvm.c b/kvm/libkvm/libkvm.c
index ba0a5d1..2a4165a 100644
--- a/kvm/libkvm/libkvm.c
+++ b/kvm/libkvm/libkvm.c
@@ -1408,7 +1408,7 @@ int kvm_get_irq_route_gsi(kvm_context_t kvm)
{
#ifdef KVM_CAP_IRQ_ROUTING
if (kvm->max_used_gsi >= KVM_IOAPIC_NUM_PINS) {
- if (kvm->max_used_gsi <= kvm_get_gsi_count(kvm))
+ if (kvm->max_used_gsi + 1 < kvm_get_gsi_count(kvm))
return kvm->max_used_gsi + 1;
else
return -ENOSPC;
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] kvm: device-assignment: Catch GSI overflow
2009-05-07 17:09 [PATCH] kvm: device-assignment: Catch GSI overflow Alex Williamson
@ 2009-05-07 17:16 ` Alex Williamson
2009-05-08 9:28 ` Sheng Yang
2009-05-12 22:14 ` [PATCH] kvm: device-assignment: Fix kvm_get_irq_route_gsi() return check Alex Williamson
1 sibling, 1 reply; 9+ messages in thread
From: Alex Williamson @ 2009-05-07 17:16 UTC (permalink / raw)
To: kvm
On Thu, 2009-05-07 at 11:09 -0600, Alex Williamson wrote:
> Fix the index at which we return -ENOSPC since the kernel side will
> reject a GSI >= KVM_MAX_IRQ_ROUTES. Also, mask as a signed int before
> testing for error.
Even with this, there still seems to be a fundamental problem with our
consumption of GSIs in kvm. For example, every time a guest writes to
the MSI capabilities area and enables MSI support, we call
kvm_get_irq_route_gsi() to get a new max_used_gsi + 1 value, then call
kvm_add_routing_entry(), which updates max_used_gsi. It doesn't take
too long before we exhaust the GSI space and the device no longer works.
This seems to happen within a minute or two of booting a guest with an
e1000e device sitting idle on a busy network. Do we need to keep a
bitmap of used GSIs or maybe just attempt to reuse the GSI we've gotten
previously for the device? Thanks,
Alex
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kvm: device-assignment: Catch GSI overflow
2009-05-07 17:16 ` Alex Williamson
@ 2009-05-08 9:28 ` Sheng Yang
0 siblings, 0 replies; 9+ messages in thread
From: Sheng Yang @ 2009-05-08 9:28 UTC (permalink / raw)
To: kvm; +Cc: Alex Williamson
On Friday 08 May 2009 01:16:53 Alex Williamson wrote:
> On Thu, 2009-05-07 at 11:09 -0600, Alex Williamson wrote:
> > Fix the index at which we return -ENOSPC since the kernel side will
> > reject a GSI >= KVM_MAX_IRQ_ROUTES. Also, mask as a signed int before
> > testing for error.
>
> Even with this, there still seems to be a fundamental problem with our
> consumption of GSIs in kvm. For example, every time a guest writes to
> the MSI capabilities area and enables MSI support, we call
> kvm_get_irq_route_gsi() to get a new max_used_gsi + 1 value, then call
> kvm_add_routing_entry(), which updates max_used_gsi. It doesn't take
> too long before we exhaust the GSI space and the device no longer works.
> This seems to happen within a minute or two of booting a guest with an
> e1000e device sitting idle on a busy network. Do we need to keep a
> bitmap of used GSIs or maybe just attempt to reuse the GSI we've gotten
> previously for the device? Thanks,
Yes, I think a bitmap is much better here.
Thanks.
--
regards
Yang, Sheng
>
> Alex
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] kvm: device-assignment: Fix kvm_get_irq_route_gsi() return check
2009-05-07 17:09 [PATCH] kvm: device-assignment: Catch GSI overflow Alex Williamson
2009-05-07 17:16 ` Alex Williamson
@ 2009-05-12 22:14 ` Alex Williamson
2009-05-13 3:36 ` Yang, Sheng
2009-05-13 4:45 ` [PATCH v2] " Alex Williamson
1 sibling, 2 replies; 9+ messages in thread
From: Alex Williamson @ 2009-05-12 22:14 UTC (permalink / raw)
To: kvm; +Cc: sheng.yang, alex.williamson
Cast to a signed int to test for < 0.
Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---
This supersedes "[PATCH] kvm: device-assignment: Catch GSI overflow"
hw/device-assignment.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index a6cc9b9..5bdae24 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -798,7 +798,7 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev, unsigned int ctrl_pos)
pci_dev->cap.start + PCI_MSI_DATA_32);
assigned_dev->entry->type = KVM_IRQ_ROUTING_MSI;
assigned_dev->entry->gsi = kvm_get_irq_route_gsi(kvm_context);
- if (assigned_dev->entry->gsi < 0) {
+ if ((int)(assigned_dev->entry->gsi) < 0) {
perror("assigned_dev_update_msi: kvm_get_irq_route_gsi");
return;
}
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] kvm: device-assignment: Fix kvm_get_irq_route_gsi() return check
2009-05-12 22:14 ` [PATCH] kvm: device-assignment: Fix kvm_get_irq_route_gsi() return check Alex Williamson
@ 2009-05-13 3:36 ` Yang, Sheng
2009-05-13 3:55 ` Alex Williamson
2009-05-13 4:45 ` [PATCH v2] " Alex Williamson
1 sibling, 1 reply; 9+ messages in thread
From: Yang, Sheng @ 2009-05-13 3:36 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm
On Wednesday 13 May 2009 06:14:01 Alex Williamson wrote:
> Cast to a signed int to test for < 0.
>
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> ---
>
> This supersedes "[PATCH] kvm: device-assignment: Catch GSI overflow"
>
> hw/device-assignment.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index a6cc9b9..5bdae24 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -798,7 +798,7 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev,
> unsigned int ctrl_pos) pci_dev->cap.start + PCI_MSI_DATA_32);
> assigned_dev->entry->type = KVM_IRQ_ROUTING_MSI;
> assigned_dev->entry->gsi = kvm_get_irq_route_gsi(kvm_context);
> - if (assigned_dev->entry->gsi < 0) {
> + if ((int)(assigned_dev->entry->gsi) < 0) {
> perror("assigned_dev_update_msi: kvm_get_irq_route_gsi");
> return;
> }
Use a return value(r) seems better.
And I realized there is memory leak here. Entry seems haven't been freed for
error... So does MSI-X...
--
regards
Yang, Sheng
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kvm: device-assignment: Fix kvm_get_irq_route_gsi() return check
2009-05-13 3:36 ` Yang, Sheng
@ 2009-05-13 3:55 ` Alex Williamson
2009-05-13 4:09 ` Yang, Sheng
0 siblings, 1 reply; 9+ messages in thread
From: Alex Williamson @ 2009-05-13 3:55 UTC (permalink / raw)
To: Yang, Sheng; +Cc: kvm
On Wed, 2009-05-13 at 11:36 +0800, Yang, Sheng wrote:
> On Wednesday 13 May 2009 06:14:01 Alex Williamson wrote:
> > --- a/hw/device-assignment.c
> > +++ b/hw/device-assignment.c
> > @@ -798,7 +798,7 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev,
> > unsigned int ctrl_pos) pci_dev->cap.start + PCI_MSI_DATA_32);
> > assigned_dev->entry->type = KVM_IRQ_ROUTING_MSI;
> > assigned_dev->entry->gsi = kvm_get_irq_route_gsi(kvm_context);
> > - if (assigned_dev->entry->gsi < 0) {
> > + if ((int)(assigned_dev->entry->gsi) < 0) {
> > perror("assigned_dev_update_msi: kvm_get_irq_route_gsi");
> > return;
> > }
>
> Use a return value(r) seems better.
Hi Sheng,
Do you mean:
r = kvm_get_irq_route_gsi(kvm_context);
if (r < 0) {
...
}
assigned_dev->entry->gsi = r;
> And I realized there is memory leak here. Entry seems haven't been freed for
> error... So does MSI-X...
I hadn't noticed that one, but now that you mention it, yep. Thanks,
Alex
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kvm: device-assignment: Fix kvm_get_irq_route_gsi() return check
2009-05-13 3:55 ` Alex Williamson
@ 2009-05-13 4:09 ` Yang, Sheng
0 siblings, 0 replies; 9+ messages in thread
From: Yang, Sheng @ 2009-05-13 4:09 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm
On Wednesday 13 May 2009 11:55:50 Alex Williamson wrote:
> On Wed, 2009-05-13 at 11:36 +0800, Yang, Sheng wrote:
> > On Wednesday 13 May 2009 06:14:01 Alex Williamson wrote:
> > > --- a/hw/device-assignment.c
> > > +++ b/hw/device-assignment.c
> > > @@ -798,7 +798,7 @@ static void assigned_dev_update_msi(PCIDevice
> > > *pci_dev, unsigned int ctrl_pos) pci_dev->cap.start + PCI_MSI_DATA_32);
> > > assigned_dev->entry->type = KVM_IRQ_ROUTING_MSI;
> > > assigned_dev->entry->gsi = kvm_get_irq_route_gsi(kvm_context);
> > > - if (assigned_dev->entry->gsi < 0) {
> > > + if ((int)(assigned_dev->entry->gsi) < 0) {
> > > perror("assigned_dev_update_msi: kvm_get_irq_route_gsi");
> > > return;
> > > }
> >
> > Use a return value(r) seems better.
>
> Hi Sheng,
>
> Do you mean:
>
> r = kvm_get_irq_route_gsi(kvm_context);
> if (r < 0) {
> ...
> }
> assigned_dev->entry->gsi = r;
Yes.
> > And I realized there is memory leak here. Entry seems haven't been freed
> > for error... So does MSI-X...
>
> I hadn't noticed that one, but now that you mention it, yep. Thanks,
Thanks. :)
--
regards
Yang, Sheng
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] kvm: device-assignment: Fix kvm_get_irq_route_gsi() return check
2009-05-12 22:14 ` [PATCH] kvm: device-assignment: Fix kvm_get_irq_route_gsi() return check Alex Williamson
2009-05-13 3:36 ` Yang, Sheng
@ 2009-05-13 4:45 ` Alex Williamson
2009-05-13 4:58 ` Yang, Sheng
1 sibling, 1 reply; 9+ messages in thread
From: Alex Williamson @ 2009-05-13 4:45 UTC (permalink / raw)
To: kvm; +Cc: sheng.yang, alex.williamson
Use 'r' for the return value since gsi is unsigned.
Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---
v2: Use 'r' instead of a cast, per Sheng Yang
hw/device-assignment.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index a6cc9b9..4107915 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -797,11 +797,12 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev, unsigned int ctrl_pos)
assigned_dev->entry->u.msi.data = *(uint16_t *)(pci_dev->config +
pci_dev->cap.start + PCI_MSI_DATA_32);
assigned_dev->entry->type = KVM_IRQ_ROUTING_MSI;
- assigned_dev->entry->gsi = kvm_get_irq_route_gsi(kvm_context);
- if (assigned_dev->entry->gsi < 0) {
+ r = kvm_get_irq_route_gsi(kvm_context);
+ if (r < 0) {
perror("assigned_dev_update_msi: kvm_get_irq_route_gsi");
return;
}
+ assigned_dev->entry->gsi = r;
kvm_add_routing_entry(kvm_context, assigned_dev->entry);
if (kvm_commit_irq_routes(kvm_context) < 0) {
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] kvm: device-assignment: Fix kvm_get_irq_route_gsi() return check
2009-05-13 4:45 ` [PATCH v2] " Alex Williamson
@ 2009-05-13 4:58 ` Yang, Sheng
0 siblings, 0 replies; 9+ messages in thread
From: Yang, Sheng @ 2009-05-13 4:58 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm
On Wednesday 13 May 2009 12:45:11 Alex Williamson wrote:
> Use 'r' for the return value since gsi is unsigned.
>
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> ---
Acked.
--
regards
Yang, Sheng
>
> v2: Use 'r' instead of a cast, per Sheng Yang
>
> hw/device-assignment.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index a6cc9b9..4107915 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -797,11 +797,12 @@ static void assigned_dev_update_msi(PCIDevice
> *pci_dev, unsigned int ctrl_pos) assigned_dev->entry->u.msi.data =
> *(uint16_t *)(pci_dev->config + pci_dev->cap.start + PCI_MSI_DATA_32);
> assigned_dev->entry->type = KVM_IRQ_ROUTING_MSI;
> - assigned_dev->entry->gsi = kvm_get_irq_route_gsi(kvm_context);
> - if (assigned_dev->entry->gsi < 0) {
> + r = kvm_get_irq_route_gsi(kvm_context);
> + if (r < 0) {
> perror("assigned_dev_update_msi: kvm_get_irq_route_gsi");
> return;
> }
> + assigned_dev->entry->gsi = r;
>
> kvm_add_routing_entry(kvm_context, assigned_dev->entry);
> if (kvm_commit_irq_routes(kvm_context) < 0) {
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2009-05-13 4:57 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-05-07 17:09 [PATCH] kvm: device-assignment: Catch GSI overflow Alex Williamson
2009-05-07 17:16 ` Alex Williamson
2009-05-08 9:28 ` Sheng Yang
2009-05-12 22:14 ` [PATCH] kvm: device-assignment: Fix kvm_get_irq_route_gsi() return check Alex Williamson
2009-05-13 3:36 ` Yang, Sheng
2009-05-13 3:55 ` Alex Williamson
2009-05-13 4:09 ` Yang, Sheng
2009-05-13 4:45 ` [PATCH v2] " Alex Williamson
2009-05-13 4:58 ` Yang, Sheng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.