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

[David Gibson <david@gibson.dropbear.id.au>]

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

On Mon, Jun 18, 2018 at 07:34:00PM +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 <clg@kaod.org>

Applied to ppc-for-3.0, thanks.

> ---
>  include/hw/ppc/spapr.h |  4 ++++
>  hw/ppc/spapr.c         | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/ppc/spapr_events.c  | 18 ++++++++++++++----
>  hw/ppc/spapr_pci.c     | 23 ++++++++++++++++++++---
>  hw/ppc/spapr_vio.c     | 10 +++++++++-
>  5 files changed, 97 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 3388750fc795..15f771acc892 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -776,6 +776,10 @@ 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, 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 db0fb385d4e0..1fa398111111 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3816,6 +3816,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
>   */
> @@ -3894,6 +3924,26 @@ int spapr_irq_alloc_block(sPAPRMachineState *spapr, int num, bool lsi,
>      return first;
>  }
>  
> +int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp)
> +{
> +    ICSState *ics = spapr->ics;
> +
> +    assert(ics);
> +
> +    if (!ics_valid_irq(ics, irq)) {
> +        error_setg(errp, "IRQ %d is invalid", irq);
> +        return -1;
> +    }
> +
> +    if (!ICS_IRQ_FREE(ics, irq - ics->offset)) {
> +        error_setg(errp, "IRQ %d is not free", irq);
> +        return -1;
> +    }
> +
> +    spapr_irq_set_lsi(spapr, irq, lsi);
> +    return 0;
> +}
> +
>  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..e4f5946a2188 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, 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, 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..497b896c7d24 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -279,6 +279,7 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      spapr_pci_msi *msi;
>      int *config_addr_key;
>      Error *err = NULL;
> +    int i;
>  
>      /* Fins sPAPRPHBState */
>      phb = spapr_pci_find_phb(spapr, buid);
> @@ -371,8 +372,7 @@ 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);
> @@ -380,6 +380,16 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>          return;
>      }
>  
> +    for (i = 0; i < req_num; i++) {
> +        spapr_irq_claim(spapr, irq + i, false, &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;
> +        }
> +    }
> +
>      /* Release previous MSIs */
>      if (msi) {
>          spapr_irq_free(spapr, msi->first_irq, msi->num);
> @@ -1698,7 +1708,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, 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..daf85130b5ef 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, 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

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