[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 09/39] xen/riscv: implement virtual APLIC MMIO emulation
- To: Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>
- From: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
- Date: Fri, 4 Sep 2026 13:58:11 +0200
- Authentication-results: eu.smtp.expurgate.cloud; dkim=pass header.s=20251104 header.d=gmail.com header.i="@gmail.com" header.h="Content-Transfer-Encoding:Content-Type:In-Reply-To:From:Content-Language:References:Cc:To:Subject:User-Agent:MIME-Version:Date:Message-ID"
- Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx, Romain Caritey <Romain.Caritey@xxxxxxxxxxxxx>, Zheng Zhang <zhangzheng@xxxxxxxxxxx>, Alistair Francis <alistair.francis@xxxxxxx>, Connor Davis <connojdavis@xxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Anthony PERARD <anthony.perard@xxxxxxxxxx>, Michal Orzel <michal.orzel@xxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Julien Grall <julien@xxxxxxx>, Roger Pau Monné <roger@xxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>
- Delivery-date: Fri, 04 Sep 2026 11:58:20 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
On 9/2/26 1:51 PM, Baptiste Le Duc wrote:
+/*
+ * The arrangement of IMSIC interrupt files in MMIO space follows a topology
+ * defined by the RISC-V AIA specification. An IMSIC group is a set of
+ * interrupt files (e.g., in a cluster or socket) co-located in memory.
+ *
+ * The physical address of an outgoing MSI is calculated by bitwise ORing a
+ * Base Physical Page Number (Base PPN) with the Group Index (g), the Hart
+ * Index (h) and, for a supervisor-level interrupt domain, the Guest Index:
+ *
+ * ( Base PPN | (g << (HHXS + 12)) | (h << LHXS) | guest ) << 12
Nit: it should be Guest Index (according to the spec) instead of `guest`
wording:
( Base PPN | (g << (HHXS + 12)) | (h << LHXS) | Guest Index ) << 12
Applied.
+ *
+ * where Base PPN, HHXS, LHXS, HHXW and LHXW come from the {m,s}msiaddrcfg[h]
+ * registers of the interrupt domain that sends the MSI:
+ *
+ * XLEN-1 HHXS+24 LHXS+12 12 0
+ * | | | | |
+ * ------------------------------------------------------------
+ * |xxxx| g |xxxxxxxx| h |xxxx|Guest Index| 0 |
+ * ------------------------------------------------------------
+ *
+ * - g: group number.
+ * - h: hart number relative to the group.
+ * - xxxx: remaining Base PPN bits; each gap may be zero-width.
+ * - Guest Index: selects one of the 4 KiB pages right above the hart's own
+ * supervisor-level file, i.e. it starts at bit 12; LHXS must therefore be
+ * at least as large as the number of guest index bits.
+ * - Bits 11:0: always zero because IMSIC files are 4 KiB page-aligned.
+ *
+ * For wired interrupts in MSI delivery mode (domaincfg.DM = 1) the APLIC
+ * builds that address itself from the "Hart Index" field (bits 31:18) of the
+ * corresponding target[i] register. That field holds a hart index *number*,
+ * in which both indices are packed adjacently:
+ *
+ * 13 lhxw+hhxw lhxw 0
+ * | | | |
+ * ------------------------------------
+ * | 0 |Group Index|Hart Index|
+ * ------------------------------------
+ *
+ * - lhxw (Low Hart Index Width): the number of bits used for the hart number
+ * within a group.
+ * - hhxw (High Hart Index Width): the number of bits used for the group
+ * number; the remaining bits of the field must be zero.
I think it's not very clear that the schema represents the "Hart Index" field
i.e. target[i] bits 31:18. Moreover, the schema like that is wrong as it is
not Group Index or Hart Index but `g` and `h`.
I'd suggest something like this:
* For wired interrupts in MSI delivery mode (domaincfg.DM = 1), the APLIC
* computes the MSI target address itself from the "Hart Index" field
* (bits 31:18) of the corresponding target[i] register. This 14-bit field
* holds both g and h:
*
* 13 lhxw+hhxw lhxw 0
* | | | |
* ------------------------------------
* | 0 | g | h |
* ------------------------------------
*
* - lhxw (Low Hart Index Width): the number of bits used for the hart
number
* within a group.
* - hhxw (High Hart Index Width): the number of bits used for the group
* number; the remaining bits of the field must be zero.
Applied.
+ *
+ * The Guest Index isn't a part of it: for a supervisor-level interrupt domain
+ * it has its own field (bits 17:12) in target[i].
+ *
+ * Because there are "xxxx" gaps (Base PPN bits) between the indices in the
+ * physical address (depending on HHXS and LHXS), software must extract the
+ * group and hart components separately and pack them into the APLIC-defined
+ * Hart Index format to ensure correct MSI targeting.
+ */
+static unsigned long aplic_hart_field(unsigned int cpu)
I should have renamed this to aplic_hart_index() as it's formerly what
the function returns.
Applied.
+{
+ const struct imsic_config *imsic = imsic_get_config();
+ const struct imsic_msi *msi = &imsic->msi[cpu];
Nit: this could be const ...
Sorry, I am not understand what do you expect from me to do with 'const'
here. At the moment we don't chnage anything in this function connected
to msi variable, just a reading.
+ /* Low Hart Index Shift */
+ unsigned int lhxs = imsic->guest_index_bits;
It seems incoherent with the diagram above as there is some xxxx
between Guest Index bits and lhxs + 12. Therefore, it is not that obvious
that lhxs is equal to guest_index_bits.
+ /* Low Hart Index Width */
+ unsigned int lhxw = imsic->hart_index_bits;
+ /* High Hart Index Width */
+ unsigned int hhxw = imsic->group_index_bits;
+ /* High Hart Index Shift */
+ unsigned int hhxs =
+ imsic->group_index_shift - APLIC_xMSICFGADDR_PPN_SHIFT * 2;
...
+ /*
+ * msi->base_addr is the base of the MMIO regset this CPU's interrupt
So if I understood correctly, msi->base_addr corresponds to the group
terminology? Is it always the case?
I think - yes. This value is taken from DTS which and is used to
describe IMSIC group.
+ * files live in, and one regset can cover several harts; msi->offset
+ * selects this CPU's block inside it. The hart index bits are part of
+ * that offset, so both indexes have to be derived from the full address.
+ */
+ paddr_t target_addr = msi->base_addr + msi->offset;
+ unsigned long tppn = target_addr >> APLIC_xMSICFGADDR_PPN_SHIFT;
+ unsigned long g =
+ (tppn >> APLIC_xMSICFGADDR_PPN_HHX_SHIFT(hhxs)) &
+ APLIC_xMSICFGADDR_PPN_HHX_MASK(hhxw);
+ unsigned long h =
+ (tppn >> APLIC_xMSICFGADDR_PPN_LHX_SHIFT(lhxs)) &
+ APLIC_xMSICFGADDR_PPN_LHX_MASK(lhxw);
+
+ return (g << lhxw) | h;
+}
+
+uint32_t aplic_msi_target_gen(const struct vcpu *target_vcpu,
+ uint32_t base_val)
+{
+ unsigned int guest_id = vcpu_guest_file_id(target_vcpu);
+ unsigned long hart_field = aplic_hart_field(target_vcpu->processor);
Nit: could be const
Techically I agree. Then for guest_id it should const too, right?
But it seems like Xen in such cases don't use const. Have you found a
rule that we have to use const in such cases?
I don't mind to put const here but then it would be nice if someone will
tell me some kind of rule...
Should be hart_index too, according to previous comment.
Applied.
+
+ base_val &= APLIC_TARGET_EIID;
+ base_val |= MASK_INSR(guest_id, APLIC_TARGET_GUEST_IDX);
+ base_val |= MASK_INSR(hart_field, APLIC_TARGET_HART_IDX);
+
+ return base_val;
+}
+
+uint32_t aplic_hw_read_reg(unsigned int offset)
+{
+ unsigned long flags;
+ uint32_t val;
+
+ ASSERT((offset < aplic.size) && IS_ALIGNED(offset, sizeof(uint32_t)));
+
+ spin_lock_irqsave(&aplic.lock, flags);
+ val = readl((volatile void __iomem *)aplic.regs + offset);
+ spin_unlock_irqrestore(&aplic.lock, flags);
+
+ return val;
+}
+
+void aplic_hw_write_reg(unsigned int offset, uint32_t value)
+{
+ unsigned long flags;
+
+ ASSERT((offset < aplic.size) && IS_ALIGNED(offset, sizeof(uint32_t)));
+
+ spin_lock_irqsave(&aplic.lock, flags);
+ writel(value, (volatile void __iomem *)aplic.regs + offset);
+ spin_unlock_irqrestore(&aplic.lock, flags);
+}
+
static void __init aplic_init_hw_interrupts(void)
{
unsigned int i;
@@ -53,9 +173,9 @@ static void __init aplic_init_hw_interrupts(void)
/*
* Low bits of target register contains Interrupt Priority bits which
* can't be zero according to AIA spec.
- * Thereby they are initialized to APLIC_DEFAULT_PRIORITY.
+ * Thereby they are initialized to APLIC_TARGET_IPRIO_DEFAULT.
*/
- writel(APLIC_DEFAULT_PRIORITY, &aplic.regs->target[i]);
+ writel(APLIC_TARGET_IPRIO_DEFAULT, &aplic.regs->target[i]);
}
writel(APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM, &aplic.regs->domaincfg);
diff --git a/xen/arch/riscv/include/asm/aplic.h
b/xen/arch/riscv/include/asm/aplic.h
index a2af55d54f..babba38607 100644
--- a/xen/arch/riscv/include/asm/aplic.h
+++ b/xen/arch/riscv/include/asm/aplic.h
@@ -39,6 +39,13 @@
#define APLIC_DOMAINCFG_IE BIT(8, U)
#define APLIC_DOMAINCFG_DM BIT(2, U)
#define APLIC_DOMAINCFG_BE BIT(0, U)
+/*
+ * The bits a write may change. Everything else, including the read-only zero
+ * bit 7 and the reserved bits, has to read back as zero, and BE is WARL and
+ * hardwired to 0 as Xen is little-endian only.
+ */
+#define APLIC_DOMAINCFG_WMASK (APLIC_DOMAINCFG_IE | \
+ APLIC_DOMAINCFG_DM)
#define APLIC_SOURCECFG_BASE 0x0004
#define APLIC_SOURCECFG_LAST 0x0ffc
@@ -89,6 +96,9 @@
#define APLIC_TARGET_GUEST_IDX GENMASK(17, 12)
/* Bit 11 is reserved and reads as zero */
#define APLIC_TARGET_EIID GENMASK(10, 0)
+/* If target is in DM mode */
I think this comment is not clear; I expect, by reading it, to have
domaincfg.DM = 1 which is MSI mode, but I think you were talking about
direct delivery mode, right? If so, I would change this comment to
/* If target is in direct delivery mode (domaincfg.DM = 0) */
Yes, my comment is incorrect, it should be yours. Applied.
+#define APLIC_TARGET_IPRIO GENMASK(7, 0)
+#define APLIC_TARGET_IPRIO_DEFAULT 1U
#define APLIC_IDC_SIZE 32
@@ -98,6 +108,27 @@
#define APLIC_SIZE(nr_cpus) \
(APLIC_MIN_SIZE + APLIC_SIZE_ALIGN(APLIC_IDC_SIZE * (nr_cpus)))
+/*
+ * Using setip is fine here, as all SET* and CLR* register groups consist of 32
+ * registers and therefore have identical sizes.
+ *
+ * Lowest 2 bits are always zero for SET* and CLR* registers.
+ */
+#define APLIC_SETCLR_OFFSET_MASK \
+ (sizeof_field(struct aplic_regs, setip) - sizeof(uint32_t))
+
+#define APLIC_xMSICFGADDR_PPN_SHIFT IMSIC_MMIO_PAGE_SHIFT
+
+#define APLIC_xMSICFGADDR_PPN_HHX_MASK(hhxw) \
+ (BIT(hhxw, UL) - 1)
+#define APLIC_xMSICFGADDR_PPN_HHX_SHIFT(hhxs) \
+ ((hhxs) + APLIC_xMSICFGADDR_PPN_SHIFT)
+
+#define APLIC_xMSICFGADDR_PPN_LHX_MASK(lhxw) \
+ (BIT(lhxw, UL) - 1)
+#define APLIC_xMSICFGADDR_PPN_LHX_SHIFT(lhxs) \
+ (lhxs)
+
struct aplic_regs {
uint32_t domaincfg; /* 0x0000 */
uint32_t sourcecfg[1023]; /* 0x0004 */
@@ -141,4 +172,7 @@ struct aplic_regs {
uint32_t target[1023]; /* 0x3004 */
};
+uint32_t aplic_hw_read_reg(unsigned int offset);
+void aplic_hw_write_reg(unsigned int offset, uint32_t value);
+
#endif /* ASM_RISCV_APLIC_H */
diff --git a/xen/arch/riscv/include/asm/imsic.h
b/xen/arch/riscv/include/asm/imsic.h
index 2425430ed1..93f9e44c7d 100644
--- a/xen/arch/riscv/include/asm/imsic.h
+++ b/xen/arch/riscv/include/asm/imsic.h
@@ -40,6 +40,19 @@ struct imsic_config {
/* Base address */
paddr_t base_addr;
+ /*
+ * MSI Target Address Scheme
+ *
+ * XLEN-1 HHXS+24 LHXS+12 12 0
+ * | | | | |
+ * ------------------------------------------------------------
+ * |xxxx| g |xxxxxxxx| h |xxxx|Guest Index| 0 |
+ * ------------------------------------------------------------
+ * - g: group number.
+ * - h: hart number relative to the group.
+ * - xxxx: remaining Base PPN bits; each gap may be zero-width.
+ */
+
Is this really needed as you already explain this above in aplic.c?
Please choose one place between the two if not.
No, I don't think so. For me, it is also enough to have in one place. I
will drop it here and keep only in aplic.c.
[...]
+
+static bool vaplic_emulate_store(const struct vcpu *curr, paddr_t addr,
+ uint32_t value)
+{
+ const struct domain *currd = curr->domain;
+ unsigned int offset = addr & APLIC_CTRL_REGION_OFFSET_MASK;
+
+ ASSERT(curr == current);
+
+ switch ( offset )
+ {
+ case APLIC_SETIP_BASE ... APLIC_SETIP_LAST:
+ case APLIC_CLRIP_BASE ... APLIC_CLRIP_LAST:
+ case APLIC_SETIE_BASE ... APLIC_SETIE_LAST:
+ case APLIC_CLRIE_BASE ... APLIC_CLRIE_LAST:
+ {
+ unsigned int word_idx =
+ regoffset_to_word_idx(offset & APLIC_SETCLR_OFFSET_MASK);
+
+ value &= generate_auth_mask(currd, word_idx);
+
+ break;
+ }
+
+ case APLIC_SOURCECFG_BASE ... APLIC_SOURCECFG_LAST:
+ if ( value & APLIC_SOURCECFG_D )
+ {
+ dprintk(XENLOG_ERR, "APLIC_SOURCECFG_D isn't supported\n");
+
+ goto fail;
+ }
+
+ /*
+ * As sourcecfg register starts from 1:
+ * 0x0000 domaincfg
+ * 0x0004 sourcecfg[1]
+ * 0x0008 sourcecfg[2]
+ * ...
+ * 0x0FFC sourcecfg[1023]
+ * It is necessary to calculate an interrupt number by subtracting
+ * APLIC_DOMAINCFG instead of APLIC_SOURCECFG_BASE.
+ */
+ if ( !AUTH_IRQ_BIT(currd,
+ regoffset_to_word_idx(offset - APLIC_DOMAINCFG)) )
+ /* Interrupt not enabled, ignore it */
+ return true;
+
+ if ( value > APLIC_SOURCECFG_SM_LEVEL_LOW )
+ {
+ gdprintk(XENLOG_ERR,
+ "value(%#x) is incorrect for sourcecfg register\n",
+ value);
+
+ return true;
+ }
+
+ break;
+
+ case APLIC_TARGET_BASE ... APLIC_TARGET_LAST:
+ {
+ struct vaplic *vaplic = to_vaplic(currd);
+ struct vcpu *target_vcpu;
+ unsigned int guest_hart_idx = MASK_EXTR(value, APLIC_TARGET_HART_IDX);
+ /*
+ * Look at vaplic_emulate_load() for explanation why APLIC_GENMSI is
+ * subtracted.
+ */
+ unsigned int srcn = regoffset_to_word_idx(offset - APLIC_GENMSI);
+
+ if ( !AUTH_IRQ_BIT(currd, srcn) )
+ /* Interrupt not enabled, ignore it */
+ return true;
+
+ target_vcpu = domain_vcpu(currd, guest_hart_idx);
+
+ if ( !target_vcpu )
+ {
+ dprintk(XENLOG_ERR, "Invalid vCPU id in target register\n");
+
+ /* Ignore such writings */
+ return true;
+ }
+
+ if ( vaplic->regs.domaincfg & APLIC_DOMAINCFG_DM )
+ {
+ /*
+ * A non-zero guest index asks for delivery to an interrupt file of
+ * nested guest. The vIMSIC node has no riscv,guest-index-bits
+ * property, so a guest is told its harts have no guest interrupt
+ * files and the field is read-only zero for them. The write isn't
+ * rejected (that would throw away a valid hart index and EIID);
+ * instead the field is dropped, which is also what
+ * aplic_msi_target_gen() does with it when programming the h/w.
+ */
+ if ( MASK_EXTR(value, APLIC_TARGET_GUEST_IDX) )
+ {
+ printk_once(XENLOG_WARNING
+ "%pd: vAPLIC target guest index != 0 is
unsupported\n",
+ currd);
+
+ /* Ignore such writes ... */
+ return true;
+ }
Comment above this says "The write isn't rejected ... instead the field
is dropped, which is also what aplic_msi_target_gen() does with it." But
the code doesn't follow it as it returns true immediately here before
the write occurred and without zeroing the guest index field.
You're right that the comment doesn't match the code, but the fix is in
the comment rather than in the code. vaplic->regs.target[] comes from
xvzalloc_array(), so the guest index field starts as zero, and every
write carrying a non-zero guest index is rejected here, the field can
therefore never become non-zero and there is nothing to mask out.
Rejecting the whole write is deliberate: a non-zero guest index is
illegal for a guest whose vIMSIC advertises no guest interrupt files.
The second half of the comment was wrong too: aplic_msi_target_gen()
doesn't drop the field, it overwrites it with vcpu_guest_file_id() of
the target vCPU, which is non-zero when that vCPU owns a h/w VS-file.
I'll reword the comment in v3 accordingly:
/*
* A non-zero guest index asks for delivery to an interrupt file
* of a nested guest. The vIMSIC node has no riscv,guest-index-bits
* property, so a guest is told its harts have no guest interrupt
* files and the field reads as zero for them. Such a write is
* illegal and is therefore ignored as a whole: the stored copy
* keeps the zero it was allocated with, so the field never needs
* to be masked out here.
* What ends up in the h/w register is Xen's own value anyway:
* aplic_msi_target_gen() overwrites the field with
* vcpu_guest_file_id() of the target vCPU.
*/
Are you okay with that?
Thanks!
~ Oleksii
|
