Hi Zenghui, On 12/18/19 4:46 AM, Zenghui Yu wrote: > Hi Eric, > > I have to admit that this is the first time I've looked into > the kvm-unit-tests code, so only some minor comments inline :) no problem. Thank you for looking at this. By the way, with patch 16 I was able to test yout fix: "KVM: arm/arm64: vgic: Don't rely on the wrong pending table". Reverting it produced an error. I forgot to mention that. > > On 2019/12/16 22:02, Eric Auger wrote: >> Detect the presence of an ITS as part of the GICv3 init >> routine, initialize its base address and read few registers >> the IIDR, the TYPER to store its dimensioning parameters. >> >> This is our first ITS test, belonging to a new "its" group. >> >> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx> > > [...] > >> diff --git a/lib/arm/asm/gic-v3-its.h b/lib/arm/asm/gic-v3-its.h >> new file mode 100644 >> index 0000000..2ce483e >> --- /dev/null >> +++ b/lib/arm/asm/gic-v3-its.h >> @@ -0,0 +1,116 @@ >> +/* >> + * All ITS* defines are lifted from include/linux/irqchip/arm-gic-v3.h >> + * >> + * Copyright (C) 2016, Red Hat Inc, Andrew Jones <drjones@xxxxxxxxxx> >> + * >> + * This work is licensed under the terms of the GNU LGPL, version 2. >> + */ >> +#ifndef _ASMARM_GIC_V3_ITS_H_ >> +#define _ASMARM_GIC_V3_ITS_H_ >> + >> +#ifndef __ASSEMBLY__ >> + >> +#define GITS_CTLR 0x0000 >> +#define GITS_IIDR 0x0004 >> +#define GITS_TYPER 0x0008 >> +#define GITS_CBASER 0x0080 >> +#define GITS_CWRITER 0x0088 >> +#define GITS_CREADR 0x0090 >> +#define GITS_BASER 0x0100 >> + >> +#define GITS_TYPER_PLPIS (1UL << 0) >> +#define GITS_TYPER_IDBITS_SHIFT 8 >> +#define GITS_TYPER_DEVBITS_SHIFT 13 >> +#define GITS_TYPER_DEVBITS(r) ((((r) >> >> GITS_TYPER_DEVBITS_SHIFT) & 0x1f) + 1) >> +#define GITS_TYPER_PTA (1UL << 19) >> +#define GITS_TYPER_HWCOLLCNT_SHIFT 24 >> + >> +#define GITS_CTLR_ENABLE (1U << 0) >> + >> +#define GITS_CBASER_VALID (1UL << 63) >> +#define GITS_CBASER_SHAREABILITY_SHIFT (10) >> +#define GITS_CBASER_INNER_CACHEABILITY_SHIFT (59) >> +#define GITS_CBASER_OUTER_CACHEABILITY_SHIFT (53) >> +#define >> GITS_CBASER_SHAREABILITY_MASK \ >> + GIC_BASER_SHAREABILITY(GITS_CBASER, SHAREABILITY_MASK) >> +#define >> GITS_CBASER_INNER_CACHEABILITY_MASK \ >> + GIC_BASER_CACHEABILITY(GITS_CBASER, INNER, MASK) >> +#define >> GITS_CBASER_OUTER_CACHEABILITY_MASK \ >> + GIC_BASER_CACHEABILITY(GITS_CBASER, OUTER, MASK) >> +#define GITS_CBASER_CACHEABILITY_MASK >> GITS_CBASER_INNER_CACHEABILITY_MASK >> + >> +#define >> GITS_CBASER_InnerShareable \ >> + GIC_BASER_SHAREABILITY(GITS_CBASER, InnerShareable) >> + >> +#define GITS_CBASER_nCnB GIC_BASER_CACHEABILITY(GITS_CBASER, >> INNER, nCnB) >> +#define GITS_CBASER_nC GIC_BASER_CACHEABILITY(GITS_CBASER, >> INNER, nC) >> +#define GITS_CBASER_RaWt GIC_BASER_CACHEABILITY(GITS_CBASER, >> INNER, RaWt) >> +#define GITS_CBASER_RaWb GIC_BASER_CACHEABILITY(GITS_CBASER, >> INNER, RaWt) > > s/RaWt/RaWb/ OK > >> +#define GITS_CBASER_WaWt GIC_BASER_CACHEABILITY(GITS_CBASER, >> INNER, WaWt) >> +#define GITS_CBASER_WaWb GIC_BASER_CACHEABILITY(GITS_CBASER, >> INNER, WaWb) >> +#define GITS_CBASER_RaWaWt GIC_BASER_CACHEABILITY(GITS_CBASER, >> INNER, RaWaWt) >> +#define GITS_CBASER_RaWaWb GIC_BASER_CACHEABILITY(GITS_CBASER, >> INNER, RaWaWb) >> + >> +#define GITS_BASER_NR_REGS 8 >> + >> +#define GITS_BASER_VALID (1UL << 63) >> +#define GITS_BASER_INDIRECT (1ULL << 62) >> + >> +#define GITS_BASER_INNER_CACHEABILITY_SHIFT (59) >> +#define GITS_BASER_OUTER_CACHEABILITY_SHIFT (53) >> +#define GITS_BASER_CACHEABILITY_MASK 0x7 >> + >> +#define GITS_BASER_nCnB GIC_BASER_CACHEABILITY(GITS_BASER, >> INNER, nCnB) >> + >> +#define GITS_BASER_TYPE_SHIFT (56) >> +#define GITS_BASER_TYPE(r) (((r) >> >> GITS_BASER_TYPE_SHIFT) & 7) >> +#define GITS_BASER_ENTRY_SIZE_SHIFT (48) >> +#define GITS_BASER_ENTRY_SIZE(r) ((((r) >> >> GITS_BASER_ENTRY_SIZE_SHIFT) & 0x1f) + 1) >> +#define GITS_BASER_SHAREABILITY_SHIFT (10) >> +#define >> GITS_BASER_InnerShareable \ >> + GIC_BASER_SHAREABILITY(GITS_BASER, InnerShareable) >> +#define GITS_BASER_PAGE_SIZE_SHIFT (8) >> +#define GITS_BASER_PAGE_SIZE_4K (0UL << >> GITS_BASER_PAGE_SIZE_SHIFT) >> +#define GITS_BASER_PAGE_SIZE_16K (1UL << >> GITS_BASER_PAGE_SIZE_SHIFT) >> +#define GITS_BASER_PAGE_SIZE_64K (2UL << >> GITS_BASER_PAGE_SIZE_SHIFT) >> +#define GITS_BASER_PAGE_SIZE_MASK (3UL << >> GITS_BASER_PAGE_SIZE_SHIFT) >> +#define GITS_BASER_PAGES_MAX 256 >> +#define GITS_BASER_PAGES_SHIFT (0) >> +#define GITS_BASER_NR_PAGES(r) (((r) & 0xff) + 1) >> +#define GITS_BASER_PHYS_ADDR_MASK 0xFFFFFFFFF000 >> + >> +#define GITS_BASER_TYPE_NONE 0 >> +#define GITS_BASER_TYPE_DEVICE 1 >> +#define GITS_BASER_TYPE_VCPU 2 >> +#define GITS_BASER_TYPE_CPU 3 > > '3' is one of the reserved values of the GITS_BASER.Type field, and > what do we expect with a "GITS_BASER_TYPE_CPU" table type? ;-) Yes I agree. This is code extracted from the irqchip header in Dec 2016. I should have checked again. I only use DEVICE and COLLECTION here. I will remove all the defines I am not using at the moment. I guess most of the defines related to memory/cache mgt are not mandated either. > > I think we can copy (and might update in the future) all these > macros against the latest Linux kernel. > >> +#define GITS_BASER_TYPE_COLLECTION 4 >> + >> +#define ITS_FLAGS_CMDQ_NEEDS_FLUSHING (1ULL << 0) > + >> +struct its_typer { >> + unsigned int ite_size; >> + unsigned int eventid_bits; >> + unsigned int deviceid_bits; >> + unsigned int collid_bits; >> + unsigned int hw_collections; >> + bool pta; >> + bool cil; >> + bool cct; >> + bool phys_lpi; >> + bool virt_lpi; >> +}; >> + >> +struct its_data { >> + void *base; >> + struct its_typer typer; >> +}; >> + >> +extern struct its_data its_data; >> + >> +#define gicv3_its_base() (its_data.base) >> + >> +extern void its_parse_typer(void); >> +extern void its_init(void); >> + >> +#endif /* !__ASSEMBLY__ */ >> +#endif /* _ASMARM_GIC_V3_ITS_H_ */ >> diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h >> index 55dd84b..b44da9c 100644 >> --- a/lib/arm/asm/gic.h >> +++ b/lib/arm/asm/gic.h >> @@ -40,6 +40,7 @@ >> #include <asm/gic-v2.h> >> #include <asm/gic-v3.h> >> +#include <asm/gic-v3-its.h> >> #define PPI(irq) ((irq) + 16) >> #define SPI(irq) ((irq) + GIC_FIRST_SPI) >> diff --git a/lib/arm/gic-v3-its.c b/lib/arm/gic-v3-its.c >> new file mode 100644 >> index 0000000..34f4d0e >> --- /dev/null >> +++ b/lib/arm/gic-v3-its.c >> @@ -0,0 +1,41 @@ >> +/* >> + * Copyright (C) 2016, Red Hat Inc, Eric Auger <eric.auger@xxxxxxxxxx> >> + * >> + * This work is licensed under the terms of the GNU LGPL, version 2. >> + */ >> +#include <asm/gic.h> >> + >> +struct its_data its_data; >> + >> +void its_parse_typer(void) >> +{ >> + u64 typer = readq(gicv3_its_base() + GITS_TYPER); >> + >> + its_data.typer.ite_size = ((typer >> 4) & 0xf) + 1; >> + its_data.typer.pta = typer & GITS_TYPER_PTA; >> + its_data.typer.eventid_bits = >> + ((typer >> GITS_TYPER_IDBITS_SHIFT) & 0x1f) + 1; >> + its_data.typer.deviceid_bits = GITS_TYPER_DEVBITS(typer) + 1; > > No need to '+1'. As GITS_TYPER_DEVBITS already helps us to calculate > the implemented DeviceID bits. OK > >> + >> + its_data.typer.cil = (typer >> 36) & 0x1; >> + if (its_data.typer.cil) >> + its_data.typer.collid_bits = ((typer >> 32) & 0xf) + 1; >> + else >> + its_data.typer.collid_bits = 16; >> + >> + its_data.typer.hw_collections = >> + (typer >> GITS_TYPER_HWCOLLCNT_SHIFT) & 0xff; >> + >> + its_data.typer.cct = typer & 0x4; >> + its_data.typer.virt_lpi = typer & 0x2; >> + its_data.typer.phys_lpi = typer & GITS_TYPER_PLPIS; > > Personally, mix using of GITS_TYPER_* macros and some magic constants to > parse the TYPER makes it a bit difficult to review the code. Maybe we > can have more such kinds of macros in the header file and get rid of all > hardcoded numbers? Sure I will clean that up. Thanks! Eric > > > Thanks, > Zenghui > > _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm