[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 04/28] ARM: GICv3 ITS: allocate device and collection table



Hi Andre,

On 30/01/17 18:31, Andre Przywara wrote:
Each ITS maps a pair of a DeviceID (usually the PCI b/d/f triplet) and
an EventID (the MSI payload or interrupt ID) to a pair of LPI number
and collection ID, which points to the target CPU.
This mapping is stored in the device and collection tables, which software
has to provide for the ITS to use.
Allocate the required memory and hand it the ITS.

s/hand it the/hand it to the/ ?

The maximum number of devices is limited to a compile-time constant
exposed in Kconfig.

Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
---
 xen/arch/arm/Kconfig             |  14 +++++
 xen/arch/arm/gic-v3-its.c        | 129 +++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/gic-v3.c            |   5 ++
 xen/include/asm-arm/gic_v3_its.h |  55 ++++++++++++++++-
 4 files changed, 202 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 71734a1..81bc233 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -64,6 +64,20 @@ config MAX_PHYS_LPI_BITS
           This can be overriden on the command line with the max_lpi_bits
           parameter.

+config MAX_PHYS_ITS_DEVICE_BITS
+        depends on HAS_ITS
+        int "Number of device bits the ITS supports"
+        range 1 32
+        default "10"
+        help
+          Specifies the maximum number of devices which want to use the ITS.
+          Xen needs to allocates memory for the whole range very early.
+          The allocation scheme may be sparse, so a much larger number must
+          be supported to cover devices with a high bus number or those on
+          separate bus segments.

As for MAX_PHYS_LPI_BITS, this Kconfig is questionable. The default value is arbitrary and may not fit everyone.

The way forward is to use the 2-level table if available to limit the memory usage. If only flat table is supported, then the user can use the command line option to limit it.

+          This can be overriden on the command line with the 
max_its_device_bits

s/overriden/overridden/

+          parameter.
+
 endmenu

 menu "ARM errata workaround via the alternative framework"
diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index ff0f571..c31fef6 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -20,9 +20,138 @@
 #include <xen/lib.h>
 #include <xen/device_tree.h>
 #include <xen/libfdt/libfdt.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>
+
+#define BASER_ATTR_MASK                                           \
+        ((0x3UL << GITS_BASER_SHAREABILITY_SHIFT)               | \
+         (0x7UL << GITS_BASER_OUTER_CACHEABILITY_SHIFT)         | \
+         (0x7UL << GITS_BASER_INNER_CACHEABILITY_SHIFT))
+#define BASER_RO_MASK   (GENMASK(58, 56) | GENMASK(52, 48))
+
+static uint64_t encode_phys_addr(paddr_t addr, int page_bits)
+{
+    uint64_t ret;
+
+    if ( page_bits < 16 )
+        return (uint64_t)addr & GENMASK(47, page_bits);
+
+    ret = addr & GENMASK(47, 16);
+    return ret | (addr & GENMASK(51, 48)) >> (48 - 12);
+}
+
+#define PAGE_BITS(sz) ((sz) * 2 + PAGE_SHIFT)

I know that PAGE_SHIFT has been suggested by Stefano on the previous version. However, I think this is wrong. The PAGE_BITS is not based on the page granularity of Xen, so I would much prefer to keep an 12 hardcoded with a comment.

+
+static int its_map_baser(void __iomem *basereg, uint64_t regc, int nr_items)

s/int/unsigned int/

+{
+    uint64_t attr, reg;
+    int entry_size = ((regc >> GITS_BASER_ENTRY_SIZE_SHIFT) & 0x1f) + 1;

s/int/unsigned int/

+    int pagesz = 0, order, table_size;

s/int/unsigned int/

+    void *buffer = NULL;
+
+    attr  = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
+    attr |= GIC_BASER_CACHE_SameAsInner << GITS_BASER_OUTER_CACHEABILITY_SHIFT;
+    attr |= GIC_BASER_CACHE_RaWaWb << GITS_BASER_INNER_CACHEABILITY_SHIFT;
+
+    /*
+     * Setup the BASE register with the attributes that we like. Then read
+     * it back and see what sticks (page size, cacheability and shareability
+     * attributes), retrying if necessary.
+     */
+    while ( 1 )

This loop is really confusing to read. A set of goto would probably make it more readable thanks to the labels labels.

+    {
+        table_size = ROUNDUP(nr_items * entry_size, BIT(PAGE_BITS(pagesz)));
+        order = get_order_from_bytes(table_size);
+
+        if ( !buffer )
+            buffer = alloc_xenheap_pages(order, 0);
+        if ( !buffer )
+            return -ENOMEM;
+
+        reg  = attr;
+        reg |= (pagesz << GITS_BASER_PAGE_SIZE_SHIFT);
+        reg |= table_size >> PAGE_BITS(pagesz);
+        reg |= regc & BASER_RO_MASK;
+        reg |= GITS_VALID_BIT;
+        reg |= encode_phys_addr(virt_to_maddr(buffer), PAGE_BITS(pagesz));
+
+        writeq_relaxed(reg, basereg);
+        regc = readl_relaxed(basereg);
+
+        /* The host didn't like our attributes, just use what it returned. */
+        if ( (regc & BASER_ATTR_MASK) != attr )
+        {
+            /* If we can't map it shareable, drop cacheability as well. */
+            if ( (regc & GITS_BASER_SHAREABILITY_MASK) == 
GIC_BASER_NonShareable )
+            {
+                regc &= ~GITS_BASER_INNER_CACHEABILITY_MASK;

So you drop cacheability, but you never clean & invalidate the cache. Is it normal?

+                attr = regc & BASER_ATTR_MASK;
+                continue;
+            }
+            attr = regc & BASER_ATTR_MASK;
+        }
+
+        /* If the host accepted our page size, we are done. */
+        if ( (regc & (3UL << GITS_BASER_PAGE_SIZE_SHIFT)) == pagesz )

This check looks wrong to me. The page size is encoded in bits 9:8 but I don't see any shift here.

Also a mask for the field would be useful.

+            return 0;
+
+        /* None of the page sizes was accepted, give up */
+        if ( pagesz >= 2 )
+            break;
+
+        free_xenheap_pages(buffer, order);
+        buffer = NULL;

If you move the check "if ( pagesz >= 2 )" here ...

+
+        pagesz++;
+    }
+
+    if ( buffer )
+        free_xenheap_pages(buffer, order);

... those 2 lines could be dropped.

+
+    return -EINVAL;
+}
+
+static unsigned int max_its_device_bits = CONFIG_MAX_PHYS_ITS_DEVICE_BITS;
+integer_param("max_its_device_bits", max_its_device_bits);

This new command line option needs to be documented in docs/misc/xen-command-line.markdown.

+
+int gicv3_its_init(struct host_its *hw_its)
+{
+    uint64_t reg;
+    int i;
+
+    hw_its->its_base = ioremap_nocache(hw_its->addr, hw_its->size);
+    if ( !hw_its->its_base )
+        return -ENOMEM;
+
+    for ( i = 0; i < GITS_BASER_NR_REGS; i++ )
+    {
+        void __iomem *basereg = hw_its->its_base + GITS_BASER0 + i * 8;
+        int type;
+
+        reg = readq_relaxed(basereg);
+        type = (reg & GITS_BASER_TYPE_MASK) >> GITS_BASER_TYPE_SHIFT;
+        switch ( type )
+        {
+        case GITS_BASER_TYPE_NONE:
+            continue;
+        case GITS_BASER_TYPE_DEVICE:
+            /* TODO: find some better way of limiting the number of devices */
+            its_map_baser(basereg, reg, BIT(max_its_device_bits));

You will waste space if the platform support less DevID bits max_its_device_bits.

+            break;
+        case GITS_BASER_TYPE_COLLECTION:
+            its_map_baser(basereg, reg, NR_CPUS);
+            break;
+        default:
+            continue;
+        }
+    }
+
+    return 0;
+}

 /* Scan the DT for any ITS nodes and create a list of host ITSes out of it. */
 void gicv3_its_dt_init(const struct dt_device_node *node)
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index fcb86c8..440c079 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -29,6 +29,7 @@
 #include <xen/irq.h>
 #include <xen/iocap.h>
 #include <xen/sched.h>
+#include <xen/err.h>
 #include <xen/errno.h>
 #include <xen/delay.h>
 #include <xen/device_tree.h>
@@ -1563,6 +1564,7 @@ static int __init gicv3_init(void)
 {
     int res, i;
     uint32_t reg;
+    struct host_its *hw_its;

     if ( !cpu_has_gicv3 )
     {
@@ -1618,6 +1620,9 @@ static int __init gicv3_init(void)
     res = gicv3_cpu_init();
     gicv3_hyp_init();

+    list_for_each_entry(hw_its, &host_its_list, entry)
+        gicv3_its_init(hw_its);
+

This loop could be handled in gic-v3-its.c.

     spin_unlock(&gicv3.lock);

     return res;

[...]

 #ifndef __ASSEMBLY__
 #include <xen/device_tree.h>

@@ -27,6 +74,7 @@ struct host_its {
     const struct dt_device_node *dt_node;
     paddr_t addr;
     paddr_t size;
+    void __iomem *its_base;
 };

 extern struct list_head host_its_list;
@@ -42,8 +90,9 @@ void gicv3_its_dt_init(const struct dt_device_node *node);
 uint64_t gicv3_lpi_get_proptable(void);
 uint64_t gicv3_lpi_allocate_pendtable(void);

-/* Initialize the host structures for LPIs. */
+/* Initialize the host structures for LPIs and the host ITSes. */
 int gicv3_lpi_init_host_lpis(unsigned int nr_lpis);
+int gicv3_its_init(struct host_its *hw_its);

 #else

@@ -62,6 +111,10 @@ static inline int gicv3_lpi_init_host_lpis(unsigned int 
nr_lpis)
 {
     return 0;
 }

Newline here.

+static inline int gicv3_its_init(struct host_its *hw_its)
+{
+    return 0;
+}

Newline here.

 #endif /* CONFIG_HAS_ITS */

 #endif /* __ASSEMBLY__ */


Regards,

--
Julien Grall

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.