[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 03/27] ARM: GICv3: allocate LPI pending and property table
Hi Andre, Yeah... another round repeating the same things. On 04/03/2017 09:28 PM, Andre Przywara wrote: diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c new file mode 100644 index 0000000..a003a72 --- /dev/null +++ b/xen/arch/arm/gic-v3-lpi.c [...] +#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> +#include <asm/io.h> +#include <asm/page.h> + +#define LPI_PROPTABLE_NEEDS_FLUSHING (1U << 0) Newline here. +/* Global state */ +static struct { + /* The global LPI property table, shared by all redistributors. */ + uint8_t *lpi_property; + /* + * Number of physical LPIs the host supports. This is a property of + * the GIC hardware. We depart from the habit of naming these things + * "physical" in Xen, as the GICv3/4 spec uses the term "physical LPI" + * in a different context to differentiate them from "virtual LPIs". + */ + unsigned long int nr_host_lpis; On v2, you said you will rename this variable to max_host_lpi_ids and ... + unsigned int flags; +} lpi_data; + +struct lpi_redist_data { + void *pending_table; +}; + +static DEFINE_PER_CPU(struct lpi_redist_data, lpi_redist); + +#define MAX_PHYS_LPIS (lpi_data.nr_host_lpis - LPI_OFFSET) ... this one to MAX_NR_PHYS_LPIS or even MAX_NR_HOST_LPIS to stay consistent. So please do it. + +static int gicv3_lpi_allocate_pendtable(uint64_t *reg) +{ + uint64_t val; + void *pendtable; + + if ( this_cpu(lpi_redist).pending_table ) + return -EBUSY; + + val = GIC_BASER_CACHE_RaWaWb << GICR_PENDBASER_INNER_CACHEABILITY_SHIFT; + val |= GIC_BASER_CACHE_SameAsInner << GICR_PENDBASER_OUTER_CACHEABILITY_SHIFT; + val |= GIC_BASER_InnerShareable << GICR_PENDBASER_SHAREABILITY_SHIFT; + + /* + * 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, also requires + * physically contiguous memory. + */ + pendtable = _xzalloc(lpi_data.nr_host_lpis / 8, SZ_64K); + if ( !pendtable ) + return -ENOMEM; + + /* Make sure the physical address can be encoded in the register. */ + if ( (virt_to_maddr(pendtable) & ~GENMASK_ULL(51, 16)) ) The middle ( ... ) are not necessary. [...] +/* + * Tell a redistributor about the (shared) property table, allocating one + * if not already done. + */ +static int gicv3_lpi_set_proptable(void __iomem * rdist_base) +{ [...] + /* Encode the number of bits needed, minus one */ + reg |= (fls(lpi_data.nr_host_lpis - 1) - 1); The outer ( ... ) are not necessary. [...] +int gicv3_lpi_init_rdist(void __iomem * rdist_base) +{ + uint32_t reg; + uint64_t table_reg; + int ret; + + /* We don't support LPIs without an ITS. */ + if ( !gicv3_its_host_has_its() ) + 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; + + ret = gicv3_lpi_allocate_pendtable(&table_reg); + if (ret) Coding style: if ( ... ) + return ret; + writeq_relaxed(table_reg, rdist_base + GICR_PENDBASER); + table_reg = readq_relaxed(rdist_base + GICR_PENDBASER); + + /* If the hardware reports non-shareable, drop cacheability as well. */ + if ( !(table_reg & GICR_PENDBASER_SHAREABILITY_MASK) ) + { + table_reg &= GICR_PENDBASER_SHAREABILITY_MASK; + table_reg &= GICR_PENDBASER_INNER_CACHEABILITY_MASK; + table_reg |= GIC_BASER_CACHE_nC << GICR_PENDBASER_INNER_CACHEABILITY_SHIFT; + + writeq_relaxed(table_reg, rdist_base + GICR_PENDBASER); + } + + return gicv3_lpi_set_proptable(rdist_base); +} + +static unsigned int max_lpi_bits = 20; +integer_param("max_lpi_bits", max_lpi_bits); + +int gicv3_lpi_init_host_lpis(unsigned int hw_lpi_bits) +{ Again, this should be sanitize. A user could pass max_lpi_bits=10, and I don't think this code will behave well. + lpi_data.nr_host_lpis = BIT_ULL(min(hw_lpi_bits, max_lpi_bits)); Again, nr_host_lpis is "unsigned long" so why are you using BIT_ULL? Looking at the introduction of GENMASK_ULL, it likely means nr_host_lpis should be unsigned long long. + + if ( lpi_data.nr_host_lpis > 16 * 1024 * 1024 ) Hmmm? 16 * 1024 * 1024? Where does it come from? Please add a comment and explain in the commit message. Also, you could make the code more readable and using "16 << 20". + printk(XENLOG_WARNING "Allocating %lu host LPIs, please limit with --max_lpi_bits\n", The command line options on xen does not start with "--". Also the user may have purposefully chosen a value higher than 16 << 20. So this comment seem a big weird. How about: "%lu host LPIs will allocated, to limit memory usage please restrict it with max_lpi_bits.\n". + lpi_data.nr_host_lpis); Please use warning_add, it will gather at the end of the boot. diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h index b2edf95..1f730ce 100644 --- a/xen/include/asm-arm/config.h +++ b/xen/include/asm-arm/config.h @@ -19,6 +19,8 @@ #define BITS_PER_LONG (BYTES_PER_LONG << 3) #define POINTER_ALIGN BYTES_PER_LONG +#define BITS_PER_LONG_LONG (sizeof (long long) * BITS_PER_BYTE) + /* xen_ulong_t is always 64 bits */ #define BITS_PER_XEN_ULONG 64 diff --git a/xen/include/asm-arm/gic_v3_defs.h b/xen/include/asm-arm/gic_v3_defs.h index 6bd25a5..7cdebc5 100644 --- a/xen/include/asm-arm/gic_v3_defs.h +++ b/xen/include/asm-arm/gic_v3_defs.h @@ -44,7 +44,10 @@ #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_ID_BITS(r) ((((r) >> GICD_TYPE_ID_BITS_SHIFT) & 0x1f) + 1) Please align with the rest. [...] diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h index 7d88987..7c5a2fa 100644 --- a/xen/include/asm-arm/gic_v3_its.h +++ b/xen/include/asm-arm/gic_v3_its.h @@ -76,7 +76,10 @@ void gicv3_its_dt_init(const struct dt_device_node *node); bool gicv3_its_host_has_its(void); -/* Initialize the host structures for the host ITSes. */ +int gicv3_lpi_init_rdist(void __iomem * rdist_base); + +/* Initialize the host structures for LPIs and the host ITSes. */ +int gicv3_lpi_init_host_lpis(unsigned int nr_lpis); Again, in the implementation, the parameter is called "hw_lpi_bits". Please stay consistent. int gicv3_its_init(void); #else @@ -92,6 +95,16 @@ static inline bool gicv3_its_host_has_its(void) return false; } +static inline int gicv3_lpi_init_rdist(void __iomem * rdist_base) +{ + return -ENODEV; +} + +static inline int gicv3_lpi_init_host_lpis(unsigned int nr_lpis) +{ Ditto. + return 0; +} + static inline int gicv3_its_init(void) { return 0; diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h index 4849f16..f940092 100644 --- a/xen/include/asm-arm/irq.h +++ b/xen/include/asm-arm/irq.h @@ -18,8 +18,16 @@ struct arch_irq_desc { }; #define NR_LOCAL_IRQS 32 + +/* + * This only covers the interrupts that Xen cares about, so SGIs, PPIs and + * SPIs. LPIs are too numerous, also only propagated to guests, so they are + * not included in this number. + */ #define NR_IRQS 1024 +#define LPI_OFFSET 8192 + #define nr_irqs NR_IRQS #define nr_static_irqs NR_IRQS #define arch_hwdom_irqs(domid) NR_IRQS diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h index bd0883a..9261e06 100644 --- a/xen/include/xen/bitops.h +++ b/xen/include/xen/bitops.h @@ -5,11 +5,14 @@ /* * Create a contiguous bitmask starting at bit position @l and ending at * position @h. For example - * GENMASK(30, 21) gives us the 32bit vector 0x01fe00000. + * GENMASK(30, 21) gives us the 32bit vector 0x7fe00000. You said, you will fix it tomorrow. But for record, this is a new addition in this series and should really be a separate patch with the appropriate maintainers CCed. */ #define GENMASK(h, l) \ (((~0UL) << (l)) & (~0UL >> (BITS_PER_LONG - 1 - (h)))) +#define GENMASK_ULL(h, l) \ + (((~0ULL) << (l)) & (~0ULL >> (BITS_PER_LONG_LONG - 1 - (h)))) This should also be a separate patch with BITS_PER_LONG_LONG too. Please take a look at https://patchwork.kernel.org/patch/8824091/ for some suggestion about this. + /* * ffs: find first bit set. This is defined the same way as * the libc and compiler builtin ffs routines, therefore Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |