Re: [Qemu-devel] [PATCH 1/3] hw/arm/virt: Create the GIC ourselves rather than (ab)using a15mpcore_priv

[Peter Maydell]

On 27 April 2014 02:09, Peter Crosthwaite <address@hidden> wrote:
> On Fri, Apr 25, 2014 at 3:54 AM, Peter Maydell <address@hidden> wrote:
>> Rather than having the virt machine model create an a15mpcore_priv
>> device regardless of the actual CPU type in order to instantiate the GIC,
>> move to having the machine model create the GIC directly. This
>> corresponds to a system which uses a standalone GIC (eg the GIC-400)
>> rather than the one built in to the CPU core.
>>
>> The primary motivation for this is to support the Cortex-A57,
>> which for a KVM configuration will use a GICv2, which is not
>> built into the CPU.
>>
>> Signed-off-by: Peter Maydell <address@hidden>
>> ---
>>  hw/arm/virt.c | 82 
>> ++++++++++++++++++++++++++++++++++++++---------------------
>>  1 file changed, 53 insertions(+), 29 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 2bbc931..ecff256 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -75,8 +75,6 @@ typedef struct MemMapEntry {
>>  typedef struct VirtBoardInfo {
>>      struct arm_boot_info bootinfo;
>>      const char *cpu_model;
>> -    const char *qdevname;
>> -    const char *gic_compatible;
>
> I'm not sure I understand the motivation for removing the data driven
> type and dts-compat. They seem useful to me esp. considering there may
> be variation if some virt boards want later GIC versions and others
> stay behind. Cant these just drive the GIC type and compat rather than
> going back to hardcodedness?

Basically they're not what you'd want for "maybe GICv2, maybe
GICv2 + v2m, maybe GICv3". I think for those you probably
end up needing a simple enum and three different functions for
setup. I put these into the struct when I was expecting to have
a lot of CPU container objects  like a15mpcore_priv, which all
behave essentially the same way. The GIC objects definitely
won't, so this is definitely insufficient, and we can't know what's
actually going to be required until we've got a GICv3 we can
wire up. So it seemed best to remove the now-pointless fields.
(Also, GICv2 vs v3 vs v2+v2m is probably not a per-CPU
decision; it's orthogonal. So even if we wanted it data driven
it might need to go somewhere else.)

>>      const MemMapEntry *memmap;
>>      const int *irqmap;
>>      int smp_cpus;
>> @@ -117,16 +115,11 @@ static const int a15irqmap[] = {
>>  static VirtBoardInfo machines[] = {
>>      {
>>          .cpu_model = "cortex-a15",
>> -        .qdevname = "a15mpcore_priv",
>
> Which would mean this would just change over to GICs qdev name.
>
>> -        .gic_compatible = "arm,cortex-a15-gic",
>
> This would stay as is.
>
>>          .memmap = a15memmap,
>>          .irqmap = a15irqmap,
>>      },
>>      {
>>          .cpu_model = "host",
>> -        /* We use the A15 private peripheral model to get a V2 GIC */
>> -        .qdevname = "a15mpcore_priv",
>> -        .gic_compatible = "arm,cortex-a15-gic",
>>          .memmap = a15memmap,
>>          .irqmap = a15irqmap,
>>      },
>> @@ -251,8 +244,9 @@ static void fdt_add_gic_node(const VirtBoardInfo *vbi)
>>      qemu_fdt_setprop_cell(vbi->fdt, "/", "interrupt-parent", gic_phandle);
>>
>>      qemu_fdt_add_subnode(vbi->fdt, "/intc");
>> +    /* 'cortex-a15-gic' means 'GIC v2' */
>>      qemu_fdt_setprop_string(vbi->fdt, "/intc", "compatible",
>> -                                vbi->gic_compatible);
>> +                            "arm,cortex-a15-gic");
>
> no change here either I think.
>
>>      qemu_fdt_setprop_cell(vbi->fdt, "/intc", "#interrupt-cells", 3);
>>      qemu_fdt_setprop(vbi->fdt, "/intc", "interrupt-controller", NULL, 0);
>>      qemu_fdt_setprop_sized_cells(vbi->fdt, "/intc", "reg",
>> @@ -263,6 +257,56 @@ static void fdt_add_gic_node(const VirtBoardInfo *vbi)
>>      qemu_fdt_setprop_cell(vbi->fdt, "/intc", "phandle", gic_phandle);
>>  }
>>
>> +static void create_gic(const VirtBoardInfo *vbi, qemu_irq *pic)
>> +{
>> +    /* We create a standalone GIC v2 */
>> +    DeviceState *gicdev;
>> +    SysBusDevice *gicbusdev;
>> +    const char *gictype = "arm_gic";
>
> And this remains data driven.

Except it can't, because of:

>> +    int i;
>> +
>> +    if (kvm_irqchip_in_kernel()) {
>> +        gictype = "kvm-arm-gic";
>> +    }

So what you would need to be expressing in the data is
"arm_gic if external irqchip, else kvm-arm-gic". One field
isn't enough for that.

>> +    for (i = 0; i < smp_cpus; i++) {
>> +        DeviceState *cpudev = DEVICE(qemu_get_cpu(i));
>> +        int ppibase = NUM_IRQS + i * 32;
>> +        /* physical timer; we wire it up to the non-secure timer's ID,
>> +         * since a real A15 always has TrustZone but QEMU doesn't.
>> +         */
>> +        qdev_connect_gpio_out(cpudev, 0,
>> +                              qdev_get_gpio_in(gicdev, ppibase + 30));
>> +        /* virtual timer */
>> +        qdev_connect_gpio_out(cpudev, 1,
>> +                              qdev_get_gpio_in(gicdev, ppibase + 27));
>
> I'd take the oppurtunity to tie the dts PPI mappings and these magic
> numbers together. Eg, macroify "30" "27" and then macrofiy:
>
>     qemu_fdt_setprop_cells(vbi->fdt, "/timer", "interrupts",
>                                GIC_FDT_IRQ_TYPE_PPI, 13, irqflags,
>                                GIC_FDT_IRQ_TYPE_PPI, 14, irqflags,
>                                GIC_FDT_IRQ_TYPE_PPI, 11, irqflags,
>                                GIC_FDT_IRQ_TYPE_PPI, 10, irqflags);
>
> with the -16 shift.

Maybe. I think I'll put that in a separate patch.

thanks
-- PMM