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

[PATCH v3 09/39] xen/riscv: build the target hart index via aplic_hart_index()



aplic_set_irq_affinity() open-coded the packing of the group and hart
indices into the target register, and got two things wrong along the
way:

 - imsic_config.msi[] is indexed by logical CPU id, but the index was
   run through cpuid_to_hartid() first. On any platform where the two
   spaces differ this picks another CPU's interrupt file, or reads past
   the array;

 - the same hart id was then used verbatim as the low hart index, and
   the group index was derived from msi[].base_addr alone. The hart
   index bits live in msi[].offset, the base address only covers the
   MMIO regset, which may hold the files of several harts. Both indices
   have to come out of base_addr + offset.

aplic_hart_index() already extracts them that way, and is what the vAPLIC
target path uses, so call it here as well and insert the result with
MASK_INSR(APLIC_TARGET_HART_IDX) instead of a bare shift, which keeps the
value from spilling out of the 14-bit field. This drops the last in-tree
duplicate of the AIA hart index formula, and with it the last user of
APLIC_TARGET_HART_IDX_SHIFT, which is removed.

desc->irq is used as the EIID as is. Assert that it is a valid APLIC
source, which also guarantees that it fits the EIID field.

No functional change on a single-group platform whose hart ids match
their CPU ids and whose IMSIC regset holds one file per hart.

Fixes: d4676a1398bc ("xen/riscv: implementation of aplic and imsic operations")
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
---
Changes in v3:
 - s/aplic_hart_field/aplic_hart_index/ (in the subject too) following the
   rename of the helper in "xen/riscv: implement virtual APLIC MMIO
   emulation".
 - Don't mask desc->irq with APLIC_TARGET_EIID. Instead, bound it by
   the APLIC source range rather than NR_IRQS, which will also have to
   cover device MSIs: add a BUILD_BUG_ON() that ARRAY_SIZE(target) fits
   the EIID field and an ASSERT() that desc->irq is a valid APLIC source
   (1..num_irqs), which also covers the target[desc->irq - 1] index.
---
Changes in v2:
 - New patch.
---
---
 xen/arch/riscv/aplic.c             | 33 ++++++++++--------------------
 xen/arch/riscv/include/asm/aplic.h |  1 -
 2 files changed, 11 insertions(+), 23 deletions(-)

diff --git a/xen/arch/riscv/aplic.c b/xen/arch/riscv/aplic.c
index febc451760bf..62bbcc9afacf 100644
--- a/xen/arch/riscv/aplic.c
+++ b/xen/arch/riscv/aplic.c
@@ -335,9 +335,7 @@ static unsigned int aplic_get_cpu_from_mask(const cpumask_t 
*cpumask)
 static void cf_check aplic_set_irq_affinity(struct irq_desc *desc, const 
cpumask_t *mask)
 {
     unsigned int cpu;
-    uint64_t group_index, base_ppn;
-    uint32_t hhxw, lhxw, hhxs, value;
-    const struct imsic_config *imsic = aplic.imsic_cfg;
+    uint32_t value;
 
     /*
      * TODO: Currently, APLIC is supported only with MSI interrupts.
@@ -350,27 +348,18 @@ static void cf_check aplic_set_irq_affinity(struct 
irq_desc *desc, const cpumask
 
     ASSERT(spin_is_locked(&desc->lock));
 
-    cpu = cpuid_to_hartid(aplic_get_cpu_from_mask(mask));
-    hhxw = imsic->group_index_bits;
-    lhxw = imsic->hart_index_bits;
+    cpu = aplic_get_cpu_from_mask(mask);
+
     /*
-     * Although this variable is used only once in the calculation of
-     * group_index, and it might seem that hhxs could be defined as:
-     *   hhxs = imsic->group_index_shift - IMSIC_MMIO_PAGE_SHIFT;
-     * and then the addition of IMSIC_MMIO_PAGE_SHIFT could be omitted
-     * when calculating the group index.
-     * It was done intentionally this way to follow the formula from
-     * the AIA specification for calculating the MSI address.
+     * desc->irq is used as the EIID as-is, which is only valid for APLIC
+     * sources: they are numbered 1..num_irqs, with at most ARRAY_SIZE(target).
      */
-    hhxs = imsic->group_index_shift - IMSIC_MMIO_PAGE_SHIFT * 2;
-    base_ppn = imsic->msi[cpu].base_addr >> IMSIC_MMIO_PAGE_SHIFT;
-
-    /* Update hart and EEID in the target register */
-    group_index = (base_ppn >> (hhxs + IMSIC_MMIO_PAGE_SHIFT)) &
-                  (BIT(hhxw, UL) - 1);
-    value = desc->irq;
-    value |= cpu << APLIC_TARGET_HART_IDX_SHIFT;
-    value |= group_index << (lhxw + APLIC_TARGET_HART_IDX_SHIFT);
+    BUILD_BUG_ON(ARRAY_SIZE(aplic.regs->target) >
+                 MASK_EXTR(~0U, APLIC_TARGET_EIID));
+    ASSERT(desc->irq && desc->irq <= aplic_info.num_irqs);
+
+    value = MASK_INSR(aplic_hart_index(cpu), APLIC_TARGET_HART_IDX) |
+            desc->irq;
 
     spin_lock(&aplic.lock);
 
diff --git a/xen/arch/riscv/include/asm/aplic.h 
b/xen/arch/riscv/include/asm/aplic.h
index 664bda2cb7eb..9018936ca95a 100644
--- a/xen/arch/riscv/include/asm/aplic.h
+++ b/xen/arch/riscv/include/asm/aplic.h
@@ -98,7 +98,6 @@
 #define APLIC_TARGET_BASE               0x3004
 #define APLIC_TARGET_LAST               0x3ffc
 #define  APLIC_TARGET_HART_IDX          GENMASK(31, 18)
-#define  APLIC_TARGET_HART_IDX_SHIFT    18
 #define  APLIC_TARGET_GUEST_IDX         GENMASK(17, 12)
 /* Bit 11 is reserved and reads as zero */
 #define  APLIC_TARGET_EIID              GENMASK(10, 0)
-- 
2.55.0




 


Rackspace

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