Re: [Qemu-devel] [PATCH v12 5/5] hw/arm/virt: Add gic-version option to virt machine

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

On 26 August 2015 at 11:28, Pavel Fedin <p.fedin@samsung.com> wrote:
> Add gic_version to VirtMachineState, set it 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 126 (calculated from redistributor
> space available in the memory map). GICv2 compatibility check happens
> inside arm_gic_common_realize().
>
> ITS regions are added to the memory map too, however currently they
> are not used, just reserved.
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> Tested-by: Ashok kumar <ashoks@broadcom.com>

We also need to update the ACPI table generation code, otherwise
we will be instantiating a system with a GICv3 and claiming
in the ACPI tables that it is a GICv2.

Other than that, I think my remaining review comments on this
patch should be pretty straightforward to address.

(I also haven't tried running this patchset yet, but I should
be able to get it going on a fast model tomorrow I hope.)

> ---
>  hw/arm/virt.c         | 118 ++++++++++++++++++++++++++++++++++++++++----------
>  include/hw/arm/virt.h |   5 ++-
>  2 files changed, 99 insertions(+), 24 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d5a8417..7044cb9 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -50,6 +50,7 @@
>  #include "hw/arm/fdt.h"
>  #include "hw/intc/arm_gic_common.h"
>  #include "kvm_arm.h"
> +#include "qapi/visitor.h"
>
>  /* Number of external interrupt lines to configure the GIC with */
>  #define NUM_IRQS 256
> @@ -79,6 +80,7 @@ typedef struct {
>  typedef struct {
>      MachineState parent;
>      bool secure;
> +    int32_t gic_version;
>  } VirtMachineState;
>
>  #define TYPE_VIRT_MACHINE   "virt"
> @@ -109,6 +111,10 @@ 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 },

Why have separate ITS_CONTROL and ITS_TRANSLATION entries, given
that the ITS pages are architecturally required to be contiguous
and the dt binding doesn't require you to specify the two addresses
separately?

> +    /* This redistributor space allows up to 2*64kB*126 CPUs */
> +    [VIRT_GIC_REDIST] =         { 0x08040000, 0x00FC0000 },
>      [VIRT_UART] =               { 0x09000000, 0x00001000 },
>      [VIRT_RTC] =                { 0x09010000, 0x00001000 },
>      [VIRT_FW_CFG] =             { 0x09020000, 0x0000000a },

Hmm, redistributor space. The good news here is that the architecture
(and the dt binding) lets us specify multiple blocks of memory space
for redistributors, so we can always raise the CPU limit by adding an
extra block of redistributors somewhere else in the memory map
(ie above 4GB somewhere) without breaking backwards compatibility.
The bad news is that the KVM API for the VGIC only currently
supports a single contiguous block of redistributors, so we'll need
to extend that somehow when we get to that point.

I think we need to leave enough space for all of GICC/GICV/GICH
(that's 2 pages for GICC, 2 for GICV, 1 for GICH). They're optional
in a GICv3, but we may want them for emulation later on and if we
haven't left ourselves enough space we'll be a bit stuck.

> @@ -285,6 +294,18 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi)
>  {
>      int cpu;
>
> +    /*
> +     * From Documentation/devicetree/bindings/arm/cpus.txt
> +     *  On ARM v8 64-bit systems value should be set to 2,
> +     *  that corresponds to the MPIDR_EL1 register size.
> +     *  If MPIDR_EL1[63:32] value is equal to 0 on all CPUs
> +     *  in the system, #address-cells can be set to 1, since
> +     *  MPIDR_EL1[63:32] bits are not used for CPUs
> +     *  identification.
> +     *
> +     *  Now GIC500 doesn't support affinities 2 & 3 so currently
> +     *  #address-cells can stay 1 until future GIC
> +     */

This change is true even without the GICv3 additions. I think
it ought to go in its own patch, together with a check so we
can exit with an error if armcpu->mp_affinity is too large to fit
in a single dt cell.

>      qemu_fdt_add_subnode(vbi->fdt, "/cpus");
>      qemu_fdt_setprop_cell(vbi->fdt, "/cpus", "#address-cells", 0x1);
>      qemu_fdt_setprop_cell(vbi->fdt, "/cpus", "#size-cells", 0x0);
> @@ -953,6 +1014,14 @@ static void virt_instance_init(Object *obj)
>                                      "Set on/off to enable/disable the ARM "
>                                      "Security Extensions (TrustZone)",
>                                      NULL);
> +
> +    /* Default GIC type is v2 */
> +    vms->gic_version = 2;
> +    object_property_add(obj, "gic-version", "int", virt_get_gic_version,
> +                        virt_set_gic_version, NULL, NULL, NULL);
> +    object_property_set_description(obj, "gic-version",
> +                                    "Set GIC version. "
> +                                    "Valid values are 2 and 3", NULL);

I think we should also allow 'gic-version=host" to use whatever the
best VGIC the host kernel supports is (by analogy with -cpu host).

(I'm also starting to think we want a more general board option
for "do the best you can for everything" that works for setting
both CPU and GIC, and works for both KVM and TCG, but we can add
that later.)

thanks
-- PMM