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

[Greg Kurz <groug@kaod.org>]

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