See inline.
On Friday, July 24, 2015, Peter Maydell <
address@hidden> wrote:
On 24 July 2015 at 10:55, Pavel Fedin <address@hidden> wrote:
> From: Shlomo Pongratz <address@hidden>
>
> This class is to be used by both software and KVM implementations of GICv3
>
> +++ b/include/hw/intc/arm_gicv3_common.h
> @@ -0,0 +1,112 @@
> +/*
> + * ARM GIC support
> + *
> + * Copyright (c) 2012 Linaro Limited
> + * Copyright (c) 2015 Huawei.
> + * Written by Peter Maydell
> + * Extended to 64 cores by Shlomo Pongratz
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef HW_ARM_GICV3_COMMON_H
> +#define HW_ARM_GICV3_COMMON_H
> +
> +#include "hw/sysbus.h"
> +#include "hw/intc/arm_gic_common.h"
> +
> +/* Maximum number of possible interrupts, determined by the GIC architecture */
> +#define GICV3_MAXIRQ 1020
So how do LPIs work? They have IDs above 1023.
Currently I don't use LPI as the virt virtual machine doesn't use it. There is no point to write something I can't test.
> +#define GICV3_NCPU 64
Where does '64' come from as a maximum limit?
The goal is 128 (the GIC500 limitation) but since the largest builtin is uint64_t this will require the usage of bitfiled library. Therefore I thought it should be delayed to future version after the logic part is debugged.
> +
> +typedef struct gicv3_irq_state {
> + /* The enable bits are only banked for per-cpu interrupts. */
> + uint64_t enabled;
> + uint64_t pending;
> + uint64_t active;
> + uint64_t level;
> + uint64_t group;
Why are these uint64_t ?
These are bitfields. which were enlarged from 8 (old limit) to 64 (new limit)
> + bool edge_trigger; /* true: edge-triggered, false: level-triggered */
> +} gicv3_irq_state;
> +
> +typedef struct gicv3_sgi_state {
> + uint64_t pending[GICV3_NCPU];
> +} gicv3_sgi_state;
> +
> +typedef struct GICv3State {
> + /*< private >*/
> + SysBusDevice parent_obj;
> + /*< public >*/
> +
> + qemu_irq parent_irq[GICV3_NCPU];
> + qemu_irq parent_fiq[GICV3_NCPU];
> + /* GICD_CTLR; for a GIC with the security extensions the NS banked version
> + * of this register is just an alias of bit 1 of the S banked version.
> + */
> + uint32_t ctlr;
> + /* Sim GICC_CTLR; again, the NS banked version is just aliases of bits of
> + * the S banked register, so our state only needs to store the S version.
> + */
"Sim" ?
Should be simulate as I only support system registers and not memory mapped registers, but keeping this variable makes the code more close to GICv2 code.
> + uint32_t cpu_ctlr[GICV3_NCPU];
> + bool cpu_enabled[GICV3_NCPU];
> +
> + gicv3_irq_state irq_state[GICV3_MAXIRQ];
> + uint64_t irq_target[GICV3_MAXIRQ];
> + uint8_t priority1[GIC_INTERNAL][GICV3_NCPU];
> + uint8_t priority2[GICV3_MAXIRQ - GIC_INTERNAL];
> + uint16_t last_active[GICV3_MAXIRQ][GICV3_NCPU];
> + /* For each SGI on the target CPU, we store 64 bits
> + * indicating which source CPUs have made this SGI
> + * pending on the target CPU. These correspond to
> + * the bytes in the GIC_SPENDSGIR* registers as
> + * read by the target CPU.
This comment doesn't make any sense. You can't fit
64 bits into a byte, and GIC_SPENDSGIR<n> only hold
8 set-pending bits per SGI.
Correct, the comment is wrong and should be removed as spi_mending was replaced by a structure.
The usage of a structure and not by a uint64_t vector is because there is no serialization macro for it.
> + */
> + gicv3_sgi_state sgi_state[GIC_NR_SGIS];
> +
> + uint16_t priority_mask[GICV3_NCPU];u
> + uint16_t running_irq[GICV3_NCPU];
> + uint16_t running_priority[GICV3_NCPU];
> + uint16_t current_pending[GICV3_NCPU];
> +
> + uint32_t cpu_mask; /* For redistributor */
> + uint32_t num_cpu;
> + MemoryRegion iomem_dist; /* Distributor */
> + MemoryRegion iomem_lpi; /* Redistributor */
> + uint32_t num_irq;
> + uint32_t revision;
> + bool security_levels;
> + int dev_fd; /* kvm device fd if backed by kvm vgic support */
> +} GICv3State;
This whole struct reads like "we just took the GICv2 state
and changed it as little as possible beyond bumping the
NCPU define a bit". That doesn't make me very confident
that it's actually correct for GICv3...
I admit that the intention of the first is to make as little changes a possible and yet to be able to boot 64 Linux kernel (which was achieved). Note that the kernel identified and used the GICv3 driver.
thanks
-- PMM