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@lists.xen.org https://lists.xen.org/xen-devel