| Message ID | 20170130183153.28566-4-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andre, On 30/01/17 18:31, Andre Przywara wrote: > The ARM GICv3 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 > redistributor. 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@arm.com> > --- > xen/arch/arm/Kconfig | 15 +++++ > xen/arch/arm/Makefile | 1 + > xen/arch/arm/gic-v3-lpi.c | 129 ++++++++++++++++++++++++++++++++++++++ > xen/arch/arm/gic-v3.c | 44 +++++++++++++ > xen/include/asm-arm/bitops.h | 1 + > xen/include/asm-arm/gic.h | 2 + > xen/include/asm-arm/gic_v3_defs.h | 52 ++++++++++++++- > xen/include/asm-arm/gic_v3_its.h | 22 ++++++- > 8 files changed, 264 insertions(+), 2 deletions(-) > create mode 100644 xen/arch/arm/gic-v3-lpi.c > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig > index bf64c61..71734a1 100644 > --- a/xen/arch/arm/Kconfig > +++ b/xen/arch/arm/Kconfig > @@ -49,6 +49,21 @@ config HAS_ITS > bool "GICv3 ITS MSI controller support" > depends on HAS_GICV3 > > +config MAX_PHYS_LPI_BITS > + 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 (in bits) Xen should take > + care of. The host ITS may provide support for a very large number > + of supported LPIs, for all of which we may not want to allocate > + memory, so this number here allows to limit this. I think the description is misleading, if a user wants 8K worth of LPIs by default, he would have to use 14 and not 13. Furthermore, you provide both a runtime option (via command line) and build time option (via Kconfig). You don't express what is the differences between both and how there are supposed to co-exist. Anyway, IHMO the command line option should be sufficient to allow override if necessary. So I would drop the Kconfig version. > + Xen itself does not know how many LPIs domains will ever need > + beforehand. > + This can be overriden on the command line with the max_lpi_bits s/overriden/overridden/ > + parameter. > + > endmenu > > menu "ARM errata workaround via the alternative framework" > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > index 5f4ff23..4ccf2eb 100644 > --- a/xen/arch/arm/Makefile > +++ b/xen/arch/arm/Makefile > @@ -19,6 +19,7 @@ obj-y += gic.o > obj-y += gic-v2.o > obj-$(CONFIG_HAS_GICV3) += gic-v3.o > obj-$(CONFIG_HAS_ITS) += gic-v3-its.o > +obj-$(CONFIG_HAS_ITS) += gic-v3-lpi.o > obj-y += guestcopy.o > obj-y += hvm.o > obj-y += io.o > diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c > new file mode 100644 > index 0000000..e2fc901 > --- /dev/null > +++ b/xen/arch/arm/gic-v3-lpi.c > @@ -0,0 +1,129 @@ > +/* > + * xen/arch/arm/gic-v3-lpi.c > + * > + * ARM GICv3 Locality-specific Peripheral Interrupts (LPI) support > + * > + * Copyright (C) 2016,2017 - ARM Ltd > + * > + * 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. > + */ > + > +#include <xen/config.h> xen/config.h is not necessary. > +#include <xen/lib.h> > +#include <xen/mm.h> > +#include <xen/sizes.h> > +#include <asm/gic.h> > +#include <asm/gic_v3_defs.h> > +#include <asm/gic_v3_its.h> > + > +/* Global state */ > +static struct { > + uint8_t *lpi_property; > + unsigned int host_lpi_bits; On the previous version, Stefano suggested to rename this to phys_lpi_bits + adding a comment as you store the number of bits. However, looking at the usage the number of bits is only required during the initialization. Runtime code (such as gic_get_host_lpi) will use the number of LPIs (see gic_get_host_lpi) and therefore require extra instructions to compute the value. So I would prefer if you store the number of LPIs here to optimize the common case. Also, I find the naming "id_bits" confusing because you store the number of bits to encode the max LPI ID and not the number of bits to encode the number of LPI. > +} 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) - LPI_OFFSET) > + > +uint64_t gicv3_lpi_allocate_pendtable(void) > +{ > + uint64_t reg; > + void *pendtable; > + > + reg = GIC_BASER_CACHE_RaWaWb << GICR_PENDBASER_INNER_CACHEABILITY_SHIFT; > + reg |= GIC_BASER_CACHE_SameAsInner << GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT; > + reg |= GIC_BASER_InnerShareable << GICR_PENDBASER_SHAREABILITY_SHIFT; > + > + if ( !this_cpu(pending_table) ) > + { > + /* > + * The pending table holds one bit per LPI and even covers bits for > + * interrupt IDs below 8192, so we allocate the full range. > + * The GICv3 imposes a 64KB alignment requirement. > + */ > + pendtable = _xmalloc(BIT_ULL(lpi_data.host_lpi_bits) / 8, SZ_64K); > + if ( !pendtable ) > + return 0; > + > + memset(pendtable, 0, BIT_ULL(lpi_data.host_lpi_bits) / 8); You can use _zalloc to do the allocation and then memset to 0. > + __flush_dcache_area(pendtable, BIT_ULL(lpi_data.host_lpi_bits) / 8); Please use clean_and_invalidate_dcache_va_range. > + > + this_cpu(pending_table) = pendtable; > + } > + else > + { > + pendtable = this_cpu(pending_table); > + } The {} are not necessary. Also, on the previous version it was mentioned this should be an error and then replace by a BUG_ON(). Please do the change. > + > + reg |= GICR_PENDBASER_PTZ; > + > + ASSERT(!(virt_to_maddr(pendtable) & ~GENMASK(51, 16))); I don't understand the purpose of this ASSERT. The bits 15:0 should always be zero otherwise this would be a bug in the memory allocator. For bits 64:52, the architecture so far only support up to 52 bits. By keeping this ASSERT, you will make our life more difficult to extend the number of physical address supported if ARM decides to bump it. So please drop this ASSERT. > + reg |= virt_to_maddr(pendtable); > + > + return reg; > +} > + > +uint64_t gicv3_lpi_get_proptable(void) > +{ > + uint64_t reg; > + > + reg = GIC_BASER_CACHE_RaWaWb << GICR_PENDBASER_INNER_CACHEABILITY_SHIFT; > + reg |= GIC_BASER_CACHE_SameAsInner << GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT; > + reg |= GIC_BASER_InnerShareable << GICR_PENDBASER_SHAREABILITY_SHIFT; You are using the shift defines from PENDBASER and not PROPBASER. > + > + /* > + * 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); The property table address has to be 4KB aligned right? If so, I would much prefer if you use _xmalloc(BIT_ULL(lpi_data.host_lpi_bits), SZ_4K) to avoid relying on PAGE_SIZE == 4KB. Also, you will allocate more memory than necessary because the property table only covers the LPIs. > + > + if ( !table ) > + return 0; > + > + memset(table, GIC_PRI_IRQ | LPI_PROP_RES1, MAX_PHYS_LPIS); You could combine both suggested _xmalloc and memset to 0 in a single call to _zalloc. > + __flush_dcache_area(table, MAX_PHYS_LPIS); Please use clean_and_invalidate_dcache_va_range. > + lpi_data.lpi_property = table; > + } > + > + reg |= ((lpi_data.host_lpi_bits - 1) << 0); Please avoid hardcoded shift. > + > + ASSERT(!(virt_to_maddr(lpi_data.lpi_property) & ~GENMASK(51, 12))); > + reg |= virt_to_maddr(lpi_data.lpi_property); > + > + return reg; > +} > + > +static unsigned int max_lpi_bits = CONFIG_MAX_PHYS_LPI_BITS; > +integer_param("max_lpi_bits", max_lpi_bits); Please document this new option in docs/misc/xen-command-line.markdown. > + > +int gicv3_lpi_init_host_lpis(unsigned int hw_lpi_bits) Stefano suggested to rename this function to gicv3_lpi_init_phys_lpis and I agree with him here. > +{ > + 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); s/lld/llu/. > + > + return 0; > +} > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */ > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c > index 838dd11..fcb86c8 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); A macro has been suggested on the previous version here to avoid the hardcoded 0x1f. > + > printk("GICv3: %d lines, (IID %8.8x).\n", > nr_lines, readl_relaxed(GICD + GICD_IIDR)); > > @@ -616,6 +619,33 @@ static int gicv3_enable_redist(void) > return 0; > } > > +static int gicv3_rdist_init_lpis(void __iomem * rdist_base) I think it would make sense to move this function in gicv3-lpi.c. So only one function rather than 2 would be exported. > +{ > + uint32_t reg; > + uint64_t table_reg; > + > + /* We don't support LPIs without an ITS. */ > + if ( list_empty(&host_its_list) ) See my comment on patch #2 regarding host_its_list. > + return -ENODEV; > + > + /* Make sure LPIs are disabled before setting up the tables. */ > + reg = readl_relaxed(rdist_base + GICR_CTLR); > + if ( reg & GICR_CTLR_ENABLE_LPIS ) > + return -EBUSY; Why don't you just disable LPIs here? AFAIK, it should just be writel_relaxed(reg & ~GICR_CTLR_ENABLE_LPIS, GICR_CTLR); > + > + table_reg = gicv3_lpi_allocate_pendtable(); > + if ( !table_reg ) From the spec, GICR_PENDBASER full of 0 is valid. > + return -ENOMEM; > + writeq_relaxed(table_reg, rdist_base + GICR_PENDBASER); On the first version of this series, I mentioned that based on the spec (8.11.18 in ARM IHI 0069C) cacheability and shareability may not stick. Whilst this may not (?) be a concern for the pending table, Xen will write in the property table to enable/disable LPIs. So we would need to know whether the cache needs to be cleaned after each access or not. > + > + table_reg = gicv3_lpi_get_proptable(); > + if ( !table_reg ) > + return -ENOMEM; > + writeq_relaxed(table_reg, rdist_base + GICR_PROPBASER); See all my remarks above. > + > + return 0; > +} > + > static int __init gicv3_populate_rdist(void) > { > int i; > @@ -658,6 +688,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", CPU%u > + 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..1cbfb9e 100644 > --- a/xen/include/asm-arm/bitops.h > +++ b/xen/include/asm-arm/bitops.h > @@ -24,6 +24,7 @@ > #define BIT(nr) (1UL << (nr)) > #define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_WORD)) > #define BIT_WORD(nr) ((nr) / BITS_PER_WORD) > +#define BIT_ULL(nr) (1ULL << (nr)) > #define BITS_PER_BYTE 8 > > #define ADDR (*(volatile int *) addr) > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index 836a103..12bd155 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -220,6 +220,8 @@ enum gic_version { > GIC_V3, > }; > > +#define LPI_OFFSET 8192 > + It would make much sense to have this definition moved in irq.h close to NR_IRQS. Also, I am a bit surprised that NR_IRQS & co has not been modified. Is there any reason for that? Regards,
On Mon, 30 Jan 2017, Andre Przywara wrote: > The ARM GICv3 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 > redistributor. 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@arm.com> > --- > xen/arch/arm/Kconfig | 15 +++++ > xen/arch/arm/Makefile | 1 + > xen/arch/arm/gic-v3-lpi.c | 129 ++++++++++++++++++++++++++++++++++++++ > xen/arch/arm/gic-v3.c | 44 +++++++++++++ > xen/include/asm-arm/bitops.h | 1 + > xen/include/asm-arm/gic.h | 2 + > xen/include/asm-arm/gic_v3_defs.h | 52 ++++++++++++++- > xen/include/asm-arm/gic_v3_its.h | 22 ++++++- > 8 files changed, 264 insertions(+), 2 deletions(-) > create mode 100644 xen/arch/arm/gic-v3-lpi.c > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig > index bf64c61..71734a1 100644 > --- a/xen/arch/arm/Kconfig > +++ b/xen/arch/arm/Kconfig > @@ -49,6 +49,21 @@ config HAS_ITS > bool "GICv3 ITS MSI controller support" > depends on HAS_GICV3 > > +config MAX_PHYS_LPI_BITS > + 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 (in bits) Xen should take > + care of. The host ITS may provide support for a very large number > + of supported LPIs, for all of which we may not want to allocate > + memory, so this number here allows to limit this. > + Xen itself does not know how many LPIs domains will ever need > + beforehand. > + 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/Makefile b/xen/arch/arm/Makefile > index 5f4ff23..4ccf2eb 100644 > --- a/xen/arch/arm/Makefile > +++ b/xen/arch/arm/Makefile > @@ -19,6 +19,7 @@ obj-y += gic.o > obj-y += gic-v2.o > obj-$(CONFIG_HAS_GICV3) += gic-v3.o > obj-$(CONFIG_HAS_ITS) += gic-v3-its.o > +obj-$(CONFIG_HAS_ITS) += gic-v3-lpi.o > obj-y += guestcopy.o > obj-y += hvm.o > obj-y += io.o > diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c > new file mode 100644 > index 0000000..e2fc901 > --- /dev/null > +++ b/xen/arch/arm/gic-v3-lpi.c > @@ -0,0 +1,129 @@ > +/* > + * xen/arch/arm/gic-v3-lpi.c > + * > + * ARM GICv3 Locality-specific Peripheral Interrupts (LPI) support > + * > + * Copyright (C) 2016,2017 - ARM Ltd > + * > + * 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. > + */ > + > +#include <xen/config.h> > +#include <xen/lib.h> > +#include <xen/mm.h> > +#include <xen/sizes.h> > +#include <asm/gic.h> > +#include <asm/gic_v3_defs.h> > +#include <asm/gic_v3_its.h> > + > +/* Global state */ > +static struct { > + uint8_t *lpi_property; > + unsigned int host_lpi_bits; It's still missing a comment > +} 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) - LPI_OFFSET) > + > +uint64_t gicv3_lpi_allocate_pendtable(void) > +{ > + uint64_t reg; > + void *pendtable; > + > + reg = GIC_BASER_CACHE_RaWaWb << GICR_PENDBASER_INNER_CACHEABILITY_SHIFT; > + reg |= GIC_BASER_CACHE_SameAsInner << GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT; > + reg |= GIC_BASER_InnerShareable << GICR_PENDBASER_SHAREABILITY_SHIFT; > + > + if ( !this_cpu(pending_table) ) > + { > + /* > + * The pending table holds one bit per LPI and even covers bits for > + * interrupt IDs below 8192, so we allocate the full range. > + * The GICv3 imposes a 64KB alignment requirement. > + */ > + pendtable = _xmalloc(BIT_ULL(lpi_data.host_lpi_bits) / 8, SZ_64K); > + 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); it's still missing a BUG_ON > + } > + > + reg |= GICR_PENDBASER_PTZ; > + > + ASSERT(!(virt_to_maddr(pendtable) & ~GENMASK(51, 16))); > + reg |= virt_to_maddr(pendtable); > + > + return reg; > +} > + > +uint64_t gicv3_lpi_get_proptable(void) > +{ > + uint64_t reg; > + > + reg = GIC_BASER_CACHE_RaWaWb << GICR_PENDBASER_INNER_CACHEABILITY_SHIFT; > + reg |= GIC_BASER_CACHE_SameAsInner << GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT; > + reg |= 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 |= ((lpi_data.host_lpi_bits - 1) << 0); > + > + ASSERT(!(virt_to_maddr(lpi_data.lpi_property) & ~GENMASK(51, 12))); > + reg |= virt_to_maddr(lpi_data.lpi_property); > + > + return reg; > +} > + > +static unsigned int max_lpi_bits = CONFIG_MAX_PHYS_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 > +{ > + 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; > +} > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */ > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c > index 838dd11..fcb86c8 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); > + > printk("GICv3: %d lines, (IID %8.8x).\n", > nr_lines, readl_relaxed(GICD + GICD_IIDR)); > > @@ -616,6 +619,33 @@ 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; > + > + /* We don't support LPIs without an ITS. */ > + if ( list_empty(&host_its_list) ) > + return -ENODEV; > + > + /* Make sure LPIs are disabled before setting up the tables. */ > + 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 +688,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..1cbfb9e 100644 > --- a/xen/include/asm-arm/bitops.h > +++ b/xen/include/asm-arm/bitops.h > @@ -24,6 +24,7 @@ > #define BIT(nr) (1UL << (nr)) > #define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_WORD)) > #define BIT_WORD(nr) ((nr) / BITS_PER_WORD) > +#define BIT_ULL(nr) (1ULL << (nr)) > #define BITS_PER_BYTE 8 > > #define ADDR (*(volatile int *) addr) > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index 836a103..12bd155 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -220,6 +220,8 @@ enum gic_version { > GIC_V3, > }; > > +#define LPI_OFFSET 8192 > + > extern enum gic_version gic_hw_version(void); > > /* Program the IRQ type into the GIC */ > diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h > index 6bd25a5..b307322 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,61 @@ > #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) > > +/* For specifying the inner cacheability type only */ > +#define GIC_BASER_CACHE_nCnB 0ULL > +/* For specifying the outer cacheability type only */ > +#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 GICR_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 GICR_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_PRIO_MASK 0xfc > +#define LPI_PROP_RES1 (1 << 1) > +#define LPI_PROP_ENABLED (1 << 0) > + > #define GICH_VMCR_EOI (1 << 9) > #define GICH_VMCR_VENG1 (1 << 1) > > diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h > index 2f5c51c..a66b6be 100644 > --- a/xen/include/asm-arm/gic_v3_its.h > +++ b/xen/include/asm-arm/gic_v3_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__ */ > -- > 2.9.0 >
Hi, "Yes, will fix" to everything not explicitly mentioned below. On 06/02/17 16:26, Julien Grall wrote: > Hi Andre, > > On 30/01/17 18:31, Andre Przywara wrote: >> The ARM GICv3 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 >> redistributor. 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@arm.com> >> --- >> xen/arch/arm/Kconfig | 15 +++++ >> xen/arch/arm/Makefile | 1 + >> xen/arch/arm/gic-v3-lpi.c | 129 >> ++++++++++++++++++++++++++++++++++++++ >> xen/arch/arm/gic-v3.c | 44 +++++++++++++ >> xen/include/asm-arm/bitops.h | 1 + >> xen/include/asm-arm/gic.h | 2 + >> xen/include/asm-arm/gic_v3_defs.h | 52 ++++++++++++++- >> xen/include/asm-arm/gic_v3_its.h | 22 ++++++- >> 8 files changed, 264 insertions(+), 2 deletions(-) >> create mode 100644 xen/arch/arm/gic-v3-lpi.c >> >> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig >> index bf64c61..71734a1 100644 >> --- a/xen/arch/arm/Kconfig >> +++ b/xen/arch/arm/Kconfig >> @@ -49,6 +49,21 @@ config HAS_ITS >> bool "GICv3 ITS MSI controller support" >> depends on HAS_GICV3 >> >> +config MAX_PHYS_LPI_BITS >> + 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 (in bits) Xen should take >> + care of. The host ITS may provide support for a very large >> number >> + of supported LPIs, for all of which we may not want to >> allocate >> + memory, so this number here allows to limit this. > > I think the description is misleading, if a user wants 8K worth of LPIs > by default, he would have to use 14 and not 13. > > Furthermore, you provide both a runtime option (via command line) and > build time option (via Kconfig). You don't express what is the > differences between both and how there are supposed to co-exist. > > Anyway, IHMO the command line option should be sufficient to allow > override if necessary. So I would drop the Kconfig version. The idea is simply to let the Kconfig version specify the default value if there is no command line option. So giving a command line option will always override Kconfig. Should we know a sensible default value, we can indeed get rid of this Kconfig snippet here. >> + Xen itself does not know how many LPIs domains will ever need >> + beforehand. >> + This can be overriden on the command line with the >> max_lpi_bits > > s/overriden/overridden/ > >> + parameter. >> + >> endmenu >> >> menu "ARM errata workaround via the alternative framework" >> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile >> index 5f4ff23..4ccf2eb 100644 >> --- a/xen/arch/arm/Makefile >> +++ b/xen/arch/arm/Makefile >> @@ -19,6 +19,7 @@ obj-y += gic.o >> obj-y += gic-v2.o >> obj-$(CONFIG_HAS_GICV3) += gic-v3.o >> obj-$(CONFIG_HAS_ITS) += gic-v3-its.o >> +obj-$(CONFIG_HAS_ITS) += gic-v3-lpi.o >> obj-y += guestcopy.o >> obj-y += hvm.o >> obj-y += io.o >> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c >> new file mode 100644 >> index 0000000..e2fc901 >> --- /dev/null >> +++ b/xen/arch/arm/gic-v3-lpi.c >> @@ -0,0 +1,129 @@ >> +/* >> + * xen/arch/arm/gic-v3-lpi.c >> + * >> + * ARM GICv3 Locality-specific Peripheral Interrupts (LPI) support >> + * >> + * Copyright (C) 2016,2017 - ARM Ltd >> + * >> + * 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. >> + */ >> + >> +#include <xen/config.h> > > xen/config.h is not necessary. > >> +#include <xen/lib.h> >> +#include <xen/mm.h> >> +#include <xen/sizes.h> >> +#include <asm/gic.h> >> +#include <asm/gic_v3_defs.h> >> +#include <asm/gic_v3_its.h> >> + >> +/* Global state */ >> +static struct { >> + uint8_t *lpi_property; >> + unsigned int host_lpi_bits; > > On the previous version, Stefano suggested to rename this to > phys_lpi_bits + adding a comment as you store the number of bits. > > However, looking at the usage the number of bits is only required during > the initialization. Runtime code (such as gic_get_host_lpi) will use the > number of LPIs (see gic_get_host_lpi) and therefore require extra > instructions to compute the value. > > So I would prefer if you store the number of LPIs here to optimize the > common case. Well, it's a shift, not the 5th root. This is in fact the only difference between the two approaches: 000000000024d770 <gic_get_host_lpi>: ... 24d788: 9ac22421 lsr x1, x1, x2 And I was thinking about it before, my rationale for not doing it was: - We need both the number and the shift, and it's much easier to get the number from the bits than the other way around. - The bits are the real source of the information (from TYPER). - Having a number would always raise the question whether we need to make sure that there is more than one bit set in there and how to deal with it. > Also, I find the naming "id_bits" confusing because you store the number > of bits to encode the max LPI ID and not the number of bits to encode > the number of LPI. "IDbits" is the spec term used. It describes how many bits you need to describe an LPI number. LPIs start at number 8192. GICv3 spec, 8.9.24: IDbits, bits [23:19] The number of interrupt identifier bits supported, minus one. I can rename this to "phys_lpi_idbits", if that sounds reasonable. >> +} 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) - LPI_OFFSET) >> + >> +uint64_t gicv3_lpi_allocate_pendtable(void) >> +{ >> + uint64_t reg; >> + void *pendtable; >> + >> + reg = GIC_BASER_CACHE_RaWaWb << >> GICR_PENDBASER_INNER_CACHEABILITY_SHIFT; >> + reg |= GIC_BASER_CACHE_SameAsInner << >> GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT; >> + reg |= GIC_BASER_InnerShareable << >> GICR_PENDBASER_SHAREABILITY_SHIFT; >> + >> + if ( !this_cpu(pending_table) ) >> + { >> + /* >> + * The pending table holds one bit per LPI and even covers >> bits for >> + * interrupt IDs below 8192, so we allocate the full range. >> + * The GICv3 imposes a 64KB alignment requirement. >> + */ >> + pendtable = _xmalloc(BIT_ULL(lpi_data.host_lpi_bits) / 8, >> SZ_64K); >> + if ( !pendtable ) >> + return 0; >> + >> + memset(pendtable, 0, BIT_ULL(lpi_data.host_lpi_bits) / 8); > > You can use _zalloc to do the allocation and then memset to 0. > >> + __flush_dcache_area(pendtable, >> BIT_ULL(lpi_data.host_lpi_bits) / 8); > > Please use clean_and_invalidate_dcache_va_range. > >> + >> + this_cpu(pending_table) = pendtable; >> + } >> + else >> + { >> + pendtable = this_cpu(pending_table); >> + } > > The {} are not necessary. Sure, this was coming from the Linux rule here: if the if-clause requires braces, the else-clause has to have those too. To me it looks weird otherwise. Xen's coding style document doesn't explicitly say. > Also, on the previous version it was mentioned > this should be an error and then replace by a BUG_ON(). I don't see how this would be an actual bug. Yes, the code as it is right now calls this only once, but it wouldn't hurt if we call this multiple times. And I am always a bit wary of crashing the system if we could just carry on instead, but ... > Please do the change. ... however you like. >> + >> + reg |= GICR_PENDBASER_PTZ; >> + >> + ASSERT(!(virt_to_maddr(pendtable) & ~GENMASK(51, 16))); > > I don't understand the purpose of this ASSERT. The bits 15:0 should > always be zero otherwise this would be a bug in the memory allocator. > For bits 64:52, the architecture so far only support up to 52 bits. You complained about using a mask on the address before, which I can understand. However we are writing to a register described in the spec here and should observe that the address only goes into bits [51:16]. Any other bits should not be touched or we are getting weird errors. So somehow I have to make sure we comply with this. This could either be a mask or an ASSERT. If the assert never fires: great. Nothing to worry about here. But I think this matches the ASSERT idea: we rely on this address being 4K aligned and not exceeding 52 bits worth of address bits, so we should check these assumptions. > By keeping this ASSERT, you will make our life more difficult to extend > the number of physical address supported if ARM decides to bump it. This GICR register is limited to 52 bits of physical address space according to the spec. If we ever upgrade the address size, the GIC spec would need to be upgraded as well, so chances are we either need to touch that code here anyways or we use a GICv5 or the like from the beginning. > So please drop this ASSERT. > >> + reg |= virt_to_maddr(pendtable); >> + >> + return reg; >> +} >> + >> +uint64_t gicv3_lpi_get_proptable(void) >> +{ >> + uint64_t reg; >> + >> + reg = GIC_BASER_CACHE_RaWaWb << >> GICR_PENDBASER_INNER_CACHEABILITY_SHIFT; >> + reg |= GIC_BASER_CACHE_SameAsInner << >> GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT; >> + reg |= GIC_BASER_InnerShareable << >> GICR_PENDBASER_SHAREABILITY_SHIFT; > > You are using the shift defines from PENDBASER and not PROPBASER. Doh. I thought I fixed this. >> + >> + /* >> + * 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); > > The property table address has to be 4KB aligned right? If so, I would > much prefer if you use _xmalloc(BIT_ULL(lpi_data.host_lpi_bits), SZ_4K) > to avoid relying on PAGE_SIZE == 4KB. > > Also, you will allocate more memory than necessary because the property > table only covers the LPIs. > >> + >> + if ( !table ) >> + return 0; >> + >> + memset(table, GIC_PRI_IRQ | LPI_PROP_RES1, MAX_PHYS_LPIS); > > You could combine both suggested _xmalloc and memset to 0 in a single > call to _zalloc. > >> + __flush_dcache_area(table, MAX_PHYS_LPIS); > > Please use clean_and_invalidate_dcache_va_range. > >> + lpi_data.lpi_property = table; >> + } >> + >> + reg |= ((lpi_data.host_lpi_bits - 1) << 0); > > Please avoid hardcoded shift. > >> + >> + ASSERT(!(virt_to_maddr(lpi_data.lpi_property) & ~GENMASK(51, 12))); >> + reg |= virt_to_maddr(lpi_data.lpi_property); >> + >> + return reg; >> +} >> + >> +static unsigned int max_lpi_bits = CONFIG_MAX_PHYS_LPI_BITS; >> +integer_param("max_lpi_bits", max_lpi_bits); > > Please document this new option in docs/misc/xen-command-line.markdown. > >> + >> +int gicv3_lpi_init_host_lpis(unsigned int hw_lpi_bits) > > Stefano suggested to rename this function to gicv3_lpi_init_phys_lpis > and I agree with him here. > >> +{ >> + 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); > > s/lld/llu/. > >> + >> + return 0; >> +} >> + >> +/* >> + * Local variables: >> + * mode: C >> + * c-file-style: "BSD" >> + * c-basic-offset: 4 >> + * indent-tabs-mode: nil >> + * End: >> + */ >> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c >> index 838dd11..fcb86c8 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); > > A macro has been suggested on the previous version here to avoid the > hardcoded 0x1f. > >> + >> printk("GICv3: %d lines, (IID %8.8x).\n", >> nr_lines, readl_relaxed(GICD + GICD_IIDR)); >> >> @@ -616,6 +619,33 @@ static int gicv3_enable_redist(void) >> return 0; >> } >> >> +static int gicv3_rdist_init_lpis(void __iomem * rdist_base) > > I think it would make sense to move this function in gicv3-lpi.c. So > only one function rather than 2 would be exported. > >> +{ >> + uint32_t reg; >> + uint64_t table_reg; >> + >> + /* We don't support LPIs without an ITS. */ >> + if ( list_empty(&host_its_list) ) > > See my comment on patch #2 regarding host_its_list. > >> + return -ENODEV; >> + >> + /* Make sure LPIs are disabled before setting up the tables. */ >> + reg = readl_relaxed(rdist_base + GICR_CTLR); >> + if ( reg & GICR_CTLR_ENABLE_LPIS ) >> + return -EBUSY; > > Why don't you just disable LPIs here? AFAIK, it should just be > writel_relaxed(reg & ~GICR_CTLR_ENABLE_LPIS, GICR_CTLR); Please don't shoot the messenger, but: GICv3 spec 8.11.2 "GICR_CTLR, Redistributor Control Register": Enable_LPIs, bit [0]: "... When a write changes this bit from 0 to 1, this bit becomes RES1 and the Redistributor must load the LPI Pending table from memory to check for any pending interrupts." Read: LPIs are so great that you can't disable them anymore once you have enabled them. Yes, this is a bit weird, even has nasty side effects. >> + >> + table_reg = gicv3_lpi_allocate_pendtable(); >> + if ( !table_reg ) > > From the spec, GICR_PENDBASER full of 0 is valid. Meh. Changed it to take an uint64_t * and return an error code. >> + return -ENOMEM; >> + writeq_relaxed(table_reg, rdist_base + GICR_PENDBASER); > > On the first version of this series, I mentioned that based on the spec > (8.11.18 in ARM IHI 0069C) cacheability and shareability may not stick. > > Whilst this may not (?) be a concern for the pending table, Xen will > write in the property table to enable/disable LPIs. So we would need to > know whether the cache needs to be cleaned after each access or not. Ah, yes, I implemented this for the command queue, then realized that we need it for the PROPBASER as well, but it's a per-redistributor property. At this point I decided to split the code between -lpi.c and -its.c, which took me a day, making me loose my "mental return address", so I forget to go back and fix the actual issue ;-) Sorry for that, this is now fixed in my code base. >> + >> + table_reg = gicv3_lpi_get_proptable(); >> + if ( !table_reg ) >> + return -ENOMEM; >> + writeq_relaxed(table_reg, rdist_base + GICR_PROPBASER); > > See all my remarks above. > >> + >> + return 0; >> +} >> + >> static int __init gicv3_populate_rdist(void) >> { >> int i; >> @@ -658,6 +688,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", > > CPU%u > >> + 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..1cbfb9e 100644 >> --- a/xen/include/asm-arm/bitops.h >> +++ b/xen/include/asm-arm/bitops.h >> @@ -24,6 +24,7 @@ >> #define BIT(nr) (1UL << (nr)) >> #define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_WORD)) >> #define BIT_WORD(nr) ((nr) / BITS_PER_WORD) >> +#define BIT_ULL(nr) (1ULL << (nr)) >> #define BITS_PER_BYTE 8 >> >> #define ADDR (*(volatile int *) addr) >> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h >> index 836a103..12bd155 100644 >> --- a/xen/include/asm-arm/gic.h >> +++ b/xen/include/asm-arm/gic.h >> @@ -220,6 +220,8 @@ enum gic_version { >> GIC_V3, >> }; >> >> +#define LPI_OFFSET 8192 >> + > > It would make much sense to have this definition moved in irq.h close to > NR_IRQS. > > Also, I am a bit surprised that NR_IRQS & co has not been modified. Is > there any reason for that? It wasn't needed and really shouldn't be. LPI are to some degree *not* first class citizen IRQs, and AFAICT NR_IRQS relate to struct irq_decs's, which we don't have for LPIs (since Xen itself doesn't really care about LPIs, at least as interrupts). I am still chasing every (derived) use of NR_IRQS, but so far this looks good to me. Let me know if you find any issues with that. Cheers, Andre.
On 27/02/17 11:34, Andre Przywara wrote: > Hi, Hi Andre, > > "Yes, will fix" to everything not explicitly mentioned below. > > On 06/02/17 16:26, Julien Grall wrote: >> Hi Andre, >> >> On 30/01/17 18:31, Andre Przywara wrote: >>> The ARM GICv3 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 >>> redistributor. 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@arm.com> >>> --- >>> xen/arch/arm/Kconfig | 15 +++++ >>> xen/arch/arm/Makefile | 1 + >>> xen/arch/arm/gic-v3-lpi.c | 129 >>> ++++++++++++++++++++++++++++++++++++++ >>> xen/arch/arm/gic-v3.c | 44 +++++++++++++ >>> xen/include/asm-arm/bitops.h | 1 + >>> xen/include/asm-arm/gic.h | 2 + >>> xen/include/asm-arm/gic_v3_defs.h | 52 ++++++++++++++- >>> xen/include/asm-arm/gic_v3_its.h | 22 ++++++- >>> 8 files changed, 264 insertions(+), 2 deletions(-) >>> create mode 100644 xen/arch/arm/gic-v3-lpi.c >>> >>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig >>> index bf64c61..71734a1 100644 >>> --- a/xen/arch/arm/Kconfig >>> +++ b/xen/arch/arm/Kconfig >>> @@ -49,6 +49,21 @@ config HAS_ITS >>> bool "GICv3 ITS MSI controller support" >>> depends on HAS_GICV3 >>> >>> +config MAX_PHYS_LPI_BITS >>> + 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 (in bits) Xen should take >>> + care of. The host ITS may provide support for a very large >>> number >>> + of supported LPIs, for all of which we may not want to >>> allocate >>> + memory, so this number here allows to limit this. >> >> I think the description is misleading, if a user wants 8K worth of LPIs >> by default, he would have to use 14 and not 13. >> >> Furthermore, you provide both a runtime option (via command line) and >> build time option (via Kconfig). You don't express what is the >> differences between both and how there are supposed to co-exist. >> >> Anyway, IHMO the command line option should be sufficient to allow >> override if necessary. So I would drop the Kconfig version. > > The idea is simply to let the Kconfig version specify the default value > if there is no command line option. So giving a command line option will > always override Kconfig. > Should we know a sensible default value, we can indeed get rid of this > Kconfig snippet here. Please have in mind that distribution will ship one version of Xen for all the platforms. There is no sensible default value and I don't see how a distribution will be able to pick-up one. So I still don't see the point of this default option. > >>> + Xen itself does not know how many LPIs domains will ever need >>> + beforehand. >>> + This can be overriden on the command line with the >>> max_lpi_bits >> >> s/overriden/overridden/ >> >>> + parameter. >>> + >>> endmenu >>> >>> menu "ARM errata workaround via the alternative framework" >>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile >>> index 5f4ff23..4ccf2eb 100644 >>> --- a/xen/arch/arm/Makefile >>> +++ b/xen/arch/arm/Makefile >>> @@ -19,6 +19,7 @@ obj-y += gic.o >>> obj-y += gic-v2.o >>> obj-$(CONFIG_HAS_GICV3) += gic-v3.o >>> obj-$(CONFIG_HAS_ITS) += gic-v3-its.o >>> +obj-$(CONFIG_HAS_ITS) += gic-v3-lpi.o >>> obj-y += guestcopy.o >>> obj-y += hvm.o >>> obj-y += io.o >>> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c >>> new file mode 100644 >>> index 0000000..e2fc901 >>> --- /dev/null >>> +++ b/xen/arch/arm/gic-v3-lpi.c >>> @@ -0,0 +1,129 @@ >>> +/* >>> + * xen/arch/arm/gic-v3-lpi.c >>> + * >>> + * ARM GICv3 Locality-specific Peripheral Interrupts (LPI) support >>> + * >>> + * Copyright (C) 2016,2017 - ARM Ltd >>> + * >>> + * 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. >>> + */ >>> + >>> +#include <xen/config.h> >> >> xen/config.h is not necessary. >> >>> +#include <xen/lib.h> >>> +#include <xen/mm.h> >>> +#include <xen/sizes.h> >>> +#include <asm/gic.h> >>> +#include <asm/gic_v3_defs.h> >>> +#include <asm/gic_v3_its.h> >>> + >>> +/* Global state */ >>> +static struct { >>> + uint8_t *lpi_property; >>> + unsigned int host_lpi_bits; >> >> On the previous version, Stefano suggested to rename this to >> phys_lpi_bits + adding a comment as you store the number of bits. >> >> However, looking at the usage the number of bits is only required during >> the initialization. Runtime code (such as gic_get_host_lpi) will use the >> number of LPIs (see gic_get_host_lpi) and therefore require extra >> instructions to compute the value. >> >> So I would prefer if you store the number of LPIs here to optimize the >> common case. > > Well, it's a shift, not the 5th root. This is in fact the only > difference between the two approaches: > 000000000024d770 <gic_get_host_lpi>: > ... > 24d788: 9ac22421 lsr x1, x1, x2 One instruction can make a lot of differences when a function is executed hundred times. > And I was thinking about it before, my rationale for not doing it was: > - We need both the number and the shift, and it's much easier to get the > number from the bits than the other way around. But you only need the number of bits in the initialization. I don't care if the ITS initialization is little slower than the invert. > - The bits are the real source of the information (from TYPER). So? It is fine to use another way to store the value in Xen if it makes easier to use in most of the places. This will also be less confusing for the users. > - Having a number would always raise the question whether we need to > make sure that there is more than one bit set in there and how to deal > with it. It is not difficult to handle that. > >> Also, I find the naming "id_bits" confusing because you store the number >> of bits to encode the max LPI ID and not the number of bits to encode >> the number of LPI. > > "IDbits" is the spec term used. It describes how many bits you need to > describe an LPI number. LPIs start at number 8192. > GICv3 spec, 8.9.24: > IDbits, bits [23:19] > The number of interrupt identifier bits supported, minus one. > > I can rename this to "phys_lpi_idbits", if that sounds reasonable. > >>> +} 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) - LPI_OFFSET) >>> + >>> +uint64_t gicv3_lpi_allocate_pendtable(void) >>> +{ >>> + uint64_t reg; >>> + void *pendtable; >>> + >>> + reg = GIC_BASER_CACHE_RaWaWb << >>> GICR_PENDBASER_INNER_CACHEABILITY_SHIFT; >>> + reg |= GIC_BASER_CACHE_SameAsInner << >>> GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT; >>> + reg |= GIC_BASER_InnerShareable << >>> GICR_PENDBASER_SHAREABILITY_SHIFT; >>> + >>> + if ( !this_cpu(pending_table) ) >>> + { >>> + /* >>> + * The pending table holds one bit per LPI and even covers >>> bits for >>> + * interrupt IDs below 8192, so we allocate the full range. >>> + * The GICv3 imposes a 64KB alignment requirement. >>> + */ >>> + pendtable = _xmalloc(BIT_ULL(lpi_data.host_lpi_bits) / 8, >>> SZ_64K); >>> + if ( !pendtable ) >>> + return 0; >>> + >>> + memset(pendtable, 0, BIT_ULL(lpi_data.host_lpi_bits) / 8); >> >> You can use _zalloc to do the allocation and then memset to 0. >> >>> + __flush_dcache_area(pendtable, >>> BIT_ULL(lpi_data.host_lpi_bits) / 8); >> >> Please use clean_and_invalidate_dcache_va_range. >> >>> + >>> + this_cpu(pending_table) = pendtable; >>> + } >>> + else >>> + { >>> + pendtable = this_cpu(pending_table); >>> + } >> >> The {} are not necessary. > > Sure, this was coming from the Linux rule here: if the if-clause > requires braces, the else-clause has to have those too. To me it looks > weird otherwise. Xen's coding style document doesn't explicitly say. We tend to avoid pointless {} in Xen. > >> Also, on the previous version it was mentioned >> this should be an error and then replace by a BUG_ON(). > > I don't see how this would be an actual bug. Yes, the code as it is > right now calls this only once, but it wouldn't hurt if we call this > multiple times. And I am always a bit wary of crashing the system if we > could just carry on instead, but ... > >> Please do the change. > > ... however you like. The question is not whether it would hurt to call this code twice but if it makes sense. It does not make sense to try to allocate the pendtable twice for the same CPU. So it is ever happening it means this is a programming error, hence a BUG_ON makes sense here. Another solution is having an ASSERT(this_cpu(pending_table) == NULL); at the beginning of the function. > >>> + >>> + reg |= GICR_PENDBASER_PTZ; >>> + >>> + ASSERT(!(virt_to_maddr(pendtable) & ~GENMASK(51, 16))); >> >> I don't understand the purpose of this ASSERT. The bits 15:0 should >> always be zero otherwise this would be a bug in the memory allocator. >> For bits 64:52, the architecture so far only support up to 52 bits. > > You complained about using a mask on the address before, which I can > understand. > However we are writing to a register described in the spec here and > should observe that the address only goes into bits [51:16]. Any other > bits should not be touched or we are getting weird errors. > So somehow I have to make sure we comply with this. > This could either be a mask or an ASSERT. > If the assert never fires: great. Nothing to worry about here. > But I think this matches the ASSERT idea: we rely on this address being 4K > aligned and not exceeding 52 bits worth of address bits, so we should > check these assumptions. ASSERT are only for debug build. However, nothing prevent the Xen allocator to differ between non-debug and debug build. So you may end up to never allocate address that are invalid for the GIC in non-debug build. > >> By keeping this ASSERT, you will make our life more difficult to extend >> the number of physical address supported if ARM decides to bump it. > > This GICR register is limited to 52 bits of physical address space > according to the spec. > If we ever upgrade the address size, the GIC spec would need to be > upgraded as well, so chances are we either need to touch that code here > anyways or we use a GICv5 or the like from the beginning. I am quite convinced it will make our life more difficult. But I am not going to fight for an ASSERT, there are more important stuff to fix. :) >> Why don't you just disable LPIs here? AFAIK, it should just be >> writel_relaxed(reg & ~GICR_CTLR_ENABLE_LPIS, GICR_CTLR); > > Please don't shoot the messenger, but: > GICv3 spec 8.11.2 "GICR_CTLR, Redistributor Control Register": > Enable_LPIs, bit [0]: > "... When a write changes this bit from 0 to 1, this bit becomes RES1 > and the Redistributor must load the LPI Pending table from memory to > check for any pending interrupts." > > Read: LPIs are so great that you can't disable them anymore once you > have enabled them. How this would work with kexec? > Yes, this is a bit weird, even has nasty side effects. In that case, I would prefer to have either a panic or make the CPU unusable to avoid having weird behavior afterwards. [...] >>> diff --git a/xen/include/asm-arm/bitops.h b/xen/include/asm-arm/bitops.h >>> index bda8898..1cbfb9e 100644 >>> --- a/xen/include/asm-arm/bitops.h >>> +++ b/xen/include/asm-arm/bitops.h >>> @@ -24,6 +24,7 @@ >>> #define BIT(nr) (1UL << (nr)) >>> #define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_WORD)) >>> #define BIT_WORD(nr) ((nr) / BITS_PER_WORD) >>> +#define BIT_ULL(nr) (1ULL << (nr)) >>> #define BITS_PER_BYTE 8 >>> >>> #define ADDR (*(volatile int *) addr) >>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h >>> index 836a103..12bd155 100644 >>> --- a/xen/include/asm-arm/gic.h >>> +++ b/xen/include/asm-arm/gic.h >>> @@ -220,6 +220,8 @@ enum gic_version { >>> GIC_V3, >>> }; >>> >>> +#define LPI_OFFSET 8192 >>> + >> >> It would make much sense to have this definition moved in irq.h close to >> NR_IRQS. >> >> Also, I am a bit surprised that NR_IRQS & co has not been modified. Is >> there any reason for that? > > It wasn't needed and really shouldn't be. > LPI are to some degree *not* first class citizen IRQs, and AFAICT > NR_IRQS relate to struct irq_decs's, which we don't have for LPIs (since > Xen itself doesn't really care about LPIs, at least as interrupts). > > I am still chasing every (derived) use of NR_IRQS, but so far this looks > good to me. Let me know if you find any issues with that. Can we have a comment on top of NR_IRQS explaining why LPIs are not taken into account? Cheers,
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index bf64c61..71734a1 100644 --- a/xen/arch/arm/Kconfig +++ b/xen/arch/arm/Kconfig @@ -49,6 +49,21 @@ config HAS_ITS bool "GICv3 ITS MSI controller support" depends on HAS_GICV3 +config MAX_PHYS_LPI_BITS + 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 (in bits) Xen should take + care of. The host ITS may provide support for a very large number + of supported LPIs, for all of which we may not want to allocate + memory, so this number here allows to limit this. + Xen itself does not know how many LPIs domains will ever need + beforehand. + 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/Makefile b/xen/arch/arm/Makefile index 5f4ff23..4ccf2eb 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -19,6 +19,7 @@ obj-y += gic.o obj-y += gic-v2.o obj-$(CONFIG_HAS_GICV3) += gic-v3.o obj-$(CONFIG_HAS_ITS) += gic-v3-its.o +obj-$(CONFIG_HAS_ITS) += gic-v3-lpi.o obj-y += guestcopy.o obj-y += hvm.o obj-y += io.o diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c new file mode 100644 index 0000000..e2fc901 --- /dev/null +++ b/xen/arch/arm/gic-v3-lpi.c @@ -0,0 +1,129 @@ +/* + * xen/arch/arm/gic-v3-lpi.c + * + * ARM GICv3 Locality-specific Peripheral Interrupts (LPI) support + * + * Copyright (C) 2016,2017 - ARM Ltd + * + * 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. + */ + +#include <xen/config.h> +#include <xen/lib.h> +#include <xen/mm.h> +#include <xen/sizes.h> +#include <asm/gic.h> +#include <asm/gic_v3_defs.h> +#include <asm/gic_v3_its.h> + +/* Global state */ +static struct { + uint8_t *lpi_property; + unsigned int host_lpi_bits; +} 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) - LPI_OFFSET) + +uint64_t gicv3_lpi_allocate_pendtable(void) +{ + uint64_t reg; + void *pendtable; + + reg = GIC_BASER_CACHE_RaWaWb << GICR_PENDBASER_INNER_CACHEABILITY_SHIFT; + reg |= GIC_BASER_CACHE_SameAsInner << GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT; + reg |= GIC_BASER_InnerShareable << GICR_PENDBASER_SHAREABILITY_SHIFT; + + if ( !this_cpu(pending_table) ) + { + /* + * The pending table holds one bit per LPI and even covers bits for + * interrupt IDs below 8192, so we allocate the full range. + * The GICv3 imposes a 64KB alignment requirement. + */ + pendtable = _xmalloc(BIT_ULL(lpi_data.host_lpi_bits) / 8, SZ_64K); + 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); + } + + reg |= GICR_PENDBASER_PTZ; + + ASSERT(!(virt_to_maddr(pendtable) & ~GENMASK(51, 16))); + reg |= virt_to_maddr(pendtable); + + return reg; +} + +uint64_t gicv3_lpi_get_proptable(void) +{ + uint64_t reg; + + reg = GIC_BASER_CACHE_RaWaWb << GICR_PENDBASER_INNER_CACHEABILITY_SHIFT; + reg |= GIC_BASER_CACHE_SameAsInner << GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT; + reg |= 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 |= ((lpi_data.host_lpi_bits - 1) << 0); + + ASSERT(!(virt_to_maddr(lpi_data.lpi_property) & ~GENMASK(51, 12))); + reg |= virt_to_maddr(lpi_data.lpi_property); + + return reg; +} + +static unsigned int max_lpi_bits = CONFIG_MAX_PHYS_LPI_BITS; +integer_param("max_lpi_bits", max_lpi_bits); + +int gicv3_lpi_init_host_lpis(unsigned int hw_lpi_bits) +{ + 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; +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 838dd11..fcb86c8 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); + printk("GICv3: %d lines, (IID %8.8x).\n", nr_lines, readl_relaxed(GICD + GICD_IIDR)); @@ -616,6 +619,33 @@ 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; + + /* We don't support LPIs without an ITS. */ + if ( list_empty(&host_its_list) ) + return -ENODEV; + + /* Make sure LPIs are disabled before setting up the tables. */ + 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 +688,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..1cbfb9e 100644 --- a/xen/include/asm-arm/bitops.h +++ b/xen/include/asm-arm/bitops.h @@ -24,6 +24,7 @@ #define BIT(nr) (1UL << (nr)) #define BIT_MASK(nr) (1UL << ((nr) % BITS_PER_WORD)) #define BIT_WORD(nr) ((nr) / BITS_PER_WORD) +#define BIT_ULL(nr) (1ULL << (nr)) #define BITS_PER_BYTE 8 #define ADDR (*(volatile int *) addr) diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index 836a103..12bd155 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -220,6 +220,8 @@ enum gic_version { GIC_V3, }; +#define LPI_OFFSET 8192 + extern enum gic_version gic_hw_version(void); /* Program the IRQ type into the GIC */ diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h index 6bd25a5..b307322 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,61 @@ #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) +/* For specifying the inner cacheability type only */ +#define GIC_BASER_CACHE_nCnB 0ULL +/* For specifying the outer cacheability type only */ +#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 GICR_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 GICR_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_PRIO_MASK 0xfc +#define LPI_PROP_RES1 (1 << 1) +#define LPI_PROP_ENABLED (1 << 0) + #define GICH_VMCR_EOI (1 << 9) #define GICH_VMCR_VENG1 (1 << 1) diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h index 2f5c51c..a66b6be 100644 --- a/xen/include/asm-arm/gic_v3_its.h +++ b/xen/include/asm-arm/gic_v3_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__ */
The ARM GICv3 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 redistributor. 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@arm.com> --- xen/arch/arm/Kconfig | 15 +++++ xen/arch/arm/Makefile | 1 + xen/arch/arm/gic-v3-lpi.c | 129 ++++++++++++++++++++++++++++++++++++++ xen/arch/arm/gic-v3.c | 44 +++++++++++++ xen/include/asm-arm/bitops.h | 1 + xen/include/asm-arm/gic.h | 2 + xen/include/asm-arm/gic_v3_defs.h | 52 ++++++++++++++- xen/include/asm-arm/gic_v3_its.h | 22 ++++++- 8 files changed, 264 insertions(+), 2 deletions(-) create mode 100644 xen/arch/arm/gic-v3-lpi.c