[Qemu-devel] [PATCH 0/3] spapr/pci: Improve MSI tracking

[Qemu-devel] [PATCH 0/3] spapr/pci: Improve MSI tracking

[Greg Kurz]

Some recent tests with AIX guests showed that we don't tear down
MSIs that were allocated with the "change-msi" RTAS call, when
the guest is rebooted. This series teach PHBs to do the cleanup
at reset time.

This bug has always been there. Not sure it is worth the pain to
have this fixed in 4.1.

--
Greg

---

Greg Kurz (3):
      spapr/pci: Consolidate de-allocation of MSIs
      spapr/pci: Free MSIs during reset
      spapr/irq: Drop spapr_irq_msi_reset()


 hw/ppc/spapr.c             |    4 ----
 hw/ppc/spapr_irq.c         |    7 ++-----
 hw/ppc/spapr_pci.c         |   26 +++++++++++++++++---------
 include/hw/ppc/spapr_irq.h |    1 -
 4 files changed, 19 insertions(+), 19 deletions(-)

[Qemu-devel] [PATCH 1/3] spapr/pci: Consolidate de-allocation of MSIs

[Greg Kurz]

When freeing MSIs, we need to:
- remove them from the machine's MSI bitmap
- remove them from the IC backend
- remove them from the PHB's MSI cache

This is currently open coded in two places in rtas_ibm_change_msi(),
and we're about to need this in spapr_phb_reset() as well. Instead of
duplicating this code again, make it a destroy function for the PHB's
MSI cache. Removing an MSI device from the cache will call the destroy
function internally.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_pci.c |   24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 2d5697d119f4..bc22568bfa71 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -336,10 +336,6 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, SpaprMachineState *spapr,
             return;
         }
 
-        if (!smc->legacy_irq_allocation) {
-            spapr_irq_msi_free(spapr, msi->first_irq, msi->num);
-        }
-        spapr_irq_free(spapr, msi->first_irq, msi->num);
         if (msi_present(pdev)) {
             spapr_msi_setmsg(pdev, 0, false, 0, 0);
         }
@@ -409,10 +405,6 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, SpaprMachineState *spapr,
 
     /* Release previous MSIs */
     if (msi) {
-        if (!smc->legacy_irq_allocation) {
-            spapr_irq_msi_free(spapr, msi->first_irq, msi->num);
-        }
-        spapr_irq_free(spapr, msi->first_irq, msi->num);
         g_hash_table_remove(phb->msi, &config_addr);
     }
 
@@ -1806,6 +1798,19 @@ static void spapr_phb_unrealize(DeviceState *dev, Error **errp)
     memory_region_del_subregion(get_system_memory(), &sphb->mem32window);
 }
 
+static void spapr_phb_destroy_msi(gpointer opaque)
+{
+    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+    SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
+    spapr_pci_msi *msi = opaque;
+
+    if (!smc->legacy_irq_allocation) {
+        spapr_irq_msi_free(spapr, msi->first_irq, msi->num);
+    }
+    spapr_irq_free(spapr, msi->first_irq, msi->num);
+    g_free(msi);
+}
+
 static void spapr_phb_realize(DeviceState *dev, Error **errp)
 {
     /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
@@ -2017,7 +2022,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
                                     spapr_tce_get_iommu(tcet));
     }
 
-    sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free);
+    sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free,
+                                      spapr_phb_destroy_msi);
     return;
 
 unrealize:

Re: [Qemu-devel] [PATCH 1/3] spapr/pci: Consolidate de-allocation of MSIs

[Cédric Le Goater]

On 26/07/2019 16:44, Greg Kurz wrote:
> When freeing MSIs, we need to:
> - remove them from the machine's MSI bitmap
> - remove them from the IC backend
> - remove them from the PHB's MSI cache
> 
> This is currently open coded in two places in rtas_ibm_change_msi(),
> and we're about to need this in spapr_phb_reset() as well. Instead of
> duplicating this code again, make it a destroy function for the PHB's
> MSI cache. Removing an MSI device from the cache will call the destroy
> function internally.

This looks good.



Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr_pci.c |   24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 2d5697d119f4..bc22568bfa71 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -336,10 +336,6 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, SpaprMachineState *spapr,
>              return;
>          }
>  
> -        if (!smc->legacy_irq_allocation) {
> -            spapr_irq_msi_free(spapr, msi->first_irq, msi->num);
> -        }
> -        spapr_irq_free(spapr, msi->first_irq, msi->num);
>          if (msi_present(pdev)) {
>              spapr_msi_setmsg(pdev, 0, false, 0, 0);
>          }
> @@ -409,10 +405,6 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, SpaprMachineState *spapr,
>  
>      /* Release previous MSIs */
>      if (msi) {
> -        if (!smc->legacy_irq_allocation) {
> -            spapr_irq_msi_free(spapr, msi->first_irq, msi->num);
> -        }
> -        spapr_irq_free(spapr, msi->first_irq, msi->num);
>          g_hash_table_remove(phb->msi, &config_addr);
>      }
>
> @@ -1806,6 +1798,19 @@ static void spapr_phb_unrealize(DeviceState *dev, Error **errp)
>      memory_region_del_subregion(get_system_memory(), &sphb->mem32window);
>  }
>  
> +static void spapr_phb_destroy_msi(gpointer opaque)
> +{
> +    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +    SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +    spapr_pci_msi *msi = opaque;
> +
> +    if (!smc->legacy_irq_allocation) {
> +        spapr_irq_msi_free(spapr, msi->first_irq, msi->num);
> +    }
> +    spapr_irq_free(spapr, msi->first_irq, msi->num);
> +    g_free(msi);
> +}
> +
>  static void spapr_phb_realize(DeviceState *dev, Error **errp)
>  {
>      /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
> @@ -2017,7 +2022,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>                                      spapr_tce_get_iommu(tcet));
>      }
>  
> -    sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free);
> +    sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free,
> +                                      spapr_phb_destroy_msi);
>      return;
>  
>  unrealize:
> 

Re: [Qemu-devel] [PATCH 1/3] spapr/pci: Consolidate de-allocation of MSIs

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3161 bytes --]

On Fri, Jul 26, 2019 at 04:44:38PM +0200, Greg Kurz wrote:
> When freeing MSIs, we need to:
> - remove them from the machine's MSI bitmap
> - remove them from the IC backend
> - remove them from the PHB's MSI cache
> 
> This is currently open coded in two places in rtas_ibm_change_msi(),
> and we're about to need this in spapr_phb_reset() as well. Instead of
> duplicating this code again, make it a destroy function for the PHB's
> MSI cache. Removing an MSI device from the cache will call the destroy
> function internally.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-4.2.

> ---
>  hw/ppc/spapr_pci.c |   24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 2d5697d119f4..bc22568bfa71 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -336,10 +336,6 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, SpaprMachineState *spapr,
>              return;
>          }
>  
> -        if (!smc->legacy_irq_allocation) {
> -            spapr_irq_msi_free(spapr, msi->first_irq, msi->num);
> -        }
> -        spapr_irq_free(spapr, msi->first_irq, msi->num);
>          if (msi_present(pdev)) {
>              spapr_msi_setmsg(pdev, 0, false, 0, 0);
>          }
> @@ -409,10 +405,6 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, SpaprMachineState *spapr,
>  
>      /* Release previous MSIs */
>      if (msi) {
> -        if (!smc->legacy_irq_allocation) {
> -            spapr_irq_msi_free(spapr, msi->first_irq, msi->num);
> -        }
> -        spapr_irq_free(spapr, msi->first_irq, msi->num);
>          g_hash_table_remove(phb->msi, &config_addr);
>      }
>  
> @@ -1806,6 +1798,19 @@ static void spapr_phb_unrealize(DeviceState *dev, Error **errp)
>      memory_region_del_subregion(get_system_memory(), &sphb->mem32window);
>  }
>  
> +static void spapr_phb_destroy_msi(gpointer opaque)
> +{
> +    SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +    SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> +    spapr_pci_msi *msi = opaque;
> +
> +    if (!smc->legacy_irq_allocation) {
> +        spapr_irq_msi_free(spapr, msi->first_irq, msi->num);
> +    }
> +    spapr_irq_free(spapr, msi->first_irq, msi->num);
> +    g_free(msi);
> +}
> +
>  static void spapr_phb_realize(DeviceState *dev, Error **errp)
>  {
>      /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
> @@ -2017,7 +2022,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>                                      spapr_tce_get_iommu(tcet));
>      }
>  
> -    sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free);
> +    sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free,
> +                                      spapr_phb_destroy_msi);
>      return;
>  
>  unrealize:
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 2/3] spapr/pci: Free MSIs during reset

[Greg Kurz]

When the machine is reset, the MSI bitmap is cleared but the allocated
MSIs are not freed. Some operating systems, such as AIX, can detect the
previous configuration and assert.

Empty the MSI cache, this performs the needed cleanup.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_pci.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index bc22568bfa71..e45507bf2b53 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -2078,6 +2078,8 @@ static void spapr_phb_reset(DeviceState *qdev)
     if (spapr_phb_eeh_available(SPAPR_PCI_HOST_BRIDGE(qdev))) {
         spapr_phb_vfio_reset(qdev);
     }
+
+    g_hash_table_remove_all(sphb->msi);
 }
 
 static Property spapr_phb_properties[] = {

Re: [Qemu-devel] [PATCH 2/3] spapr/pci: Free MSIs during reset

[Cédric Le Goater]

On 26/07/2019 16:44, Greg Kurz wrote:
> When the machine is reset, the MSI bitmap is cleared but the allocated
> MSIs are not freed. Some operating systems, such as AIX, can detect the
> previous configuration and assert.
> 
> Empty the MSI cache, this performs the needed cleanup.

This is fixing the reset bug.



Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr_pci.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index bc22568bfa71..e45507bf2b53 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -2078,6 +2078,8 @@ static void spapr_phb_reset(DeviceState *qdev)
>      if (spapr_phb_eeh_available(SPAPR_PCI_HOST_BRIDGE(qdev))) {
>          spapr_phb_vfio_reset(qdev);
>      }
> +
> +    g_hash_table_remove_all(sphb->msi);
>  }
>  
>  static Property spapr_phb_properties[] = {
> 

Re: [Qemu-devel] [PATCH 2/3] spapr/pci: Free MSIs during reset

[Philippe Mathieu-Daudé]

Cc'ing qemu-stable@

On 7/26/19 4:44 PM, Greg Kurz wrote:
> When the machine is reset, the MSI bitmap is cleared but the allocated
> MSIs are not freed. Some operating systems, such as AIX, can detect the
> previous configuration and assert.
> 
> Empty the MSI cache, this performs the needed cleanup.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr_pci.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index bc22568bfa71..e45507bf2b53 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -2078,6 +2078,8 @@ static void spapr_phb_reset(DeviceState *qdev)
>      if (spapr_phb_eeh_available(SPAPR_PCI_HOST_BRIDGE(qdev))) {
>          spapr_phb_vfio_reset(qdev);
>      }
> +
> +    g_hash_table_remove_all(sphb->msi);

It is not clear to my why spapr_phb_unrealize() doesn't require the same
call, but this is not related to this patch.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>  }
>  
>  static Property spapr_phb_properties[] = {
> 
> 

Re: [Qemu-devel] [PATCH 2/3] spapr/pci: Free MSIs during reset

[Greg Kurz]

On Fri, 26 Jul 2019 18:17:57 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Cc'ing qemu-stable@
> 

This patch relies on the previous one, otherwise g_hash_table_remove_all() will
just g_free() the spapr_pci_msi structures, but it will not tear down the MSIs
in the interrupt controller.

Also, this bug is so old that I'm not sure it qualifies for stable.

> On 7/26/19 4:44 PM, Greg Kurz wrote:
> > When the machine is reset, the MSI bitmap is cleared but the allocated
> > MSIs are not freed. Some operating systems, such as AIX, can detect the
> > previous configuration and assert.
> > 
> > Empty the MSI cache, this performs the needed cleanup.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/ppc/spapr_pci.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index bc22568bfa71..e45507bf2b53 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -2078,6 +2078,8 @@ static void spapr_phb_reset(DeviceState *qdev)
> >      if (spapr_phb_eeh_available(SPAPR_PCI_HOST_BRIDGE(qdev))) {
> >          spapr_phb_vfio_reset(qdev);
> >      }
> > +
> > +    g_hash_table_remove_all(sphb->msi);
> 
> It is not clear to my why spapr_phb_unrealize() doesn't require the same
> call, but this is not related to this patch.
> 

Because spapr_phb_unrealize() does this:

    if (sphb->msi) {
        g_hash_table_unref(sphb->msi);
        sphb->msi = NULL;
    }

and

https://developer.gnome.org/glib/stable/glib-Hash-Tables.html#g-hash-table-unref

g_hash_table_unref ()

void
g_hash_table_unref (GHashTable *hash_table);

Atomically decrements the reference count of hash_table by one. If the reference
count drops to 0, all keys and values will be destroyed, and all memory allocated
by the hash table is released. This function is MT-safe and may be called from any
thread.

If I have to re-post, I can make it clear with a comment.

> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> >  }
> >  
> >  static Property spapr_phb_properties[] = {
> > 
> > 

Re: [Qemu-devel] [PATCH 2/3] spapr/pci: Free MSIs during reset

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1127 bytes --]

On Fri, Jul 26, 2019 at 04:44:44PM +0200, Greg Kurz wrote:
> When the machine is reset, the MSI bitmap is cleared but the allocated
> MSIs are not freed. Some operating systems, such as AIX, can detect the
> previous configuration and assert.
> 
> Empty the MSI cache, this performs the needed cleanup.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-4.2, thanks.

> ---
>  hw/ppc/spapr_pci.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index bc22568bfa71..e45507bf2b53 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -2078,6 +2078,8 @@ static void spapr_phb_reset(DeviceState *qdev)
>      if (spapr_phb_eeh_available(SPAPR_PCI_HOST_BRIDGE(qdev))) {
>          spapr_phb_vfio_reset(qdev);
>      }
> +
> +    g_hash_table_remove_all(sphb->msi);
>  }
>  
>  static Property spapr_phb_properties[] = {
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 3/3] spapr/irq: Drop spapr_irq_msi_reset()

[Greg Kurz]

PHBs already take care of clearing the MSIs from the bitmap during reset
or unplug. No need to do this globally from the machine code. Rather add
an assert to ensure that PHBs have acted as expected.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c             |    4 ----
 hw/ppc/spapr_irq.c         |    7 ++-----
 include/hw/ppc/spapr_irq.h |    1 -
 3 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 5894329f29a9..855e9fbd9805 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1739,10 +1739,6 @@ static void spapr_machine_reset(MachineState *machine)
         ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
     }
 
-    if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
-        spapr_irq_msi_reset(spapr);
-    }
-
     /*
      * NVLink2-connected GPU RAM needs to be placed on a separate NUMA node.
      * We assign a new numa ID per GPU in spapr_pci_collect_nvgpu() which is
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index d07aed8ca9f9..c72d8433681d 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -57,11 +57,6 @@ void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num)
     bitmap_clear(spapr->irq_map, irq - SPAPR_IRQ_MSI, num);
 }
 
-void spapr_irq_msi_reset(SpaprMachineState *spapr)
-{
-    bitmap_clear(spapr->irq_map, 0, spapr->irq_map_nr);
-}
-
 static void spapr_irq_init_kvm(SpaprMachineState *spapr,
                                   SpaprIrq *irq, Error **errp)
 {
@@ -729,6 +724,8 @@ int spapr_irq_post_load(SpaprMachineState *spapr, int version_id)
 
 void spapr_irq_reset(SpaprMachineState *spapr, Error **errp)
 {
+    assert(bitmap_empty(spapr->irq_map, spapr->irq_map_nr));
+
     if (spapr->irq->reset) {
         spapr->irq->reset(spapr, errp);
     }
diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
index f965a58f8954..44fe4f9e0e2e 100644
--- a/include/hw/ppc/spapr_irq.h
+++ b/include/hw/ppc/spapr_irq.h
@@ -28,7 +28,6 @@ void spapr_irq_msi_init(SpaprMachineState *spapr, uint32_t nr_msis);
 int spapr_irq_msi_alloc(SpaprMachineState *spapr, uint32_t num, bool align,
                         Error **errp);
 void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num);
-void spapr_irq_msi_reset(SpaprMachineState *spapr);
 
 typedef struct SpaprIrq {
     uint32_t    nr_irqs;

Re: [Qemu-devel] [PATCH 3/3] spapr/irq: Drop spapr_irq_msi_reset()

[Cédric Le Goater]

On 26/07/2019 16:44, Greg Kurz wrote:
> PHBs already take care of clearing the MSIs from the bitmap during reset
> or unplug. No need to do this globally from the machine code. Rather add
> an assert to ensure that PHBs have acted as expected.

This works because spar_irq_reset() is called after qemu_devices_reset(). 
I guess this is fine.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/spapr.c             |    4 ----
>  hw/ppc/spapr_irq.c         |    7 ++-----
>  include/hw/ppc/spapr_irq.h |    1 -
>  3 files changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5894329f29a9..855e9fbd9805 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1739,10 +1739,6 @@ static void spapr_machine_reset(MachineState *machine)
>          ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
>      }
>  
> -    if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
> -        spapr_irq_msi_reset(spapr);
> -    }
> -
>      /*
>       * NVLink2-connected GPU RAM needs to be placed on a separate NUMA node.
>       * We assign a new numa ID per GPU in spapr_pci_collect_nvgpu() which is
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index d07aed8ca9f9..c72d8433681d 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -57,11 +57,6 @@ void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num)
>      bitmap_clear(spapr->irq_map, irq - SPAPR_IRQ_MSI, num);
>  }
>  
> -void spapr_irq_msi_reset(SpaprMachineState *spapr)
> -{
> -    bitmap_clear(spapr->irq_map, 0, spapr->irq_map_nr);
> -}
> -
>  static void spapr_irq_init_kvm(SpaprMachineState *spapr,
>                                    SpaprIrq *irq, Error **errp)
>  {
> @@ -729,6 +724,8 @@ int spapr_irq_post_load(SpaprMachineState *spapr, int version_id)
>  
>  void spapr_irq_reset(SpaprMachineState *spapr, Error **errp)
>  {
> +    assert(bitmap_empty(spapr->irq_map, spapr->irq_map_nr));
> +
>      if (spapr->irq->reset) {
>          spapr->irq->reset(spapr, errp);
>      }
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index f965a58f8954..44fe4f9e0e2e 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -28,7 +28,6 @@ void spapr_irq_msi_init(SpaprMachineState *spapr, uint32_t nr_msis);
>  int spapr_irq_msi_alloc(SpaprMachineState *spapr, uint32_t num, bool align,
>                          Error **errp);
>  void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num);
> -void spapr_irq_msi_reset(SpaprMachineState *spapr);
>  
>  typedef struct SpaprIrq {
>      uint32_t    nr_irqs;
> 

Re: [Qemu-devel] [PATCH 3/3] spapr/irq: Drop spapr_irq_msi_reset()

[Greg Kurz]

On Fri, 26 Jul 2019 17:01:36 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 26/07/2019 16:44, Greg Kurz wrote:
> > PHBs already take care of clearing the MSIs from the bitmap during reset
> > or unplug. No need to do this globally from the machine code. Rather add
> > an assert to ensure that PHBs have acted as expected.
> 
> This works because spar_irq_reset() is called after qemu_devices_reset(). 
> I guess this is fine.
> 

Yeah and we have this comment in spapr_machine_reset():

    /*
     * This is fixing some of the default configuration of the XIVE
     * devices. To be called after the reset of the machine devices.
     */
    spapr_irq_reset(spapr, &error_fatal);

I guess this is enough to prevent someone to break things.

> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks,
> 
> C.
> 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/ppc/spapr.c             |    4 ----
> >  hw/ppc/spapr_irq.c         |    7 ++-----
> >  include/hw/ppc/spapr_irq.h |    1 -
> >  3 files changed, 2 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 5894329f29a9..855e9fbd9805 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1739,10 +1739,6 @@ static void spapr_machine_reset(MachineState *machine)
> >          ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
> >      }
> >  
> > -    if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
> > -        spapr_irq_msi_reset(spapr);
> > -    }
> > -
> >      /*
> >       * NVLink2-connected GPU RAM needs to be placed on a separate NUMA node.
> >       * We assign a new numa ID per GPU in spapr_pci_collect_nvgpu() which is
> > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> > index d07aed8ca9f9..c72d8433681d 100644
> > --- a/hw/ppc/spapr_irq.c
> > +++ b/hw/ppc/spapr_irq.c
> > @@ -57,11 +57,6 @@ void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num)
> >      bitmap_clear(spapr->irq_map, irq - SPAPR_IRQ_MSI, num);
> >  }
> >  
> > -void spapr_irq_msi_reset(SpaprMachineState *spapr)
> > -{
> > -    bitmap_clear(spapr->irq_map, 0, spapr->irq_map_nr);
> > -}
> > -
> >  static void spapr_irq_init_kvm(SpaprMachineState *spapr,
> >                                    SpaprIrq *irq, Error **errp)
> >  {
> > @@ -729,6 +724,8 @@ int spapr_irq_post_load(SpaprMachineState *spapr, int version_id)
> >  
> >  void spapr_irq_reset(SpaprMachineState *spapr, Error **errp)
> >  {
> > +    assert(bitmap_empty(spapr->irq_map, spapr->irq_map_nr));
> > +
> >      if (spapr->irq->reset) {
> >          spapr->irq->reset(spapr, errp);
> >      }
> > diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> > index f965a58f8954..44fe4f9e0e2e 100644
> > --- a/include/hw/ppc/spapr_irq.h
> > +++ b/include/hw/ppc/spapr_irq.h
> > @@ -28,7 +28,6 @@ void spapr_irq_msi_init(SpaprMachineState *spapr, uint32_t nr_msis);
> >  int spapr_irq_msi_alloc(SpaprMachineState *spapr, uint32_t num, bool align,
> >                          Error **errp);
> >  void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num);
> > -void spapr_irq_msi_reset(SpaprMachineState *spapr);
> >  
> >  typedef struct SpaprIrq {
> >      uint32_t    nr_irqs;
> > 
> 

Re: [Qemu-devel] [PATCH 3/3] spapr/irq: Drop spapr_irq_msi_reset()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2873 bytes --]

On Fri, Jul 26, 2019 at 04:44:49PM +0200, Greg Kurz wrote:
> PHBs already take care of clearing the MSIs from the bitmap during reset
> or unplug. No need to do this globally from the machine code. Rather add
> an assert to ensure that PHBs have acted as expected.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-4.2, thanks.

> ---
>  hw/ppc/spapr.c             |    4 ----
>  hw/ppc/spapr_irq.c         |    7 ++-----
>  include/hw/ppc/spapr_irq.h |    1 -
>  3 files changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5894329f29a9..855e9fbd9805 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1739,10 +1739,6 @@ static void spapr_machine_reset(MachineState *machine)
>          ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
>      }
>  
> -    if (!SPAPR_MACHINE_GET_CLASS(spapr)->legacy_irq_allocation) {
> -        spapr_irq_msi_reset(spapr);
> -    }
> -
>      /*
>       * NVLink2-connected GPU RAM needs to be placed on a separate NUMA node.
>       * We assign a new numa ID per GPU in spapr_pci_collect_nvgpu() which is
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index d07aed8ca9f9..c72d8433681d 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -57,11 +57,6 @@ void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num)
>      bitmap_clear(spapr->irq_map, irq - SPAPR_IRQ_MSI, num);
>  }
>  
> -void spapr_irq_msi_reset(SpaprMachineState *spapr)
> -{
> -    bitmap_clear(spapr->irq_map, 0, spapr->irq_map_nr);
> -}
> -
>  static void spapr_irq_init_kvm(SpaprMachineState *spapr,
>                                    SpaprIrq *irq, Error **errp)
>  {
> @@ -729,6 +724,8 @@ int spapr_irq_post_load(SpaprMachineState *spapr, int version_id)
>  
>  void spapr_irq_reset(SpaprMachineState *spapr, Error **errp)
>  {
> +    assert(bitmap_empty(spapr->irq_map, spapr->irq_map_nr));
> +
>      if (spapr->irq->reset) {
>          spapr->irq->reset(spapr, errp);
>      }
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index f965a58f8954..44fe4f9e0e2e 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -28,7 +28,6 @@ void spapr_irq_msi_init(SpaprMachineState *spapr, uint32_t nr_msis);
>  int spapr_irq_msi_alloc(SpaprMachineState *spapr, uint32_t num, bool align,
>                          Error **errp);
>  void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num);
> -void spapr_irq_msi_reset(SpaprMachineState *spapr);
>  
>  typedef struct SpaprIrq {
>      uint32_t    nr_irqs;
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 0/3] spapr/pci: Improve MSI tracking

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 726 bytes --]

On Fri, Jul 26, 2019 at 04:44:33PM +0200, Greg Kurz wrote:
> Some recent tests with AIX guests showed that we don't tear down
> MSIs that were allocated with the "change-msi" RTAS call, when
> the guest is rebooted. This series teach PHBs to do the cleanup
> at reset time.
> 
> This bug has always been there. Not sure it is worth the pain to
> have this fixed in 4.1.

Since we've only recently started looking at AIX as a supported guest,
and these bugs don't appear to break Linux guests, no, I don't think
it is.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]