Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/3] spapr: split the IRQ allocation sequence

[Greg Kurz]

On Fri, 15 Jun 2018 19:21:19 +0200
Cédric Le Goater <address@hidden> wrote:

> On 06/15/2018 03:14 PM, Greg Kurz wrote:
> > On Fri, 15 Jun 2018 13:53:01 +0200
> > Cédric Le Goater <address@hidden> wrote:
> >   
> >> Today, when a device requests for IRQ number in a sPAPR machine, the
> >> spapr_irq_alloc() routine first scans the ICSState status array to
> >> find an empty slot and then performs the assignement of the selected
> >> numbers. Split this sequence in two distinct routines : spapr_irq_find()
> >> for lookups and spapr_irq_claim() for claiming the IRQ numbers.
> >>
> >> This will ease the introduction of a static layout of IRQ numbers.
> >>
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> >>  include/hw/ppc/spapr.h |  5 ++++
> >>  hw/ppc/spapr.c         | 64 
> >> ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  hw/ppc/spapr_events.c  | 18 ++++++++++----
> >>  hw/ppc/spapr_pci.c     | 19 ++++++++++++---
> >>  hw/ppc/spapr_vio.c     | 10 +++++++-
> >>  5 files changed, 108 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >> index 3388750fc795..6088f44c1b2a 100644
> >> --- a/include/hw/ppc/spapr.h
> >> +++ b/include/hw/ppc/spapr.h
> >> @@ -776,6 +776,11 @@ int spapr_irq_alloc(sPAPRMachineState *spapr, int 
> >> irq_hint, bool lsi,
> >>                      Error **errp);
> >>  int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
> >>                            bool align, Error **errp);
> >> +int spapr_irq_find(sPAPRMachineState *spapr, int num, bool align,
> >> +                   Error **errp);
> >> +#define spapr_irq_findone(spapr, errp) spapr_irq_find(spapr, 1, false, 
> >> errp)
> >> +int spapr_irq_claim(sPAPRMachineState *spapr, int irq, int num, bool lsi,
> >> +                    Error **errp);
> >>  void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num);
> >>  qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
> >>  
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index f59999daacfc..b1d19b328166 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -3810,6 +3810,36 @@ static int ics_find_free_block(ICSState *ics, int 
> >> num, int alignnum)
> >>      return -1;
> >>  }
> >>  
> >> +int spapr_irq_find(sPAPRMachineState *spapr, int num, bool align, Error 
> >> **errp)
> >> +{
> >> +    ICSState *ics = spapr->ics;
> >> +    int first = -1;
> >> +
> >> +    assert(ics);
> >> +
> >> +    /*
> >> +     * MSIMesage::data is used for storing VIRQ so
> >> +     * it has to be aligned to num to support multiple
> >> +     * MSI vectors. MSI-X is not affected by this.
> >> +     * The hint is used for the first IRQ, the rest should
> >> +     * be allocated continuously.
> >> +     */
> >> +    if (align) {
> >> +        assert((num == 1) || (num == 2) || (num == 4) ||
> >> +               (num == 8) || (num == 16) || (num == 32));
> >> +        first = ics_find_free_block(ics, num, num);
> >> +    } else {
> >> +        first = ics_find_free_block(ics, num, 1);
> >> +    }
> >> +
> >> +    if (first < 0) {
> >> +        error_setg(errp, "can't find a free %d-IRQ block", num);
> >> +        return -1;
> >> +    }
> >> +
> >> +    return first + ics->offset;
> >> +}
> >> +
> >>  /*
> >>   * Allocate the IRQ number and set the IRQ type, LSI or MSI
> >>   */
> >> @@ -3888,6 +3918,40 @@ int spapr_irq_alloc_block(sPAPRMachineState *spapr, 
> >> int num, bool lsi,
> >>      return first;
> >>  }
> >>  
> >> +int spapr_irq_claim(sPAPRMachineState *spapr, int irq, int num, bool lsi,
> >> +                    Error **errp)
> >> +{
> >> +    ICSState *ics = spapr->ics;
> >> +    int i;
> >> +    int srcno = irq - ics->offset;
> >> +    int ret = 0;
> >> +
> >> +    assert(ics);
> >> +
> >> +    if (!ics_valid_irq(ics, irq) || !ics_valid_irq(ics, irq + num)) {  
> > 
> > I guess it is okay to assume that if the first and last irqs are valid,
> > so are all numbers in between. Shouldn't the second check be against
> > irq + num - 1 though ?  
> 
> yes. thanks.
> 
> > This lacks an error_setg() BTW.
> >   
> >> +        return -1;
> >> +    }
> >> +
> >> +    for (i = srcno; i < srcno + num; ++i) {
> >> +        if (ICS_IRQ_FREE(ics, i)) {
> >> +            spapr_irq_set_lsi(spapr, i + ics->offset, lsi);
> >> +        } else {
> >> +            error_setg(errp, "IRQ %d is not free", i + ics->offset);
> >> +            ret = -1;
> >> +            break;
> >> +        }
> >> +    }
> >> +
> >> +    /* we could call spapr_irq_free() when rolling back */
> >> +    if (ret) {
> >> +        while (--i >= srcno) {
> >> +            memset(&ics->irqs[i], 0, sizeof(ICSIRQState));
> >> +        }
> >> +    }  
> > 
> > Hmm... I guess we should free the whole range otherwise we leak
> > srcno + num - i irqs, preferably using spapr_irq_free() to match
> > the traces from spapr_irq_alloc().  
> 
> I don't understand. This is undoing what has been done. 
> 

Oops sorry... I got confused by the suggestion to use spapr_irq_free().
So, indeed, rollback is okay. Only thing to address would be to have
matching traces in spapr_irq_claim() and spapr_irq_free() I guess.

> > Alternatively, the rollback could be pushed to the callers.  
> 
> Yes. Today none is done so we might just do nothing about it
> and return an error.
>  
> > Rest looks good.
> >   
> >> +
> >> +    return ret;
> >> +}
> >> +
> >>  void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num)
> >>  {
> >>      ICSState *ics = spapr->ics;
> >> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> >> index 86836f0626dc..3b6ae7272092 100644
> >> --- a/hw/ppc/spapr_events.c
> >> +++ b/hw/ppc/spapr_events.c
> >> @@ -707,13 +707,18 @@ void spapr_clear_pending_events(sPAPRMachineState 
> >> *spapr)
> >>  
> >>  void spapr_events_init(sPAPRMachineState *spapr)
> >>  {
> >> +    int epow_irq;
> >> +
> >> +    epow_irq = spapr_irq_findone(spapr, &error_fatal);
> >> +
> >> +    spapr_irq_claim(spapr, epow_irq, 1, false, &error_fatal);
> >> +
> >>      QTAILQ_INIT(&spapr->pending_events);
> >>  
> >>      spapr->event_sources = spapr_event_sources_new();
> >>  
> >>      spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_EPOW,
> >> -                                 spapr_irq_alloc(spapr, 0, false,
> >> -                                                  &error_fatal));
> >> +                                 epow_irq);
> >>  
> >>      /* NOTE: if machine supports modern/dedicated hotplug event source,
> >>       * we add it to the device-tree unconditionally. This means we may
> >> @@ -724,9 +729,14 @@ void spapr_events_init(sPAPRMachineState *spapr)
> >>       * checking that it's enabled.
> >>       */
> >>      if (spapr->use_hotplug_event_source) {
> >> +        int hp_irq;
> >> +
> >> +        hp_irq = spapr_irq_findone(spapr, &error_fatal);
> >> +
> >> +        spapr_irq_claim(spapr, hp_irq, 1, false, &error_fatal);
> >> +
> >>          spapr_event_sources_register(spapr->event_sources, 
> >> EVENT_CLASS_HOT_PLUG,
> >> -                                     spapr_irq_alloc(spapr, 0, false,
> >> -                                                      &error_fatal));
> >> +                                     hp_irq);
> >>      }
> >>  
> >>      spapr->epow_notifier.notify = spapr_powerdown_req;
> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> >> index f936ce63effa..7394c62b4a8b 100644
> >> --- a/hw/ppc/spapr_pci.c
> >> +++ b/hw/ppc/spapr_pci.c
> >> @@ -371,8 +371,14 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, 
> >> sPAPRMachineState *spapr,
> >>      }
> >>  
> >>      /* Allocate MSIs */
> >> -    irq = spapr_irq_alloc_block(spapr, req_num, false,
> >> -                           ret_intr_type == RTAS_TYPE_MSI, &err);
> >> +    irq = spapr_irq_find(spapr, req_num, ret_intr_type == RTAS_TYPE_MSI, 
> >> &err);
> >> +    if (err) {
> >> +        error_reportf_err(err, "Can't allocate MSIs for device %x: ",
> >> +                          config_addr);
> >> +        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> >> +        return;
> >> +    }
> >> +    spapr_irq_claim(spapr, irq, req_num, false, &err);
> >>      if (err) {
> >>          error_reportf_err(err, "Can't allocate MSIs for device %x: ",
> >>                            config_addr);
> >> @@ -1698,7 +1704,14 @@ static void spapr_phb_realize(DeviceState *dev, 
> >> Error **errp)
> >>          uint32_t irq;
> >>          Error *local_err = NULL;
> >>  
> >> -        irq = spapr_irq_alloc_block(spapr, 1, true, false, &local_err);
> >> +        irq = spapr_irq_findone(spapr, &local_err);
> >> +        if (local_err) {
> >> +            error_propagate(errp, local_err);
> >> +            error_prepend(errp, "can't allocate LSIs: ");
> >> +            return;
> >> +        }
> >> +
> >> +        spapr_irq_claim(spapr, irq, 1, true, &local_err);
> >>          if (local_err) {
> >>              error_propagate(errp, local_err);
> >>              error_prepend(errp, "can't allocate LSIs: ");
> >> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> >> index 4555c648a8e2..ad9b56e28447 100644
> >> --- a/hw/ppc/spapr_vio.c
> >> +++ b/hw/ppc/spapr_vio.c
> >> @@ -475,7 +475,15 @@ static void spapr_vio_busdev_realize(DeviceState 
> >> *qdev, Error **errp)
> >>          dev->qdev.id = id;
> >>      }
> >>  
> >> -    dev->irq = spapr_irq_alloc(spapr, dev->irq, false, &local_err);
> >> +    if (!dev->irq) {
> >> +        dev->irq = spapr_irq_findone(spapr, &local_err);
> >> +        if (local_err) {
> >> +            error_propagate(errp, local_err);
> >> +            return;
> >> +        }
> >> +    }
> >> +
> >> +    spapr_irq_claim(spapr, dev->irq, 1, false, &local_err);
> >>      if (local_err) {
> >>          error_propagate(errp, local_err);
> >>          return;  
> >   
>