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

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 21/03/17 21:23, Julien Grall wrote:
> 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.

So are you opting for taking the hardware provided value, unless there
is a command line option or a per-platform limit?

Please keep in mind that the situation here is different from the device
table:
- There is no indirect table option for the property and pending table.
  Any redistributor supporting 32 bits worth of LPIs would lead to a
  4GB property table and 512MB pending table allocation.
- An OS like Linux can make sensible assumptions about the number of
  LPIs needed and only allocate as much LPIs as required. Linux for
  instance uses at most 65536. Xen cannot easily make this decision.
  So we would need to go as high as possible, but not too high to not
  waste memory. I see different tradeoffs here between a distribution
  targeting servers or another one aiming at some embedded devices, for
  instance. So I would expect exactly this decision to be covered by a
  Kconfig option.
- In contrast to the device ID, which is guest provided, the host LPI
  number is chosen by Xen. We allocate them in chunks of 32, starting
  at the beginning and just counting up. So we can limit them without
  loosing too much flexibility, because it's not that sparse as device
  IDs, for instance.

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

So as I mentioned before, I am happy to loose the Kconfig option, but
then we need some sensible default value. In our case we could be cheeky
here for now and just use the Linux value, because a Linux Dom0 would be
the only user. But that doesn't sound very future proof, though this may
not matter for 4.9.

>> +
>>  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?

Because someone requested that I store the actual number of LPIs, not
the number of bits required to encode them. So I need to cover the case
of exactly 32 bits worth of LPIs, which u32 cannot hold.

>> +    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.

One of the reasons I argued for storing the number of bits ....

> 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.

OK, can do.

>> +
>> +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.

Mmh, somehow I missed that when I tried to decide which allocation
function is the right one. But indeed xmalloc() eventually calls
alloc_xenheap_pages() with the order of alignment, so we get the same
guarantees.

> 
> [...]
> 
>> +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?

Oh, OK, we need to remove cacheability if shareability is denied, though
that sounds like a hardware bug to me if this is not observed by the
redistributor.
The reason I didn't care about it here was that software doesn't use the
pending table, so there would be nothing on the code side to be done in
case the redistributor shoots down one of our requests.
The mentioned Linux patch covers all ITS tables and the property table
as well.

>> +
>> +    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?

Right, creeping over from Linux where the ITS is supported by ARM(32) as
well.

>> +
>> +    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.

Sorry for that, I did this for GITS_TYPER_DEVICE_ID_BITS, so mentally
ticked this one off as well.
Will be fixed.

> 
> [...]
> 
>> 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.

Do you mean this?
>> It would make much sense to have this definition moved in irq.h
>> close to NR_IRQS.

Admittedly I missed that over the next question...
I think I wasn't convinced in the first place to put this GIC specific
detail into the generic irq.h, but I see that it makes sense since we
have the do_LPI() prototype in there as well.

>> Also, I am a bit surprised that NR_IRQS & co has not been modified.
>> Is there any reason for that?

But I answered on this one.
Is there anything else you want to have answered?

> 
>> +
>>  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.

Yeah, this header file is a constant source of merge conflicts when
squashing fixes and reworks, which makes it a pain to work with. Sorry
for that.

Cheers,
Andre.

>> +
>>  #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
>>
> 

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