|
[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
On Thu, 5 Jan 2017, Andre Przywara wrote:
> 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?
Let me explain why I made that comment. When I read "host_lpi_bits", it
is not immediately clear that it is strictly related to the number of
physical lpis. If you prefer to store the number of bits, instead of the
number of plpis (althought it is trivial to calculate one from the
other), then add a comment here.
> >> +} 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.
BUG_ON is fine.
> >> + }
> >> +
> >> + 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 ...
That's fine by me. The idea is that I shouldn't have to dig up the spec
every time I want to back to understand what this code is doing.
> >> +
> >> 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?
I am OK with exporting it on ARM too.
> 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 |