Hi, See inline.
On Friday, July 24, 2015, Peter Maydell <peter.mayd...@linaro.org> wrote: > On 24 July 2015 at 10:55, Pavel Fedin <p.fe...@samsung.com <javascript:;>> > wrote: > > From: Shlomo Pongratz <shlomo.pongr...@huawei.com <javascript:;>> > > > > 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 >
