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

[Alexander Graf]

On 14.08.2013, at 11:19, Peter Maydell wrote:

> 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.)

What a shame. But then again once you implement a compatible interface in QEMU, 
the defines will be available regardless so you can get rid of the #ifdef again 
:).

> 
>>> +    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.

I agree.

> 
>>> +     * 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.

Can you just use smp_cpus as array length for the time being?

> 
>>> +    }
>>> +    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.

We shouldn't, no :).

> 
>>> +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.

Ah, ok. This is the 15 machine description, so I guess imposing a15 limits is 
ok.


Alex