Re: [Qemu-devel] [PATCH v6 2/2] hw/arm: Add 'virt' platform

[Peter Maydell]

On 14 August 2013 10:02, Alexander Graf <address@hidden> wrote:
>
> On 13.08.2013, at 14:03, Peter Maydell wrote:
>
>> +    /* No PSCI for TCG yet */
>> +#ifdef CONFIG_KVM
>
> Do you need this #ifdef? Are the headers really not included when CONFIG_KVM 
> is disabled?

Yes, the KVM_PSCI_* constants are defined in the linux-headers/
kernel include files, which are only pulled in when CONFIG_KVM is
defined. (Has to be that way, because the kernel headers aren't
guaranteed to be buildable on platforms other than Linux.)

>> +    if (kvm_enabled()) {
>> +        qemu_devtree_add_subnode(fdt, "/psci");
>> +        qemu_devtree_setprop_string(fdt, "/psci", "compatible", "arm,psci");
>> +        qemu_devtree_setprop_string(fdt, "/psci", "method", "hvc");
>> +        qemu_devtree_setprop_cell(fdt, "/psci", "cpu_suspend",
>> +                                  KVM_PSCI_FN_CPU_SUSPEND);
>> +        qemu_devtree_setprop_cell(fdt, "/psci", "cpu_off", 
>> KVM_PSCI_FN_CPU_OFF);
>> +        qemu_devtree_setprop_cell(fdt, "/psci", "cpu_on", 
>> KVM_PSCI_FN_CPU_ON);
>> +        qemu_devtree_setprop_cell(fdt, "/psci", "migrate", 
>> KVM_PSCI_FN_MIGRATE);
>> +    }
>> +#endif

>> +    /*
>> +     * Only supported method of starting secondary CPUs is PSCI and
>> +     * PSCI is not yet supported with TCG, so limit smp_cpus to 1
>
> Sounds like a good point in time to implement a KVM compatible PSCI interface 
> in TCG ;).

It is certainly on our TODO list, but since it requires tackling the
"SMC in a non-trustzone QEMU" issue I preferred not to tangle it up
with this patchset.

>> +     * if we're not using KVM.
>> +     */
>> +    if (!kvm_enabled() && smp_cpus > 1) {
>> +        error_report("mach-virt: must enable KVM to use multiple CPUs");
>> +        exit(1);
>> +    }
>> +
>> +    if (args->ram_size > vbi->memmap[VIRT_MEM].size) {
>> +        error_report("mach-virt: cannot model more than 30GB RAM");
>> +        exit(1);
>> +    }
>> +
>> +    create_fdt(vbi);
>> +    fdt_add_timer_nodes(vbi);
>> +
>> +    for (n = 0; n < smp_cpus; n++) {
>> +        ARMCPU *cpu;
>> +        qemu_irq *irqp;
>> +
>> +        cpu = cpu_arm_init(cpu_model);
>> +        irqp = arm_pic_init_cpu(cpu);
>> +        cpu_irq[n] = irqp[ARM_PIC_CPU_IRQ];
>
> I don't see a check for smp_cpus <= 4, but the array is size 4?

This array will probably go away once this is rebased on my
"get rid of arm_pic" patchset that I posted last week.

>> +    }
>> +    fdt_add_cpu_nodes(vbi);
>> +
>> +    memory_region_init_ram(ram, NULL, "mach-virt.ram", args->ram_size);
>> +    vmstate_register_ram_global(ram);
>> +    memory_region_add_subregion(sysmem, vbi->memmap[VIRT_MEM].base, ram);
>> +
>> +    dev = qdev_create(NULL, vbi->qdevname);
>> +    qdev_prop_set_uint32(dev, "num-cpu", smp_cpus);
>> +    qdev_init_nofail(dev);
>> +    busdev = SYS_BUS_DEVICE(dev);
>> +    sysbus_mmio_map(busdev, 0, vbi->memmap[VIRT_CPUPERIPHS].base);
>> +    fdt_add_gic_node(vbi);
>> +    for (n = 0; n < smp_cpus; n++) {
>> +        sysbus_connect_irq(busdev, n, cpu_irq[n]);
>> +    }
>> +
>> +    for (n = 0; n < 64; n++) {
>
> Where does that 64 come from? I presume from the GIC? Any chance this could 
> be put into a global define?

Nope, it's just a decision by this board to have 64 external
GIC interrupts. We could make it 96 or 128 if we chose (and
set a property on the GIC device to match). You could argue
that we shouldn't rely on knowing what the default value for
the GIC's num-irqs property is though.

>> +static QEMUMachine machvirt_a15_machine = {
>> +    .name = "virt",
>> +    .desc = "ARM Virtual Machine",
>> +    .init = machvirt_init,
>> +    .max_cpus = 4,
>
> Ah, this is where smp_cpus gets limited to 4.
>
> Why is it limited to 4? And this really should be a define that you
> reuse on the IRQ map above :).

An A15 can only have 4 CPU cores maximum -- hardware limit.
This will have to be adjusted when virt supports non-a15 CPUs,
though.

-- PMM