On 27 April 2014 02:09, Peter Crosthwaite <peter.crosthwaite@xxxxxxxxxx> wrote:
> On Fri, Apr 25, 2014 at 3:54 AM, Peter Maydell <peter.maydell@xxxxxxxxxx> 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 <peter.maydell@xxxxxxxxxx>
>> ---
>> 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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
