[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v3 02/26] ARM: GICv3: allocate LPI pending and property table
Hi Andre,
On 31/03/17 19:05, 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..77f6009
--- /dev/null
+++ b/xen/arch/arm/gic-v3-lpi.c
@@ -0,0 +1,209 @@
[...]
+#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)
NIT: 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)) )
NIT the middle ( ... ) are not necessary.
[...]
+static int gicv3_lpi_set_proptable(void __iomem * rdist_base)
+{
+ uint64_t reg;
+
+ reg = GIC_BASER_CACHE_RaWaWb << GICR_PROPBASER_INNER_CACHEABILITY_SHIFT;
+ reg |= GIC_BASER_CACHE_SameAsInner <<
GICR_PROPBASER_OUTER_CACHEABILITY_SHIFT;
+ reg |= GIC_BASER_InnerShareable << GICR_PROPBASER_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 = _xmalloc(lpi_data.nr_host_lpis, SZ_4K);
+
+ if ( !table )
+ return -ENOMEM;
+
+ /* Make sure the physical address can be encoded in the register. */
+ if ( (virt_to_maddr(table) & ~GENMASK_ULL(51, 12)) )
+ {
+ xfree(table);
+ return -ERANGE;
+ }
+ memset(table, GIC_PRI_IRQ | LPI_PROP_RES1, MAX_PHYS_LPIS);
+ clean_and_invalidate_dcache_va_range(table, MAX_PHYS_LPIS);
+ lpi_data.lpi_property = table;
+ }
+
+ /* Encode the number of bits needed, minus one */
+ reg |= (fls(lpi_data.nr_host_lpis - 1) - 1);
NIT: 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 ( ... )
[...]
+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, please add a comment to explain why you don't sanitize the value
from the command line.
+ 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.
+
+ printk("GICv3: using at most %lu LPIs on the host.\n", MAX_PHYS_LPIS);
+
+ return 0;
+}
[...]
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 765a655..219d109 100644
--- a/xen/include/asm-arm/gic_v3_its.h
+++ b/xen/include/asm-arm/gic_v3_its.h
@@ -40,6 +40,11 @@ void gicv3_its_dt_init(const struct dt_device_node *node);
bool gicv3_its_host_has_its(void);
+int gicv3_lpi_init_rdist(void __iomem * rdist_base);
+
+/* Initialize the host structures for LPIs. */
+int gicv3_lpi_init_host_lpis(unsigned int nr_lpis);
Again, in the implementation, the parameter is called "hw_lpi_bits".
Please stay consistent.
+
#else
static LIST_HEAD(host_its_list);
@@ -53,6 +58,15 @@ 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;
+}
#endif /* CONFIG_HAS_ITS */
#endif
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index 8f7a167..13528c0 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -19,8 +19,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.
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.
+
/*
* 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
|
