1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH 0/3] spapr/pci: Improve MSI tracking
  4. View patch

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

Greg Kurz posted 3 patches 6 years, 6 months ago
Maintainers: David Gibson <david@gibson.dropbear.id.au>
[Qemu-devel] [PATCH 3/3] spapr/irq: Drop spapr_irq_msi_reset()
Posted by Greg Kurz 6 years, 6 months ago 
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()
Posted by Cédric Le Goater 6 years, 6 months ago 
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()
Posted by Greg Kurz 6 years, 6 months ago 
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()
Posted by David Gibson 6 years, 6 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.