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
>
>
