On 11/14/2017 09:42 AM, Greg Kurz wrote:
> On Fri, 10 Nov 2017 15:20:11 +0000
> Cédric Le Goater <c...@kaod.org> wrote:
>
>> Let's define a new set of XICSFabric IRQ operations for the latest
>> pseries machine. These simply use a a bitmap 'irq_map' as a IRQ number
>> allocator.
>>
>> The previous pseries machines keep the old set of IRQ operations using
>> the ICSIRQState array.
>>
>> Signed-off-by: Cédric Le Goater <c...@kaod.org>
>> ---
>>
>> Changes since v2 :
>>
>> - introduced a second set of XICSFabric IRQ operations for older
>> pseries machines
>>
>> hw/ppc/spapr.c | 76
>> ++++++++++++++++++++++++++++++++++++++++++++++----
>> include/hw/ppc/spapr.h | 3 ++
>> 2 files changed, 74 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 4bdceb45a14f..4ef0b73559ca 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1681,6 +1681,22 @@ static const VMStateDescription
>> vmstate_spapr_patb_entry = {
>> },
>> };
>>
>> +static bool spapr_irq_map_needed(void *opaque)
>> +{
>> + return true;
>
> I see that the next patch adds some code to avoid sending the
> bitmap if it doesn't contain state, but I guess you should also
> explicitly have this function to return false for older machine
> types (see remark below).
>
>> +}
>> +
>> +static const VMStateDescription vmstate_spapr_irq_map = {
>> + .name = "spapr_irq_map",
>> + .version_id = 0,
>> + .minimum_version_id = 0,
>> + .needed = spapr_irq_map_needed,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_BITMAP(irq_map, sPAPRMachineState, 0, nr_irqs),
>> + VMSTATE_END_OF_LIST()
>> + },
>> +};
>> +
>> static const VMStateDescription vmstate_spapr = {
>> .name = "spapr",
>> .version_id = 3,
>> @@ -1700,6 +1716,7 @@ static const VMStateDescription vmstate_spapr = {
>> &vmstate_spapr_ov5_cas,
>> &vmstate_spapr_patb_entry,
>> &vmstate_spapr_pending_events,
>> + &vmstate_spapr_irq_map,
>> NULL
>> }
>> };
>> @@ -2337,8 +2354,12 @@ static void ppc_spapr_init(MachineState *machine)
>> /* Setup a load limit for the ramdisk leaving room for SLOF and FDT */
>> load_limit = MIN(spapr->rma_size, RTAS_MAX_ADDR) - FW_OVERHEAD;
>>
>> + /* Initialize the IRQ allocator */
>> + spapr->nr_irqs = XICS_IRQS_SPAPR;
>> + spapr->irq_map = bitmap_new(spapr->nr_irqs);
>> +
>
> I think you should introduce a sPAPRMachineClass::has_irq_bitmap boolean
> so that the bitmap is only allocated for newer machine types. And you should
> then use this flag in spapr_irq_map_needed() above.
yes. I can add a boot to be more explicit on the use of the bitmap.
Thanks,
C.
>
> Apart from that, the rest of the patch looks good.
>
>> /* Set up Interrupt Controller before we create the VCPUs */
>> - xics_system_init(machine, XICS_IRQS_SPAPR, &error_fatal);
>> + xics_system_init(machine, spapr->nr_irqs, &error_fatal);
>>
>> /* Set up containers for ibm,client-architecture-support negotiated
>> options
>> */
>> @@ -3560,7 +3581,7 @@ static int ics_find_free_block(ICSState *ics, int num,
>> int alignnum)
>> return -1;
>> }
>>
>> -static bool spapr_irq_test(XICSFabric *xi, int irq)
>> +static bool spapr_irq_test_2_11(XICSFabric *xi, int irq)
>> {
>> sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
>> ICSState *ics = spapr->ics;
>> @@ -3569,7 +3590,7 @@ static bool spapr_irq_test(XICSFabric *xi, int irq)
>> return !ICS_IRQ_FREE(ics, srcno);
>> }
>>
>> -static int spapr_irq_alloc_block(XICSFabric *xi, int count, int align)
>> +static int spapr_irq_alloc_block_2_11(XICSFabric *xi, int count, int align)
>> {
>> sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
>> ICSState *ics = spapr->ics;
>> @@ -3583,7 +3604,7 @@ static int spapr_irq_alloc_block(XICSFabric *xi, int
>> count, int align)
>> return srcno + ics->offset;
>> }
>>
>> -static void spapr_irq_free_block(XICSFabric *xi, int irq, int num)
>> +static void spapr_irq_free_block_2_11(XICSFabric *xi, int irq, int num)
>> {
>> sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
>> ICSState *ics = spapr->ics;
>> @@ -3601,6 +3622,46 @@ static void spapr_irq_free_block(XICSFabric *xi, int
>> irq, int num)
>> }
>> }
>>
>> +static bool spapr_irq_test(XICSFabric *xi, int irq)
>> +{
>> + sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
>> + int srcno = irq - spapr->ics->offset;
>> +
>> + return test_bit(srcno, spapr->irq_map);
>> +}
>> +
>> +static int spapr_irq_alloc_block(XICSFabric *xi, int count, int align)
>> +{
>> + sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
>> + int start = 0;
>> + int srcno;
>> +
>> + /*
>> + * The 'align_mask' parameter of bitmap_find_next_zero_area()
>> + * should be one less than a power of 2; 0 means no
>> + * alignment. Adapt the 'align' value of the former allocator to
>> + * fit the requirements of bitmap_find_next_zero_area()
>> + */
>> + align -= 1;
>> +
>> + srcno = bitmap_find_next_zero_area(spapr->irq_map, spapr->nr_irqs,
>> start,
>> + count, align);
>> + if (srcno == spapr->nr_irqs) {
>> + return -1;
>> + }
>> +
>> + bitmap_set(spapr->irq_map, srcno, count);
>> + return srcno + spapr->ics->offset;
>> +}
>> +
>> +static void spapr_irq_free_block(XICSFabric *xi, int irq, int num)
>> +{
>> + sPAPRMachineState *spapr = SPAPR_MACHINE(xi);
>> + int srcno = irq - spapr->ics->offset;
>> +
>> + bitmap_clear(spapr->irq_map, srcno, num);
>> +}
>> +
>> static void spapr_pic_print_info(InterruptStatsProvider *obj,
>> Monitor *mon)
>> {
>> @@ -3778,7 +3839,12 @@ static void
>> spapr_machine_2_11_instance_options(MachineState *machine)
>>
>> static void spapr_machine_2_11_class_options(MachineClass *mc)
>> {
>> - /* Defaults for the latest behaviour inherited from the base class */
>> + XICSFabricClass *xic = XICS_FABRIC_CLASS(mc);
>> +
>> + spapr_machine_2_12_class_options(mc);
>> + xic->irq_test = spapr_irq_test_2_11;
>> + xic->irq_alloc_block = spapr_irq_alloc_block_2_11;
>> + xic->irq_free_block = spapr_irq_free_block_2_11;
>> }
>>
>> DEFINE_SPAPR_MACHINE(2_11, "2.11", false);
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 9d21ca9bde3a..5835c694caff 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -7,6 +7,7 @@
>> #include "hw/ppc/spapr_drc.h"
>> #include "hw/mem/pc-dimm.h"
>> #include "hw/ppc/spapr_ovec.h"
>> +#include "qemu/bitmap.h"
>>
>> struct VIOsPAPRBus;
>> struct sPAPRPHBState;
>> @@ -78,6 +79,8 @@ struct sPAPRMachineState {
>> struct VIOsPAPRBus *vio_bus;
>> QLIST_HEAD(, sPAPRPHBState) phbs;
>> struct sPAPRNVRAM *nvram;
>> + int32_t nr_irqs;
>> + unsigned long *irq_map;
>> ICSState *ics;
>> sPAPRRTCState rtc;
>>
>
