From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41206) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZXY0U-00084t-OW for qemu-devel@nongnu.org; Thu, 03 Sep 2015 13:09:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZXY0R-0000Lq-I7 for qemu-devel@nongnu.org; Thu, 03 Sep 2015 13:09:02 -0400 Received: from mail-ob0-f171.google.com ([209.85.214.171]:35138) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZXY0R-0000Lh-Co for qemu-devel@nongnu.org; Thu, 03 Sep 2015 13:08:59 -0400 Received: by obuk4 with SMTP id k4so38039906obu.2 for ; Thu, 03 Sep 2015 10:08:58 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <01123b8a3d47648ef362909967f22c7dab0cfd71.1440584396.git.p.fedin@samsung.com> References: <01123b8a3d47648ef362909967f22c7dab0cfd71.1440584396.git.p.fedin@samsung.com> From: Peter Maydell Date: Thu, 3 Sep 2015 18:08:39 +0100 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v12 5/5] hw/arm/virt: Add gic-version option to virt machine List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Fedin Cc: Shlomo Pongratz , Shlomo Pongratz , QEMU Developers , Eric Auger On 26 August 2015 at 11:28, Pavel Fedin 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 > Tested-by: Ashok kumar 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