Re: [PATCH v2 02/27] ARM: GICv3: allocate LPI pending and property table

[Julien Grall <julien.grall@arm.com>]

Hi Andre,

On 03/16/2017 11:20 AM, Andre Przywara wrote:
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index bf64c61..86f7b53 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -49,6 +49,21 @@ config HAS_ITS
>          bool "GICv3 ITS MSI controller support"
>          depends on HAS_GICV3
>
> +config MAX_PHYS_LPI_BITS
> +        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 (in bits) Xen should take
> +          care of. The host ITS may provide support for a very large number
> +          of supported LPIs, for all of which we may not want to allocate
> +          memory, so this number here allows to limit this.
> +          Xen itself does not know how many LPIs domains will ever need
> +          beforehand.
> +          This can be overridden on the command line with the max_lpi_bits
> +          parameter.

I continue to disagree on this Kconfig option. There is no sensible 
default value and I don't see how a distribution will be able to pick-up 
one value here.

If the number of LPIs have to be restricted, this should be done via the 
command line or a per-platform option.

I have already made my point on previous e-mails so I am not going to 
argue more.

> +
>  endmenu
>
>  menu "ARM errata workaround via the alternative framework"
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 54860e0..02a8737 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -19,6 +19,7 @@ obj-y += gic.o
>  obj-y += gic-v2.o
>  obj-$(CONFIG_HAS_GICV3) += gic-v3.o
>  obj-$(CONFIG_HAS_ITS) += gic-v3-its.o
> +obj-$(CONFIG_HAS_ITS) += gic-v3-lpi.o
>  obj-y += guestcopy.o
>  obj-y += hvm.o
>  obj-y += io.o
> diff --git a/xen/arch/arm/gic-v3-lpi.c b/xen/arch/arm/gic-v3-lpi.c
> new file mode 100644
> index 0000000..4f8414b
> --- /dev/null
> +++ b/xen/arch/arm/gic-v3-lpi.c
> @@ -0,0 +1,201 @@
> +/*
> + * xen/arch/arm/gic-v3-lpi.c
> + *
> + * ARM GICv3 Locality-specific Peripheral Interrupts (LPI) support
> + *
> + * Copyright (C) 2016,2017 - ARM Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; under version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/lib.h>
> +#include <xen/mm.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 please.

> +/* 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;

Why unsigned long?

> +    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 is fairly confusing. When I read "nr_host_lpis" I understand that 
you store the number of LPIs. But in fact you store the maximum LPI ID.

Also it is very unclear the difference between MAX_PHYS_LPIS and 
nr_host_lpis and will likely cause trouble in the future.

So it looks like to me that nr_host_lpis should be named 
max_host_lpis_ids and MAX_PHYS_LPIS should be MAX_NR_PHYS_LPIS.

Also, please stay consistent with the naming. If you use host then use 
host everywhere and not a mix between phys and host for the same purpose.

> +
> +static int gicv3_lpi_allocate_pendtable(uint64_t *reg)
> +{
> +    uint64_t val;
> +    unsigned int order;
> +    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 contigious memory.

The bit after the comma seems quite obvious. Both xalloc and 
alloc_xenheap_pages will ensure that the region will be contiguous in 
the physical address space.

[...]

> +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. */
> +        unsigned int order = get_order_from_bytes(lpi_data.nr_host_lpis);
> +        void *table = alloc_xenheap_pages(order, 0);
> +
> +        if ( !table )
> +            return -ENOMEM;
> +
> +        /* Make sure the physical address can be encoded in the register. */
> +        if ( (virt_to_maddr(table) & ~GENMASK(51, 12)) )
> +        {
> +            free_xenheap_pages(table, 0);
> +            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) << 0);

As said on the previous version, please avoid hardcoded shift.

[...]

> +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)
> +        return ret;
> +    writeq_relaxed(table_reg, rdist_base + GICR_PENDBASER);

Same question as on the previous version. The cacheability and 
shareability may not stick. This was a bug fix in Linux (see commit 
241a386) and I am struggling to understand why Xen would not be affected?

> +
> +    return gicv3_lpi_set_proptable(rdist_base);
> +}
> +
> +static unsigned int max_lpi_bits = CONFIG_MAX_PHYS_LPI_BITS;
> +integer_param("max_lpi_bits", max_lpi_bits);
> +
> +int gicv3_lpi_init_host_lpis(unsigned int hw_lpi_bits)
> +{


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));

nr_host_lpis is "unsigned long" so why are you using BIT_ULL?

> +
> +    printk("GICv3: using at most %ld LPIs on the host.\n", MAX_PHYS_LPIS);

s/%ld/%lu/

[...]

> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 1512521..ed78363 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -548,6 +548,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);

Again, a macro has been suggested on in the last 2 versions to avoid 
hardcoded value. You said you will fix it and it is not done. Please do it.

[...]

> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 836a103..12bd155 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -220,6 +220,8 @@ enum gic_version {
>      GIC_V3,
>  };
>
> +#define LPI_OFFSET      8192

My comments on the previous version about LPI_OFFSET are not addressed. 
And one question was left unanswered.

> +
>  extern enum gic_version gic_hw_version(void);
>
>  /* Program the IRQ type into the GIC */

[...]

> 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);

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
>

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel