Re: [Qemu-devel] [PATCH 17/25] spapr: add a sPAPRXive object to the machine

[Cédric Le Goater]

On 11/30/2017 05:55 AM, David Gibson wrote:
> On Thu, Nov 23, 2017 at 02:29:47PM +0100, Cédric Le Goater wrote:
>> The XIVE object is designed to be always available, so it is created
>> unconditionally on newer machines.
> 
> There doesn't actually seem to be anything dependent on machine
> version here.

No. I thought that was too early in the patchset. This is handled 
in the last patch with a 'xive_exploitation' bool which is set to 
false on older machines. 

But, nevertheless, the XIVE objects are always created even if not
used. Something to discuss. 

>> Depending on the configuration and
>> the guest capabilities, the CAS negotiation process will decide which
>> interrupt model to use, legacy or XIVE.
>>
>> The XIVE model makes use of the full range of the IRQ number space
>> because the IRQ numbers for the CPU IPIs are allocated in the range
>> below XICS_IRQ_BASE, which is unused by XICS.
> 
> Ok.  And I take it 4096 is enough space for the XIVE IPIs for the
> forseeable future?

The biggest real system I am aware of as 16 sockets, 192 cores, SMT8. 
That's 1536 cpus. pseries has a max_cpus of 1024. 

>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>  hw/ppc/spapr.c         | 34 ++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/spapr.h |  2 ++
>>  2 files changed, 36 insertions(+)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 5d3325ca3c88..0e0107c8272c 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -56,6 +56,7 @@
>>  #include "hw/ppc/spapr_vio.h"
>>  #include "hw/pci-host/spapr.h"
>>  #include "hw/ppc/xics.h"
>> +#include "hw/ppc/spapr_xive.h"
>>  #include "hw/pci/msi.h"
>>  
>>  #include "hw/pci/pci.h"
>> @@ -204,6 +205,29 @@ static void xics_system_init(MachineState *machine, int 
>> nr_irqs, Error **errp)
>>      }
>>  }
>>  
>> +static sPAPRXive *spapr_xive_create(sPAPRMachineState *spapr, int nr_irqs,
>> +                                    Error **errp)
>> +{
>> +    Error *local_err = NULL;
>> +    Object *obj;
>> +
>> +    obj = object_new(TYPE_SPAPR_XIVE);
>> +    object_property_add_child(OBJECT(spapr), "xive", obj, &error_abort);
>> +    object_property_set_int(obj, nr_irqs, "nr-irqs",  &local_err);
>> +    if (local_err) {
>> +        goto error;
>> +    }
>> +    object_property_set_bool(obj, true, "realized", &local_err);
>> +    if (local_err) {
>> +        goto error;
>> +    }
>> +
>> +    return SPAPR_XIVE(obj);
>> +error:
>> +    error_propagate(errp, local_err);
>> +    return NULL;
>> +}
>> +
>>  static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, PowerPCCPU *cpu,
>>                                    int smt_threads)
>>  {
>> @@ -2360,6 +2384,16 @@ static void ppc_spapr_init(MachineState *machine)
>>      /* Set up Interrupt Controller before we create the VCPUs */
>>      xics_system_init(machine, XICS_IRQS_SPAPR, &error_fatal);
>>  
>> +    /* We don't have KVM support yet, so check for irqchip=on */
>> +    if (kvm_enabled() && machine_kernel_irqchip_required(machine)) {
>> +        error_report("kernel_irqchip requested. no XIVE support");
> 
> I think you want an actual exit(1) here, no?  error_report() will
> print an error but keep going.

yes. Today, it coredumps. I am not sure why. I will add an exit().

> 
>> +    } else {
>> +        /* XIVE uses the full range of IRQ numbers. The CPU IPIs will
>> +         * use the range below XICS_IRQ_BASE, which is unused by XICS. */
>> +        spapr->xive = spapr_xive_create(spapr, XICS_IRQ_BASE + 
>> XICS_IRQS_SPAPR,
>> +                                        &error_fatal);
> 
> XICS_IRQ_BASE == 4096, and XICS_IRQS_SPAPR (which we should rename at
> some point) == 1024.
> 
> So we have a total irq space of 5k, which is a bit odd.  I'd be ok
> with rounding it out to 8k for newer machines if that's useful.
> Sparse allocations in there might make life easier for getting
> consistent irq numbers without an "allocator" per se (because we can
> use different regions for VIO, PCI intx, MSI, etc. etc.).
I will start another thread on that topic.

Thanks,

C. 

> 
>> +    }
>> +
>>      /* Set up containers for ibm,client-architecture-support negotiated 
>> options
>>       */
>>      spapr->ov5 = spapr_ovec_new();
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 9a3885593c86..90e2b0f6c678 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -14,6 +14,7 @@ struct sPAPRNVRAM;
>>  typedef struct sPAPREventLogEntry sPAPREventLogEntry;
>>  typedef struct sPAPREventSource sPAPREventSource;
>>  typedef struct sPAPRPendingHPT sPAPRPendingHPT;
>> +typedef struct sPAPRXive sPAPRXive;
>>  
>>  #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
>>  #define SPAPR_ENTRY_POINT       0x100
>> @@ -127,6 +128,7 @@ struct sPAPRMachineState {
>>      MemoryHotplugState hotplug_memory;
>>  
>>      const char *icp_type;
>> +    sPAPRXive  *xive;
>>  };
>>  
>>  #define H_SUCCESS         0
>