Re: [Xen-devel] [PATCH v4 03/27] ARM: GICv3: allocate LPI pending and property table

[Julien Grall <julien.grall@xxxxxxx>]

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