Re: [Qemu-devel] [PATCH v8 5/5] Add gicversion option to virt machine

[Peter Maydell <peter.maydell@linaro.org>]

On 10 August 2015 at 13:06, Pavel Fedin <p.fedin@samsung.com> wrote:
> Set kernel_irqchip_type according to value of the option and pass it
> around where necessary. Instantiate devices and fdt nodes according
> to the choice.
>
> max_cpus for virt machine increased to 64. GICv2 compatibility check
> happens inside arm_gic_common_realize().

>  /* Number of external interrupt lines to configure the GIC with */
>  #define NUM_IRQS 256
> @@ -108,6 +109,9 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_GIC_DIST] =           { 0x08000000, 0x00010000 },
>      [VIRT_GIC_CPU] =            { 0x08010000, 0x00010000 },
>      [VIRT_GIC_V2M] =            { 0x08020000, 0x00001000 },
> +    [VIRT_ITS_CONTROL] =        { 0x08020000, 0x00010000 },
> +    [VIRT_ITS_TRANSLATION] =    { 0x08030000, 0x00010000 },

We don't implement ITS yet, so are these placeholders for that?
(That's OK, but if they don't do anything yet it's worth
commenting to that effect.)

Any particular reason for having two separate VIRT_ITS_*
entries? The spec mandates that the two 64K pages of ITS
have to be consecutive, so it would make life easier for
boards if they were just a single memory region.

> +    [VIRT_GIC_REDIST] =         { 0x08040000, 0x00800000 },

Is this layout borrowed from any particular real world board,
by the way?

>      VirtMachineState *vms = VIRT_MACHINE(obj);
> @@ -947,6 +1035,11 @@ static void virt_instance_init(Object *obj)
>
>      /* Default GIC type is v2 */
>      vms->parent.kernel_irqchip_type = QEMU_GIC_TYPE_V2;
> +    object_property_add(obj, "gicversion", "int", virt_get_gic_version,
> +                        virt_set_gic_version, NULL, NULL, NULL);
> +    object_property_set_description(obj, "gicversion",
> +                                    "Set GIC version. "
> +                                    "Valid values are 2 and 3", NULL);
>  }

"gic-version" would be more in line with other property naming
styles I think.

>
>  static void virt_class_init(ObjectClass *oc, void *data)
> @@ -956,7 +1049,8 @@ static void virt_class_init(ObjectClass *oc, void *data)
>      mc->name = TYPE_VIRT_MACHINE;
>      mc->desc = "ARM Virtual Machine",
>      mc->init = machvirt_init;
> -    mc->max_cpus = 8;
> +    /* With gic3 full implementation (with bitops) rase the lmit to 128 */
> +    mc->max_cpus = 64;

Comment disagrees with code, and in any case where does the CPU
maximum limit come from?

thanks
-- PMM