On 06/15/2018 03:14 PM, Greg Kurz wrote: > On Fri, 15 Jun 2018 13:53:01 +0200 > Cédric Le Goater <c...@kaod.org> 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 <c...@kaod.org> >> --- >> 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. > 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; >