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

[David Gibson]

On Mon, Jun 18, 2018 at 02:00:06PM +1000, David Gibson wrote:
> On Fri, Jun 15, 2018 at 01:53:01PM +0200, Cédric Le Goater 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);
> 
> Unlike find, there's no reason we need to atomically claim a block of
> irqs.  So I think claim should just grab a single irq.  Callers can
> loop if they need multiple irqs.

Actually.. I take that back.  We don't *need* a block-claim interface,
but if it's convenient there's no strong reason not to (as long as
therer aren't *both* single and block interfaces, which there aren't).

So feel free to run with that, once the rollback bugs Greg pointed out
are fixed.


> Other than that and the minor points Greg noted, this looks good.
> 
> >  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)) {
> > +        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));
> > +        }
> > +    }
> > +
> > +    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;
> 



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

signature.asc
Description: PGP signature