Re: [Qemu-devel] [PATCH 6/8] spapr: move interrupt allocator to xics

[Alexander Graf <agraf@suse.de>]

On 14.03.14 05:18, Alexey Kardashevskiy wrote:
> The current allocator returns IRQ numbers from a pool and does not
> support IRQs reuse in any form as it did not keep track of what it
> previously returned, it only had the last returned IRQ.
> However migration may change interrupts for devices depending on
> their order in the command line.

Wtf? Nonono, this sounds very bogus and wrong. Migration shouldn't 
change anything.


Alex

> This moves an allocator from SPAPR to XICS.
>
> This switches IRQ users to use new API.
>
> This uses LSI/MSI flags to know if interrupt is in use.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>   hw/intc/xics.c         | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   hw/ppc/spapr.c         | 67 ------------------------------------------
>   hw/ppc/spapr_events.c  |  2 +-
>   hw/ppc/spapr_pci.c     |  6 ++--
>   hw/ppc/spapr_vio.c     |  2 +-
>   include/hw/ppc/spapr.h | 10 -------
>   include/hw/ppc/xics.h  |  2 ++
>   trace-events           |  3 ++
>   8 files changed, 90 insertions(+), 82 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index e5195bf..8d101a3 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -690,6 +690,86 @@ void xics_set_irq_type(XICSState *icp, int irq, bool lsi)
>       ics_set_irq_type(&icp->ics[server], irq, lsi);
>   }
>   
> +#define XICS_IRQ_FREE(ics, n)   \
> +    (!((ics)->irqs[(n) - (ics)->offset].flags & \
> +       (XICS_FLAGS_LSI | XICS_FLAGS_MSI)))
> +
> +static int ics_find_free_block(ICSState *ics, int num, int alignnum)
> +{
> +    int first, i;
> +
> +    for (first = 0; first < ics->nr_irqs; first += alignnum) {
> +        if (num > (ics->nr_irqs - first)) {
> +            return -1;
> +        }
> +        for (i = first; i < first + num; ++i) {
> +            if (!XICS_IRQ_FREE(ics, i + ics->offset)) {
> +                break;
> +            }
> +        }
> +        if (i == (first + num)) {
> +            return first + ics->offset;
> +        }
> +    }
> +
> +    return -1;
> +}
> +
> +int xics_alloc(XICSState *icp, int server, int irq, bool lsi)
> +{
> +    ICSState *ics = &icp->ics[server];
> +
> +    if (irq) {
> +        assert(server == xics_find_server(icp, irq));
> +        if (!XICS_IRQ_FREE(ics, irq)) {
> +            trace_xics_alloc_failed(server, irq);
> +            return -1;
> +        }
> +    } else {
> +        irq = ics_find_free_block(ics, 1, 1);
> +    }
> +
> +    ics_set_irq_type(ics, irq, lsi);
> +    trace_xics_alloc(server, irq);
> +
> +    return irq;
> +}
> +
> +/*
> + * Allocate block of consequtive IRQs, returns a number of the first.
> + * If align==true, aligns the first IRQ number to num.
> + */
> +int xics_alloc_block(XICSState *icp, int server, int num, bool lsi, bool align)
> +{
> +    int i, first = -1;
> +    ICSState *ics = &icp->ics[server];
> +
> +    assert(server == 0);
> +    /*
> +     * 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) {
> +        for (i = first; i < first + num; ++i) {
> +            ics_set_irq_type(ics, i, lsi);
> +        }
> +    }
> +    trace_xics_alloc_block(server, first, num, lsi, align);
> +
> +    return first;
> +}
> +
>   /*
>    * Guest interfaces
>    */
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 12adc21..29ca2e0 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -83,73 +83,6 @@
>   
>   sPAPREnvironment *spapr;
>   
> -int spapr_allocate_irq(int hint, bool lsi)
> -{
> -    int irq;
> -
> -    if (hint) {
> -        irq = hint;
> -        if (hint >= spapr->next_irq) {
> -            spapr->next_irq = hint + 1;
> -        }
> -        /* FIXME: we should probably check for collisions somehow */
> -    } else {
> -        irq = spapr->next_irq++;
> -    }
> -
> -    /* Configure irq type */
> -    if (!xics_get_qirq(spapr->icp, irq)) {
> -        return 0;
> -    }
> -
> -    xics_set_irq_type(spapr->icp, irq, lsi);
> -
> -    return irq;
> -}
> -
> -/*
> - * Allocate block of consequtive IRQs, returns a number of the first.
> - * If msi==true, aligns the first IRQ number to num.
> - */
> -int spapr_allocate_irq_block(int num, bool lsi, bool msi)
> -{
> -    int first = -1;
> -    int i, hint = 0;
> -
> -    /*
> -     * 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 (msi) {
> -        assert((num == 1) || (num == 2) || (num == 4) ||
> -               (num == 8) || (num == 16) || (num == 32));
> -        hint = (spapr->next_irq + num - 1) & ~(num - 1);
> -    }
> -
> -    for (i = 0; i < num; ++i) {
> -        int irq;
> -
> -        irq = spapr_allocate_irq(hint, lsi);
> -        if (!irq) {
> -            return -1;
> -        }
> -
> -        if (0 == i) {
> -            first = irq;
> -            hint = 0;
> -        }
> -
> -        /* If the above doesn't create a consecutive block then that's
> -         * an internal bug */
> -        assert(irq == (first + i));
> -    }
> -
> -    return first;
> -}
> -
>   static XICSState *try_create_xics(const char *type, int nr_servers,
>                                     int nr_irqs)
>   {
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 16fa49e..e605430 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -314,7 +314,7 @@ static void check_exception(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>   
>   void spapr_events_init(sPAPREnvironment *spapr)
>   {
> -    spapr->epow_irq = spapr_allocate_msi(0);
> +    spapr->epow_irq = xics_alloc_block(spapr->icp, 0, 1, false, false);
>       spapr->epow_notifier.notify = spapr_powerdown_req;
>       qemu_register_powerdown_notifier(&spapr->epow_notifier);
>       spapr_rtas_register("check-exception", check_exception);
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index cbef095..4eaf364 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -343,8 +343,8 @@ static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>   
>       /* There is no cached config, allocate MSIs */
>       if (!phb->msi_table[ndev].nvec) {
> -        irq = spapr_allocate_irq_block(req_num, false,
> -                                       ret_intr_type == RTAS_TYPE_MSI);
> +        irq = xics_alloc_block(spapr->icp, 0, req_num, false,
> +                               ret_intr_type == RTAS_TYPE_MSI);
>           if (irq < 0) {
>               error_report("Cannot allocate MSIs for device#%d", ndev);
>               rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> @@ -623,7 +623,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>       for (i = 0; i < PCI_NUM_PINS; i++) {
>           uint32_t irq;
>   
> -        irq = spapr_allocate_lsi(0);
> +        irq = xics_alloc_block(spapr->icp, 0, 1, true, false);
>           if (!irq) {
>               error_setg(errp, "spapr_allocate_lsi failed");
>               return;
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index 4e33f46..8aeb263 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -448,7 +448,7 @@ static int spapr_vio_busdev_init(DeviceState *qdev)
>           dev->qdev.id = id;
>       }
>   
> -    dev->irq = spapr_allocate_msi(dev->irq);
> +    dev->irq = xics_alloc(spapr->icp, 0, dev->irq, false);
>       if (!dev->irq) {
>           return -1;
>       }
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 18332fd..5d577ff 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -323,16 +323,6 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu, target_ulong opcode,
>   int spapr_allocate_irq(int hint, bool lsi);
>   int spapr_allocate_irq_block(int num, bool lsi, bool msi);
>   
> -static inline int spapr_allocate_msi(int hint)
> -{
> -    return spapr_allocate_irq(hint, false);
> -}
> -
> -static inline int spapr_allocate_lsi(int hint)
> -{
> -    return spapr_allocate_irq(hint, true);
> -}
> -
>   /* RTAS return codes */
>   #define RTAS_OUT_SUCCESS            0
>   #define RTAS_OUT_NO_ERRORS_FOUND    1
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 4b30e9a..337398d 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -157,6 +157,8 @@ struct ICSIRQState {
>   
>   qemu_irq xics_get_qirq(XICSState *icp, int irq);
>   void xics_set_irq_type(XICSState *icp, int irq, bool lsi);
> +int xics_alloc(XICSState *icp, int server, int irq, bool lsi);
> +int xics_alloc_block(XICSState *icp, int server, int num, bool lsi, bool align);
>   
>   void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu);
>   
> diff --git a/trace-events b/trace-events
> index 002c260..ad7400e 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1143,6 +1143,9 @@ xics_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]"
>   xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
>   xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]"
>   xics_ics_eoi(int nr) "ics_eoi: irq %#x"
> +xics_alloc(int server, int irq) "server#%d, irq %d"
> +xics_alloc_failed(int server, int irq) "server#%d, irq %d"
> +xics_alloc_block(int server, int first, int num, bool lsi, int align) "server#%d, first irq %d, %d irqs, lsi=%d, alignnum %d"
>   
>   # hw/ppc/spapr_iommu.c
>   spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64