|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH v2 02/26] ARM: GICv3: allocate LPI pending and property table
Hi,
On 04/01/17 21:09, Stefano Stabellini wrote:
> On Thu, 22 Dec 2016, Andre Przywara wrote:
>> The ARM GICv3 ITS provides a new kind of interrupt called LPIs.
>> The pending bits and the configuration data (priority, enable bits) for
>> those LPIs are stored in tables in normal memory, which software has to
>> provide to the hardware.
>> Allocate the required memory, initialize it and hand it over to each
>> ITS. The maximum number of LPIs to be used can be adjusted with the
>> command line option "max_lpi_bits", which defaults to a compile time
>> constant exposed in Kconfig.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>> xen/arch/arm/Kconfig | 10 +++++
>> xen/arch/arm/efi/efi-boot.h | 1 -
>> xen/arch/arm/gic-its.c | 94
>> +++++++++++++++++++++++++++++++++++++++
>> xen/arch/arm/gic-v3.c | 43 ++++++++++++++++++
>> xen/include/asm-arm/bitops.h | 1 +
>> xen/include/asm-arm/cache.h | 4 ++
>> xen/include/asm-arm/gic-its.h | 22 ++++++++-
>> xen/include/asm-arm/gic_v3_defs.h | 48 +++++++++++++++++++-
>> 8 files changed, 220 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>> index bf64c61..a7d941c 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -49,6 +49,16 @@ config HAS_ITS
>> bool "GICv3 ITS MSI controller support"
>> depends on HAS_GICV3
>>
>> +config MAX_HOST_LPI_BITS
> ^ MAX_PHYS_LPI_BITS
Right, I missed that symbol.
>> + depends on HAS_ITS
>> + int "Maximum bits for GICv3 host LPIs (14-32)"
>> + range 14 32
>> + default "20"
>> + help
>> + Specifies the maximum number of LPIs (bits) Xen should take care
>> of.
>> + This can be overriden on the command line with the max_lpi_bits
>> + parameter.
>> +
>> endmenu
>>
>> menu "ARM errata workaround via the alternative framework"
>> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
>> index 045d6ce..dc64aec 100644
>> --- a/xen/arch/arm/efi/efi-boot.h
>> +++ b/xen/arch/arm/efi/efi-boot.h
>> @@ -10,7 +10,6 @@
>> #include "efi-dom0.h"
>>
>> void noreturn efi_xen_start(void *fdt_ptr, uint32_t fdt_size);
>> -void __flush_dcache_area(const void *vaddr, unsigned long size);
>>
>> #define DEVICE_TREE_GUID \
>> {0xb1b621d5, 0xf19c, 0x41a5, {0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa,
>> 0xe0}}
>> diff --git a/xen/arch/arm/gic-its.c b/xen/arch/arm/gic-its.c
>> index 973f9f9..6c3a35d 100644
>> --- a/xen/arch/arm/gic-its.c
>> +++ b/xen/arch/arm/gic-its.c
>> @@ -20,10 +20,104 @@
>> #include <xen/lib.h>
>> #include <xen/device_tree.h>
>> #include <xen/libfdt/libfdt.h>
>> +#include <xen/sizes.h>
>> +#include <asm/p2m.h>
>> #include <asm/gic.h>
>> #include <asm/gic_v3_defs.h>
>> #include <asm/gic-its.h>
>>
>> +/* Global state */
>> +static struct {
>> + uint8_t *lpi_property;
>> + unsigned int host_lpi_bits;
>
> I think it is best to rename host_lpi_bits to phys_lpi_bits, or, even
> better, phys_nr_lpi because it represents the number of LPIs.
But we need the number of bits in at least one function (for populating
the PROPBASER register), also the number of bits is the original input
value and sets the limit on possible values - as it needs to be a power
of two.
So shall I use nr_phys_lpi_bits instead?
>> +} lpi_data;
>> +
>> +/* Pending table for each redistributor */
>> +static DEFINE_PER_CPU(void *, pending_table);
>> +
>> +#define MAX_PHYS_LPIS (BIT_ULL(lpi_data.host_lpi_bits) - 8192)
>> +
>> +uint64_t gicv3_lpi_allocate_pendtable(void)
>> +{
>> + uint64_t reg, attr;
>> + void *pendtable;
>> +
>> + attr = GIC_BASER_CACHE_RaWaWb <<
>> GICR_PENDBASER_INNER_CACHEABILITY_SHIFT;
>> + attr |= GIC_BASER_CACHE_SameAsInner <<
>> GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT;
>> + attr |= GIC_BASER_InnerShareable << GICR_PENDBASER_SHAREABILITY_SHIFT;
>> +
>> + if ( !this_cpu(pending_table) )
>> + {
>> + /*
>> + * The pending table holds one bit per LPI, so we need (2 << 3) bits
>> + * less bytes. The pending table covers even bits for interrupt IDs
>> + * below 8192, so we allocate the full range.
>> + * The ITS imposes a 64KB alignment requirement.
>> + */
>> + pendtable = _xmalloc(BIT_ULL(lpi_data.host_lpi_bits) / 8, SZ_64K);
>
> If we need (2 << 3) less bytes, why are we dividing by 8?
Because ... I am wrong here ;-)
> Isn't this just a straightforward bits to bytes conversion?
Yes.
> I think the comment is probably wrong or outdated.
Yeah, I tried to make this clearer after your previous comments (where
this simple bits-to-bytes apparently didn't come through) and got
somehow carried away ;-)
>> + if ( !pendtable )
>> + return 0;
>> +
>> + memset(pendtable, 0, BIT_ULL(lpi_data.host_lpi_bits) / 8);
>> + __flush_dcache_area(pendtable, BIT_ULL(lpi_data.host_lpi_bits) / 8);
>> +
>> + this_cpu(pending_table) = pendtable;
>> + }
>> + else
>> + {
>> + pendtable = this_cpu(pending_table);
>
> Isn't this an error condition?
This is internally only called once when each redistributor is
enumerated. So this maybe a BUG_ON(), but on the other hand I don't see
a reason why this would be fatal. If we try to "allocate" a second time,
we just get the existing version.
But I can happily replace this with a BUG_ON(), if you prefer. Seems to
be more matching the Xen policy, I guess.
>> + }
>> +
>> + reg = attr | GICR_PENDBASER_PTZ;
>> + reg |= virt_to_maddr(pendtable) & GENMASK(51, 16);
>> +
>> + return reg;
>> +}
>> +
>> +uint64_t gicv3_lpi_get_proptable()
>> +{
>> + uint64_t attr, reg;
>> +
>> + attr = GIC_BASER_CACHE_RaWaWb <<
>> GICR_PENDBASER_INNER_CACHEABILITY_SHIFT;
>> + attr |= GIC_BASER_CACHE_SameAsInner <<
>> GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT;
>> + attr |= GIC_BASER_InnerShareable << GICR_PENDBASER_SHAREABILITY_SHIFT;
>> +
>> + /*
>> + * The property table is shared across all redistributors, so allocate
>> + * this only once, but return the same value on subsequent calls.
>> + */
>> + if ( !lpi_data.lpi_property )
>> + {
>> + /* The property table holds one byte per LPI. */
>> + void *table = alloc_xenheap_pages(lpi_data.host_lpi_bits -
>> PAGE_SHIFT,
>> + 0);
>> +
>> + if ( !table )
>> + return 0;
>> +
>> + memset(table, GIC_PRI_IRQ | LPI_PROP_RES1, MAX_PHYS_LPIS);
>> + __flush_dcache_area(table, MAX_PHYS_LPIS);
>> + lpi_data.lpi_property = table;
>> + }
>> +
>> + reg = attr | ((lpi_data.host_lpi_bits - 1) << 0);
>> + reg |= virt_to_maddr(lpi_data.lpi_property) & GENMASK(51, 12);
>> +
>> + return reg;
>> +}
>> +
>> +static unsigned int max_lpi_bits = CONFIG_MAX_HOST_LPI_BITS;
>> +integer_param("max_lpi_bits", max_lpi_bits);
>> +
>> +int gicv3_lpi_init_host_lpis(unsigned int hw_lpi_bits)
> ^ gicv3_lpi_init_phys_lpis
OK.
>> +{
>> + lpi_data.host_lpi_bits = min(hw_lpi_bits, max_lpi_bits);
>> +
>> + printk("GICv3: using at most %lld LPIs on the host.\n", MAX_PHYS_LPIS);
>> +
>> + return 0;
>> +}
>> +
>> /* Scan the DT for any ITS nodes and create a list of host ITSes out of it.
>> */
>> void gicv3_its_dt_init(const struct dt_device_node *node)
>> {
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 238da84..1c6869d 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -546,6 +546,9 @@ static void __init gicv3_dist_init(void)
>> type = readl_relaxed(GICD + GICD_TYPER);
>> nr_lines = 32 * ((type & GICD_TYPE_LINES) + 1);
>>
>> + if ( type & GICD_TYPE_LPIS )
>> + gicv3_lpi_init_host_lpis(((type >> GICD_TYPE_ID_BITS_SHIFT) & 0x1f)
>> + 1);
>
> Still need to define a mask instead of using 0x1f
I guess I prefer a macro for the whole statement here. I regularly get
annoyed by those descriptive names getting too long and forcing a rather
simple statement on two or more lines ...
>> +
>> printk("GICv3: %d lines, (IID %8.8x).\n",
>> nr_lines, readl_relaxed(GICD + GICD_IIDR));
>>
>> @@ -616,6 +619,32 @@ static int gicv3_enable_redist(void)
>> return 0;
>> }
>>
>> +static int gicv3_rdist_init_lpis(void __iomem * rdist_base)
>> +{
>> + uint32_t reg;
>> + uint64_t table_reg;
>> +
>> + if ( list_empty(&host_its_list) )
>> + return -ENODEV;
>> +
>> + /* Make sure LPIs are disabled before setting up the BASERs. */
>> + reg = readl_relaxed(rdist_base + GICR_CTLR);
>> + if ( reg & GICR_CTLR_ENABLE_LPIS )
>> + return -EBUSY;
>> +
>> + table_reg = gicv3_lpi_allocate_pendtable();
>> + if ( !table_reg )
>> + return -ENOMEM;
>> + writeq_relaxed(table_reg, rdist_base + GICR_PENDBASER);
>> +
>> + table_reg = gicv3_lpi_get_proptable();
>> + if ( !table_reg )
>> + return -ENOMEM;
>> + writeq_relaxed(table_reg, rdist_base + GICR_PROPBASER);
>> +
>> + return 0;
>> +}
>> +
>> static int __init gicv3_populate_rdist(void)
>> {
>> int i;
>> @@ -658,6 +687,20 @@ static int __init gicv3_populate_rdist(void)
>> if ( (typer >> 32) == aff )
>> {
>> this_cpu(rbase) = ptr;
>> +
>> + if ( typer & GICR_TYPER_PLPIS )
>> + {
>> + int ret;
>> +
>> + ret = gicv3_rdist_init_lpis(ptr);
>> + if ( ret && ret != -ENODEV )
>> + {
>> + printk("GICv3: CPU%d: Cannot initialize LPIs: %d\n",
>> + smp_processor_id(), ret);
>> + break;
>> + }
>> + }
>> +
>> printk("GICv3: CPU%d: Found redistributor in region %d
>> @%p\n",
>> smp_processor_id(), i, ptr);
>> return 0;
>> diff --git a/xen/include/asm-arm/bitops.h b/xen/include/asm-arm/bitops.h
>> index bda8898..c9adf79 100644
>> --- a/xen/include/asm-arm/bitops.h
>> +++ b/xen/include/asm-arm/bitops.h
>> @@ -25,6 +25,7 @@
>> #define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_WORD))
>> #define BIT_WORD(nr) ((nr) / BITS_PER_WORD)
>> #define BITS_PER_BYTE 8
>> +#define BIT_ULL(nr) (1ULL << (nr))
>>
>> #define ADDR (*(volatile int *) addr)
>> #define CONST_ADDR (*(const volatile int *) addr)
>> diff --git a/xen/include/asm-arm/cache.h b/xen/include/asm-arm/cache.h
>> index 2de6564..af96eee 100644
>> --- a/xen/include/asm-arm/cache.h
>> +++ b/xen/include/asm-arm/cache.h
>> @@ -7,6 +7,10 @@
>> #define L1_CACHE_SHIFT (CONFIG_ARM_L1_CACHE_SHIFT)
>> #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
>>
>> +#ifndef __ASSEMBLY__
>> +void __flush_dcache_area(const void *vaddr, unsigned long size);
>> +#endif
>
> I think it might be best to add it to xen/include/asm-arm/arm64/page.h
Apart from the fact that it's not needed atm on ARM, is there any other
reason to confine it to ARM64?
Cheers,
Andre.
>> #define __read_mostly __section(".data.read_mostly")
>>
>> #endif
>> diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
>> index 2f5c51c..a66b6be 100644
>> --- a/xen/include/asm-arm/gic-its.h
>> +++ b/xen/include/asm-arm/gic-its.h
>> @@ -36,12 +36,32 @@ extern struct list_head host_its_list;
>> /* Parse the host DT and pick up all host ITSes. */
>> void gicv3_its_dt_init(const struct dt_device_node *node);
>>
>> +/* Allocate and initialize tables for each host redistributor.
>> + * Returns the respective {PROP,PEND}BASER register value.
>> + */
>> +uint64_t gicv3_lpi_get_proptable(void);
>> +uint64_t gicv3_lpi_allocate_pendtable(void);
>> +
>> +/* Initialize the host structures for LPIs. */
>> +int gicv3_lpi_init_host_lpis(unsigned int nr_lpis);
>> +
>> #else
>>
>> static inline void gicv3_its_dt_init(const struct dt_device_node *node)
>> {
>> }
>> -
>> +static inline uint64_t gicv3_lpi_get_proptable(void)
>> +{
>> + return 0;
>> +}
>> +static inline uint64_t gicv3_lpi_allocate_pendtable(void)
>> +{
>> + return 0;
>> +}
>> +static inline int gicv3_lpi_init_host_lpis(unsigned int nr_lpis)
>> +{
>> + return 0;
>> +}
>> #endif /* CONFIG_HAS_ITS */
>>
>> #endif /* __ASSEMBLY__ */
>> diff --git a/xen/include/asm-arm/gic_v3_defs.h
>> b/xen/include/asm-arm/gic_v3_defs.h
>> index 6bd25a5..da5fb77 100644
>> --- a/xen/include/asm-arm/gic_v3_defs.h
>> +++ b/xen/include/asm-arm/gic_v3_defs.h
>> @@ -44,7 +44,8 @@
>> #define GICC_SRE_EL2_ENEL1 (1UL << 3)
>>
>> /* Additional bits in GICD_TYPER defined by GICv3 */
>> -#define GICD_TYPE_ID_BITS_SHIFT 19
>> +#define GICD_TYPE_ID_BITS_SHIFT 19
>> +#define GICD_TYPE_LPIS (1U << 17)
>>
>> #define GICD_CTLR_RWP (1UL << 31)
>> #define GICD_CTLR_ARE_NS (1U << 4)
>> @@ -95,12 +96,57 @@
>> #define GICR_IGRPMODR0 (0x0D00)
>> #define GICR_NSACR (0x0E00)
>>
>> +#define GICR_CTLR_ENABLE_LPIS (1U << 0)
>> #define GICR_TYPER_PLPIS (1U << 0)
>> #define GICR_TYPER_VLPIS (1U << 1)
>> #define GICR_TYPER_LAST (1U << 4)
>>
>> +#define GIC_BASER_CACHE_nCnB 0ULL
>> +#define GIC_BASER_CACHE_SameAsInner 0ULL
>> +#define GIC_BASER_CACHE_nC 1ULL
>> +#define GIC_BASER_CACHE_RaWt 2ULL
>> +#define GIC_BASER_CACHE_RaWb 3ULL
>> +#define GIC_BASER_CACHE_WaWt 4ULL
>> +#define GIC_BASER_CACHE_WaWb 5ULL
>> +#define GIC_BASER_CACHE_RaWaWt 6ULL
>> +#define GIC_BASER_CACHE_RaWaWb 7ULL
>> +#define GIC_BASER_CACHE_MASK 7ULL
>> +#define GIC_BASER_NonShareable 0ULL
>> +#define GIC_BASER_InnerShareable 1ULL
>> +#define GIC_BASER_OuterShareable 2ULL
>> +
>> +#define GICR_PROPBASER_SHAREABILITY_SHIFT 10
>> +#define GICR_PROPBASER_INNER_CACHEABILITY_SHIFT 7
>> +#define GICR_PROPBASER_OUTER_CACHEABILITY_SHIFT 56
>> +#define GICR_PROPBASER_SHAREABILITY_MASK \
>> + (3UL << GICR_PROPBASER_SHAREABILITY_SHIFT)
>> +#define GICR_PROPBASER_INNER_CACHEABILITY_MASK \
>> + (7UL << GICR_PROPBASER_INNER_CACHEABILITY_SHIFT)
>> +#define GICR_PROPBASER_OUTER_CACHEABILITY_MASK \
>> + (7UL << GICR_PROPBASER_OUTER_CACHEABILITY_SHIFT)
>> +#define PROPBASER_RES0_MASK \
>> + (GENMASK(63, 59) | GENMASK(55, 52) | GENMASK(6, 5))
>> +
>> +#define GICR_PENDBASER_SHAREABILITY_SHIFT 10
>> +#define GICR_PENDBASER_INNER_CACHEABILITY_SHIFT 7
>> +#define GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT 56
>> +#define GICR_PENDBASER_SHAREABILITY_MASK \
>> + (3UL << GICR_PENDBASER_SHAREABILITY_SHIFT)
>> +#define GICR_PENDBASER_INNER_CACHEABILITY_MASK \
>> + (7UL << GICR_PENDBASER_INNER_CACHEABILITY_SHIFT)
>> +#define GICR_PENDBASER_OUTER_CACHEABILITY_MASK \
>> + (7UL << GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT)
>> +#define GICR_PENDBASER_PTZ BIT(62)
>> +#define PENDBASER_RES0_MASK \
>> + (BIT(63) | GENMASK(61, 59) | GENMASK(55, 52) | \
>> + GENMASK(15, 12) | GENMASK(6, 0))
>> +
>> #define DEFAULT_PMR_VALUE 0xff
>>
>> +#define LPI_PROP_DEFAULT_PRIO 0xa0
>> +#define LPI_PROP_RES1 (1 << 1)
>> +#define LPI_PROP_ENABLED (1 << 0)
>> +
>> #define GICH_VMCR_EOI (1 << 9)
>> #define GICH_VMCR_VENG1 (1 << 1)
>>
>> --
>> 2.9.0
>>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |