[PATCH] kvm: device-assignment: Catch GSI overflow

[PATCH] kvm: device-assignment: Catch GSI overflow

[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;

Re: [PATCH] kvm: device-assignment: Catch GSI overflow

[Alex Williamson]

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

Re: [PATCH] kvm: device-assignment: Catch GSI overflow

[Sheng Yang]

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

[PATCH] kvm: device-assignment: Fix kvm_get_irq_route_gsi() return check

[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;
         }

Re: [PATCH] kvm: device-assignment: Fix kvm_get_irq_route_gsi() return check

[Yang, Sheng]

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

Re: [PATCH] kvm: device-assignment: Fix kvm_get_irq_route_gsi() return check

[Alex Williamson]

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

Re: [PATCH] kvm: device-assignment: Fix kvm_get_irq_route_gsi() return check

[Yang, Sheng]

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

[PATCH v2] kvm: device-assignment: Fix kvm_get_irq_route_gsi() return check

[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) {

Re: [PATCH v2] kvm: device-assignment: Fix kvm_get_irq_route_gsi() return check

[Yang, Sheng]

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) {