virtual interrupt controller (vAPLIC/vIMSIC) support

Both constants were inherited from OpenSBI and their names are misleading. The length of an instruction is encoded in its low bits: bits[1:0] != 11 means 16 bits, otherwise bits[4:2] != 111 means 32 bits, and anything else is 48 bits or wider. 0x3 and 0x1c are the patterns compared against in these two tests, not masks selecting a 16-bit or a 32-bit instruction. As each constant is needed in one place only, once INSN_IS_16BIT() and INSN_IS_32BIT() encapsulate the tests, drop them and spell the values out in those two macros instead.

[...]

Quoted 8 lines · Oleksii Kurochko

Both constants were inherited from OpenSBI and their names are misleading. The length of an instruction is encoded in its low bits: bits[1:0] != 11 means 16 bits, otherwise bits[4:2] != 111 means 32 bits, and anything else is 48 bits or wider. 0x3 and 0x1c are the patterns compared against in these two tests, not masks selecting a 16-bit or a 32-bit instruction. As each constant is needed in one place only, once INSN_IS_16BIT() and INSN_IS_32BIT() encapsulate the tests, drop them and spell the values out in those two macros instead.

Reviewed-by Baptiste Le Duc <[email protected]>

kernel_image64_probe() never set info->type, so every domain kept the DOMAIN_32BIT value a zeroed KERNEL_INFO_INIT leaves behind, even though Xen on RISC-V is only built for RV64 and accepts nothing but a 64-bit Image.

Nothing consumes d->type on RISC-V yet, so there is no functional change.

[...]

Quoted 8 lines · Oleksii Kurochko

kernel_image64_probe() never set info->type, so every domain kept the DOMAIN_32BIT value a zeroed KERNEL_INFO_INIT leaves behind, even though Xen on RISC-V is only built for RV64 and accepts nothing but a 64-bit Image.

Nothing consumes d->type on RISC-V yet, so there is no functional change.

[...]

Reviewed-by Baptiste Le Duc <[email protected]>

On RV32 a 64-bit CSR is accessed as two halves, <csr> and <csr>H. Reading

Quoted 1 line

On RV32 a 64-bit CSR is accessed as two halves, <csr> and <csr>H. Reading

As far as I can tell RV32 can't currently be selected (RISCV_64 is def_bool y, no RISCV_32 in Kconfig), so this path can't even be build-tested.

Is the retry
loop really needed now, or a least in this patch series?

On RV32 a 64-bit CSR is accessed as two halves, <csr> and <csr>H. Reading the halves with two separate instructions isn't always safe: for a CSR which hardware increments, a carry from the low half into the high one can happen in between, so a plain pair of reads can produce a value the CSR never held. Therefore the high half is re-read and the sequence retried if it changed in the meantime.

Only CSRs which change under Xen's feet need that, hence the name. The majority can be read with a plain pair of csr_read()s, so no helper for those is introduced here as there would be no user for it.

Use it for CSR_TIME, which is exactly such a counter, and widen cycles_t to uint64_t. Otherwise get_cycles() would still truncate the time counter to 32 bits on RV32.

No functional change on RV64.

Fixes a541ddadec0a ("xen/riscv: introduce time.h")

Signed-off-by Oleksii Kurochko <[email protected]>

xen/arch/riscv/include/asm/csr.h +4 -0

@@ -39,12 +39,39 @@
     csr_write(csr, v_);             \
     csr_write(csr ## H, v_ >> 32);  \
 })
+
+/*
+ * The two halves are read by separate instructions, so a CSR which hardware
+ * increments can carry from the low half into the high one in between,

Quoted 15 lines · Oleksii Kurochko

On RV32 a 64-bit CSR is accessed as two halves, <csr> and <csr>H. Reading the halves with two separate instructions isn't always safe: for a CSR which hardware increments, a carry from the low half into the high one can happen in between, so a plain pair of reads can produce a value the CSR never held. Therefore the high half is re-read and the sequence retried if it changed in the meantime.

Only CSRs which change under Xen's feet need that, hence the name. The majority can be read with a plain pair of csr_read()s, so no helper for those is introduced here as there would be no user for it.

Use it for CSR_TIME, which is exactly such a counter, and widen cycles_t to uint64_t. Otherwise get_cycles() would still truncate the time counter to 32 bits on RV32.

Quoted 4 lines

No functional change on RV64.

Fixes a541ddadec0a ("xen/riscv: introduce time.h")

Signed-off-by Oleksii Kurochko <[email protected]>

Reviewed-by Jan Beulich <[email protected]>

Nevertheless, a few remarks below.

Quoted 10 lines

xen/arch/riscv/include/asm/csr.h +4 -0

@@ -39,12 +39,39 @@
      csr_write(csr, v_);             \
      csr_write(csr ## H, v_ >> 32);  \
  })
+
+/*
+ * The two halves are read by separate instructions, so a CSR which hardware
+ * increments can carry from the low half into the high one in between,

xen/arch/riscv/include/asm/csr.h line 55

Quoted 10 lines

xen/arch/riscv/include/asm/csr.h +10 -0

+ * yielding a value the CSR never held. Re-read the high half and retry the
+ * sequence if it changed.
+ *
+ * Only needed for CSRs which change under Xen's feet.
+ */
+#define csr_volatile_read64(csr)                    \
+({                                                  \
+    uint32_t hi_, lo_, old_;                        \
+                                                    \
+    hi_ = csr_read(csr ## H);                       \

This could be the initializer of the variable.

xen/arch/riscv/include/asm/csr.h line 67

Quoted 12 lines

xen/arch/riscv/include/asm/csr.h +7 -0

+    do {                                            \
+        old_ = hi_;                                 \
+        lo_ = csr_read(csr);                        \
+    } while ( (hi_ = csr_read(csr ## H)) != old_ ); \
+                                                    \
+    ((uint64_t)hi_ << 32) | lo_;                    \
+})
  #else
  #define csr_write64(csr, val)       \
  ({                                  \
      csr_write(csr, val);            \
      (void)csr ## H;                 \

While I notice the same exists here already, I wonder what the purpose of this and ...

xen/arch/riscv/include/asm/csr.h line 72

Quoted 6 lines

xen/arch/riscv/include/asm/csr.h +4 -0

  })
+
+#define csr_volatile_read64(csr)    \
+({                                  \
+    (void)csr ## H;                 \

... this is (and why their placements differ). Generally a pure 64-bit environment would have no need for these ...H constants, and their availability looks to be all that's checked for here. As Baptiste did point out on patch 11, these uses constitute Misra violations, which we then would need to deviate. I think we want to try to limit the number of needed deviations.

And then, seeing how patch 11 introduces an exact copy of this as csr_read64() - can't we simply have

#define csr_volatile_read64 csr_read64

?

Quoted 37 lines · Jan Beulich

Quoted 10 lines · Oleksii Kurochko

xen/arch/riscv/include/asm/csr.h +4 -0

@@ -39,12 +39,39 @@
      csr_write(csr, v_);             \
      csr_write(csr ## H, v_ >> 32);  \
  })
+
+/*
+ * The two halves are read by separate instructions, so a CSR which hardware
+ * increments can carry from the low half into the high one in between,

xen/arch/riscv/include/asm/csr.h line 55

Quoted 10 lines

xen/arch/riscv/include/asm/csr.h +10 -0

+ * yielding a value the CSR never held. Re-read the high half and retry the
+ * sequence if it changed.
+ *
+ * Only needed for CSRs which change under Xen's feet.
+ */
+#define csr_volatile_read64(csr)                    \
+({                                                  \
+    uint32_t hi_, lo_, old_;                        \
+                                                    \
+    hi_ = csr_read(csr ## H);                       \

This could be the initializer of the variable.

xen/arch/riscv/include/asm/csr.h line 67

Quoted 12 lines

xen/arch/riscv/include/asm/csr.h +7 -0

+    do {                                            \
+        old_ = hi_;                                 \
+        lo_ = csr_read(csr);                        \
+    } while ( (hi_ = csr_read(csr ## H)) != old_ ); \
+                                                    \
+    ((uint64_t)hi_ << 32) | lo_;                    \
+})
  #else
  #define csr_write64(csr, val)       \
  ({                                  \
      csr_write(csr, val);            \
      (void)csr ## H;                 \

Quoted 2 lines

While I notice the same exists here already, I wonder what the purpose of this and ...

I can drop it in separate patch. The purpose was just for the symmetry with RV32 case.

Quoted 13 lines

xen/arch/riscv/include/asm/csr.h line 72

Quoted 6 lines

xen/arch/riscv/include/asm/csr.h +4 -0

  })
+
+#define csr_volatile_read64(csr)    \
+({                                  \
+    (void)csr ## H;                 \

... this is (and why their placements differ). Generally a pure 64-bit environment would have no need for these ...H constants, and their availability looks to be all that's checked for here. As Baptiste did point out on patch 11, these uses constitute Misra violations, which we then would need to deviate. I think we want to try to limit the number of needed deviations.

If I understood MISRA Rule 20.12 correctly (Baptiste pointed me to it on patch 11), the issue is that the csr parameter is used both as an operand of ## and in ordinary, macro-expanded context.

I think it would be better to just rewrite these macros so that no deviation is needed:

#ifdef CONFIG_RISCV_32
#define csr_write64(name, val) \

({                                              \
     uint64_t v_ = (val);                        \
     csr_write(CSR_ ## name, v_);                \
     csr_write(CSR_ ## name ## H, v_ >> 32);     \
})

#define csr_read64(name) \

({                                              \
     uint64_t v_ = csr_read(CSR_ ## name ## H);  \
                                                 \
     (v_ << 32) | csr_read(CSR_ ## name);        \
})
/* csr_volatile_read64() similarly */
#else
#define csr_write64(name, val)      \
({                                  \
     csr_write(CSR_ ## name, val);   \
     (void)CSR_ ## name ## H;        \
})

...
#endif

For RV64, '(void)CSR_ ## name ## H;' could be dropped altogether, and csr_volatile_read64() would be reworked in the same way.

IIUC, with such definitions there is no violation of MISRA Rule 20.12.

vmid_handle_vmenter() reports that no flush is needed when VMIDs are unavailable (e.g. vmid=off, or hardware with no more than one VMID bit). Every domain then runs under VMID 0 with nothing flushed in between, so as soon as a hart runs more than one domain, a domain entered there can use the G-stage translations left behind by the domain which ran before it.

Report that a flush is needed on every entry instead.

[...]

Quoted 6 lines · Oleksii Kurochko

vmid_handle_vmenter() reports that no flush is needed when VMIDs are unavailable (e.g. vmid=off, or hardware with no more than one VMID bit). Every domain then runs under VMID 0 with nothing flushed in between, so as soon as a hart runs more than one domain, a domain entered there can use the G-stage translations left behind by the domain which ran before it.

Quoted 3 lines

Report that a flush is needed on every entry instead.

[...]

Reviewed-by Baptiste Le Duc <[email protected]>

vstimecmp is a 64-bit CSR independently of XLEN, which is why it is written with csr_write64(). On RV32 that macro splits the value into the vstimecmp/vstimecmph pair, so passing ULONG_MAX (0xffffffff there) writes all ones to the low half and zero to the high half, leaving the CSR at 0x00000000ffffffff rather than at its maximum. A VS-timer irq would then become pending as soon as (time + htimedelta) reaches 2^32, which is exactly what the code is trying to avoid.

Use ~0 instead: csr_write64() converts its argument to uint64_t, and the conversion of the signed -1 sign-extends to all ones, so both halves get 0xffffffff. On RV64 the result is equal to ULONG_MAX, so no functional change there.

Fixes 25e032730690 ("xen/riscv: allow Xen to use SSTC while hiding it from guests")

Signed-off-by Oleksii Kurochko <[email protected]>

Reviewed-by Baptiste Le Duc <[email protected]>

A RISC-V guest can be given several emulated devices at distinct GPA ranges; the virtual interrupt controllers alone account for vPLIC on legacy machines and vAPLIC together with vIMSIC on AIA-compliant ones. Routing MMIO faults via a per-device is_access() check in the trap handler would couple that handler to every device it must serve, requiring a new conditional branch in the fault path for each emulated device added.

Introduce a per-domain MMIO handler registration table, modeled after the equivalent ARM framework, so that virtual devices self-register their GPA ranges and read/write callbacks at domain creation time. The MMIO fault path delegates to a single do_mmio() entry point and remains agnostic of which device owns a particular address.

Nothing registers a handler and do_mmio() has no callers yet, so this patch is a no-op; the trap handler is left untouched.

Signed-off-by Oleksii Kurochko <[email protected]>

---
do_mmio() gets its first caller in "xen/riscv: add guest load emulation for trapped MMIO accesses" in this series.
---
Changes in v3:
- Update the commit message.
- Capitalize the comments inside mmio_info_t and drop the list of access widths from the one on len, as it could go stale.
- Drop the parentheses around the typedef names.
- Reject an access width which isn't a power of 2 in do_mmio().
- Fold the !size check of register_mmio_handler() into end <= addr.
- Return -EADDRINUSE instead of -EEXIST for an overlapping region.
- Drop setting d->arch.vmmio.num_entries to 0, as struct domain starts out zero-filled.
---
Changes in v2:
- Drop copyright from mmio.c as it will go stale anyway as code moves around.
- Drop the max_count parameter of domain_io_init() (MAX_IO_HANDLER is a global boundary) and embed the handler array directly in struct vmmio as struct mmio_handler handlers[MAX_IO_HANDLER]. This removes the xvzalloc_array() allocation, the max_num_entries field and domain_io_free() altogether; domain_io_init() consequently cannot fail any longer and now returns void. Note this goes slightly beyond the suggested variant in that max_num_entries is dropped as well, since ARRAY_SIZE(vmmio->handlers) serves the same purpose.
- Drop the separate register_t argument of mmio_read_t/mmio_write_t; handlers now produce and consume the value through info->data. handle_read()/handle_write() are gone as a result, with try_handle_mmio() invoking ops->read()/ops->write() directly.
- Turn mmio_read_t/mmio_write_t into function types rather than pointer-to-function types, so that pointer-ness is visible at the use sites in struct mmio_handler_ops. Constify the mmio_info_t * of the write callback, which has no reason to modify it any more.
- register_mmio_handler() returns int instead of BUG_ON()ing on a full
table: -ENOSPC now lets the caller fail domain creation. It also validates its inputs, rejecting a NULL ops (or one with a missing read/write callback) as well as zero-sized and address-wrapping regions with -EINVAL.
- Guarantee the non-overlap property that cmp_mmio_handler() relies on:
register_mmio_handler() checks the new region against both neighbours of its insertion slot and returns -EEXIST on overlap.
- Replace the sort() call per registration with an insertion into the already sorted array: locate the slot and memmove() the tail up by one. sort(), swap_mmio_handler() and <xen/sort.h> are gone.
- Extend cmp_mmio_handler()'s comment to state that it is a bsearch() comparator and to explain the key/elem asymmetry; document why the neighbours' addr + size cannot overflow.
- Fix over-long lines and a mis-indented label.
---

---
 xen/arch/riscv/Makefile             |   1 +
 xen/arch/riscv/domain.c             |   3 +
 xen/arch/riscv/include/asm/domain.h |   3 +
 xen/arch/riscv/include/asm/mmio.h   |  63 ++++++++++
 xen/arch/riscv/mmio.c               | 175 ++++++++++++++++++++++++++++
 5 files changed, 245 insertions(+)
 create mode 100644 xen/arch/riscv/include/asm/mmio.h
 create mode 100644 xen/arch/riscv/mmio.c

xen/arch/riscv/Makefile +1 -0

index 665ec26933..5454517724 100644
@@ -14,6 +14,7 @@ obj-y += intc.o
 obj-y += irq.o
 obj-y += kernel.init.o
 obj-y += mm.o
+obj-y += mmio.o
 obj-y += p2m.o
 obj-y += paging.o
 obj-y += pt.o

xen/arch/riscv/domain.c +3 -0

index d94652809e..fd62296356 100644
@@ -12,6 +12,7 @@
 #include <asm/cpufeature.h>
 #include <asm/csr.h>
 #include <asm/intc.h>
+#include <asm/mmio.h>
 #include <asm/riscv_encoding.h>
 #include <asm/vtimer.h>
 

@@ -310,6 +311,8 @@ int arch_domain_create(struct domain *d,
     if ( (rc = p2m_init(d, config)) != 0)
         goto fail;
 
+    domain_io_init(d);
+
     if ( (rc = domain_vintc_init(d)) )
         goto fail;
 

xen/arch/riscv/include/asm/domain.h +3 -0

index e035b33ddf..15e8fa1968 100644
@@ -9,6 +9,7 @@
 
 #include <asm/cpufeature.h>
 #include <asm/guest-layout.h>
+#include <asm/mmio.h>
 #include <asm/p2m.h>
 #include <asm/vtimer.h>
 

@@ -101,6 +102,8 @@ struct arch_domain {
     const unsigned long *isa;
 
     struct vintc *vintc;
+
+    struct vmmio vmmio;
 };
 
 #include <xen/sched.h>

xen/arch/riscv/include/asm/mmio.h new file +0 -0

new file mode 100644
index 0000000000..1bf48571a9

xen/arch/riscv/include/asm/mmio.h new file +10 -0

+#define RISCV_MMIO_H
+
+#include <xen/lib.h>
+#include <xen/rwlock.h>
+
+struct domain;
+struct vcpu;
+
+#define MAX_IO_HANDLER  16
+

xen/arch/riscv/include/asm/mmio.h new file +41 -0

+    IO_ABORT,       /* The IO was handled and led to an abort. */
+    IO_HANDLED,     /* The IO was successfully handled. */
+    IO_UNHANDLED,   /* No handler found for the IO. */
+};
+
+typedef enum io_state mmio_read_t(struct vcpu *v, mmio_info_t *info);
+typedef enum io_state mmio_write_t(struct vcpu *v, const mmio_info_t *info);
+
+struct mmio_handler_ops {
+    mmio_read_t *read;
+    mmio_write_t *write;
+};
+
+struct mmio_handler {
+    paddr_t addr;
+    paddr_t size;
+    const struct mmio_handler_ops *ops;
+};
+
+struct vmmio {
+    unsigned int num_entries;
+    rwlock_t lock;
+    struct mmio_handler handlers[MAX_IO_HANDLER];
+};
+
+int do_mmio(mmio_info_t *info, paddr_t fault_addr, unsigned int len);
+int register_mmio_handler(struct domain *d,
+                          const struct mmio_handler_ops *ops,
+                          paddr_t addr, paddr_t size);
+void domain_io_init(struct domain *d);
+
+#endif /* RISCV_MMIO_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */

xen/arch/riscv/mmio.c new file +55 -0

new file mode 100644
index 0000000000..cf739a790a
@@ -0,0 +1,175 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include <xen/bsearch.h>
+#include <xen/lib.h>
+#include <xen/rwlock.h>
+#include <xen/sched.h>
+#include <xen/string.h>
+
+#include <asm/current.h>
+#include <asm/mmio.h>
+
+/*
+ * bsearch() comparator: @key holds the address to look up in its addr field,
+ * @elem is an entry of vmmio->handlers. Relies on the regions not
+ * overlapping, which register_mmio_handler() enforces.
+ */
+static int cmp_mmio_handler(const void *key, const void *elem)
+{
+    const struct mmio_handler *handler0 = key;
+    const struct mmio_handler *handler1 = elem;
+
+    if ( handler0->addr < handler1->addr )
+        return -1;
+
+    if ( handler0->addr >= (handler1->addr + handler1->size) )
+        return 1;
+
+    return 0;
+}
+
+/*
+ * Return a copy of the matching handler rather than a pointer into
+ * vmmio->handlers: a concurrent register_mmio_handler() shifts entries
+ * up to keep the array sorted, so an escaped pointer could refer to a
+ * different (or torn) entry once the lock is dropped. The copy stays
+ * valid as the ops structures are never freed.
+ */
+static bool find_mmio_handler(struct domain *d, paddr_t gpa,
+                              struct mmio_handler *out)
+{
+    struct vmmio *vmmio = &d->arch.vmmio;
+    struct mmio_handler key = { .addr = gpa };
+    const struct mmio_handler *handler;
+
+    read_lock(&vmmio->lock);
+    handler = bsearch(&key, vmmio->handlers, vmmio->num_entries,
+                      sizeof(*handler), cmp_mmio_handler);
+    if ( handler )
+        *out = *handler;
+    read_unlock(&vmmio->lock);
+
+    return handler != NULL;
+}
+
+static enum io_state try_handle_mmio(mmio_info_t *info)

xen/arch/riscv/include/asm/mmio.h line 2

Quoted 5 lines

xen/arch/riscv/include/asm/mmio.h new file +2 -0

@@ -0,0 +1,63 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef RISCV_MMIO_H

Should be ASM__RISCV_MMIO_H instead to be consistant with other header files in riscv/include/asm.

Quoted 7 lines · Baptiste Le Duc

xen/arch/riscv/include/asm/mmio.h line 2

Quoted 5 lines

xen/arch/riscv/include/asm/mmio.h new file +2 -0

@@ -0,0 +1,63 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef RISCV_MMIO_H

Should be ASM__RISCV_MMIO_H instead to be consistant with other header files in riscv/include/asm.

No, please see the bottom of ./CODING_STYLE. It's many of the other headers which would want updating (eventually; there's no urgency I think).

xen/arch/riscv/include/asm/mmio.h line 2

Quoted 8 lines

xen/arch/riscv/include/asm/mmio.h new file +2 -0

new file mode 100644
index 0000000000..1bf48571a9
@@ -0,0 +1,63 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef RISCV_MMIO_H

Should be ASM__RISCV_MMIO_H instead to be consistant with other header files in riscv/include/asm.

Quoted 13 lines

xen/arch/riscv/include/asm/mmio.h new file +13 -0

+#define RISCV_MMIO_H
+
+#include <xen/lib.h>
+#include <xen/rwlock.h>
+
+struct domain;
+struct vcpu;
+
+#define MAX_IO_HANDLER  16
+
+typedef struct {
+    paddr_t gpa;
+    unsigned int len;  /* Access width in bytes */

xen/arch/riscv/include/asm/mmio.h line 22

Quoted 7 lines

xen/arch/riscv/include/asm/mmio.h new file +7 -0

+    bool is_write;
+    /* Store: value to write; load: value read (set by handler) */
+    register_t data;
+} mmio_info_t;
+
+enum io_state
+{

Style: should be directly after enum io_state.

Quoted 5 lines

xen/arch/riscv/include/asm/mmio.h new file +5 -0

+    IO_ABORT,       /* The IO was handled and led to an abort. */
+    IO_HANDLED,     /* The IO was successfully handled. */
+    IO_UNHANDLED,   /* No handler found for the IO. */
+};
+

xen/arch/riscv/include/asm/mmio.h

Quoted 11 lines

xen/arch/riscv/include/asm/mmio.h new file +11 -0

+static enum io_state try_handle_mmio(mmio_info_t *info)
+{
+    struct vcpu *v = current;
+    struct mmio_handler handler = {};
+
+    if ( !find_mmio_handler(v->domain, info->gpa, &handler) )
+        return IO_UNHANDLED;
+
+    if ( info->is_write )
+        return handler.ops->write(v, info);
+    else

The else is not necessary here, misra would call that "dead code". Here are two suggestions:

       if ( info->is_write )
           return handler.ops->write(v, info);

       return handler.ops->read(v, info);

or:

       return info->is_write ? handler.ops->write(v, info)
                             : handler.ops->read(v, info);

xen/arch/riscv/include/asm/domain.h +1 -0

@@ -9,6 +9,7 @@
 
 #include <asm/cpufeature.h>
 #include <asm/guest-layout.h>
+#include <asm/mmio.h>
 #include <asm/p2m.h>
 #include <asm/vtimer.h>
 

xen/arch/riscv/mmio.c new file +28 -0

@@ -0,0 +1,175 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include <xen/bsearch.h>
+#include <xen/lib.h>
+#include <xen/rwlock.h>
+#include <xen/sched.h>
+#include <xen/string.h>
+
+#include <asm/current.h>
+#include <asm/mmio.h>
+
+/*
+ * bsearch() comparator: @key holds the address to look up in its addr field,
+ * @elem is an entry of vmmio->handlers. Relies on the regions not
+ * overlapping, which register_mmio_handler() enforces.
+ */
+static int cmp_mmio_handler(const void *key, const void *elem)
+{
+    const struct mmio_handler *handler0 = key;
+    const struct mmio_handler *handler1 = elem;
+
+    if ( handler0->addr < handler1->addr )
+        return -1;
+
+    if ( handler0->addr >= (handler1->addr + handler1->size) )
+        return 1;
+
+    return 0;

xen/arch/riscv/mmio.c new file +9 -0

+                              struct mmio_handler *out)
+{
+    struct vmmio *vmmio = &d->arch.vmmio;
+    struct mmio_handler key = { .addr = gpa };
+    const struct mmio_handler *handler;
+
+    read_lock(&vmmio->lock);
+    handler = bsearch(&key, vmmio->handlers, vmmio->num_entries,
+                      sizeof(*handler), cmp_mmio_handler);

Quoted 13 lines · Oleksii Kurochko

xen/arch/riscv/include/asm/domain.h +1 -0

@@ -9,6 +9,7 @@
 
  #include <asm/cpufeature.h>
  #include <asm/guest-layout.h>
+#include <asm/mmio.h>
  #include <asm/p2m.h>
  #include <asm/vtimer.h>
 

@@ -101,6 +102,8 @@ struct arch_domain {
      const unsigned long *isa;
 

xen/arch/riscv/include/asm/domain.h struct arch_domain { · line 106

Quoted 3 lines

xen/arch/riscv/include/asm/domain.h +2 -0

      struct vintc *vintc;
+
+    struct vmmio vmmio;

This is a decent size structure, so I wonder whether this really wants embedding in struct domain. If it was a pointer here (with the struct dynamically allocated), the struct definition itself may not even need exposing in a header.

Quoted 36 lines

xen/arch/riscv/mmio.c new file +32 -0

@@ -0,0 +1,175 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include <xen/bsearch.h>
+#include <xen/lib.h>
+#include <xen/rwlock.h>
+#include <xen/sched.h>
+#include <xen/string.h>
+
+#include <asm/current.h>
+#include <asm/mmio.h>
+
+/*
+ * bsearch() comparator: @key holds the address to look up in its addr field,
+ * @elem is an entry of vmmio->handlers. Relies on the regions not
+ * overlapping, which register_mmio_handler() enforces.
+ */
+static int cmp_mmio_handler(const void *key, const void *elem)
+{
+    const struct mmio_handler *handler0 = key;
+    const struct mmio_handler *handler1 = elem;
+
+    if ( handler0->addr < handler1->addr )
+        return -1;
+
+    if ( handler0->addr >= (handler1->addr + handler1->size) )
+        return 1;
+
+    return 0;
+}
+
+/*
+ * Return a copy of the matching handler rather than a pointer into

xen/arch/riscv/mmio.c line 38

Quoted 6 lines

xen/arch/riscv/mmio.c new file +6 -0

+ * vmmio->handlers: a concurrent register_mmio_handler() shifts entries
+ * up to keep the array sorted, so an escaped pointer could refer to a
+ * different (or torn) entry once the lock is dropped. The copy stays
+ * valid as the ops structures are never freed.
+ */
+static bool find_mmio_handler(struct domain *d, paddr_t gpa,

The first parameter here apparently wants to be named currd, much like ...

Quoted 15 lines

xen/arch/riscv/mmio.c new file +14 -0

+                              struct mmio_handler *out)
+{
+    struct vmmio *vmmio = &d->arch.vmmio;
+    struct mmio_handler key = { .addr = gpa };
+    const struct mmio_handler *handler;
+
+    read_lock(&vmmio->lock);
+    handler = bsearch(&key, vmmio->handlers, vmmio->num_entries,
+                      sizeof(*handler), cmp_mmio_handler);
+    if ( handler )
+        *out = *handler;
+    read_unlock(&vmmio->lock);
+
+    return handler != NULL;

xen/arch/riscv/mmio.c line 57

Quoted 5 lines

xen/arch/riscv/mmio.c new file +5 -0

+}
+
+static enum io_state try_handle_mmio(mmio_info_t *info)
+{
+    struct vcpu *v = current;

... this variable wants to be named curr. Unless of course there are plans to not unconditionally use current here.

Quoted 11 lines

xen/arch/riscv/mmio.c new file +10 -0

+int register_mmio_handler(struct domain *d,
+                          const struct mmio_handler_ops *ops,
+                          paddr_t addr, paddr_t size)
+{
+    struct vmmio *vmmio = &d->arch.vmmio;
+    struct mmio_handler *handlers = vmmio->handlers;
+    paddr_t end = addr + size;
+    unsigned int i;
+    int rc = 0;
+    bool overlap;

xen/arch/riscv/mmio.c line 112

Quoted 3 lines

xen/arch/riscv/mmio.c new file +3 -0

+
+    if ( !ops || !ops->read || !ops->write || end <= addr )
+        return -EINVAL;

I think this checking goes a little too far: It's absurd for some call site to pass NULL into this function. The other three checks are sufficiently plausible, albeit I wonder whether this wouldn't want to be an assertion then.

Introduce CPU_NONE, a CPU number which doesn't identify any physical CPU, to be used wherever a pCPU has to be recorded but there may be none.

Its first user is vsfile_cpu in struct vimsic_state, which records the pCPU whose h/w guest interrupt file backs the vCPU's IMSIC VS-file. NR_CPUS has been used there as the value meaning that there is no such pCPU, i.e. that the s/w VS-file is in use. NR_CPUS is however an upper

bound rather than a "no CPU" marker, and it already carries other
meanings: hartid_to_cpuid() returns it for a hart Xen doesn't know, and pcpu_info[] uses it for an entry whose processor_id isn't valid yet. Use CPU_NONE for vsfile_cpu instead, so that the s/w VS-file case can't be mistaken for any of those.

Quoted 8 lines

Introduce CPU_NONE, a CPU number which doesn't identify any physical CPU, to be used wherever a pCPU has to be recorded but there may be none.

Its first user is vsfile_cpu in struct vimsic_state, which records the pCPU whose h/w guest interrupt file backs the vCPU's IMSIC VS-file. NR_CPUS has been used there as the value meaning that there is no such pCPU, i.e. that the s/w VS-file is in use. NR_CPUS is however an upper

Nit: It's hard to understand when reading the message why s/w is mentioning. I'd reword it to:

     ```
     NR_CPUS has been used there to mean that no h/w guest interrupt file is
     attached (i.e. the vCPU uses the s/w VS-file), so there is no pCPU to
     record.
     ```

Quoted 5 lines

bound rather than a "no CPU" marker, and it already carries other
meanings: hartid_to_cpuid() returns it for a hart Xen doesn't know, and pcpu_info[] uses it for an entry whose processor_id isn't valid yet. Use CPU_NONE for vsfile_cpu instead, so that the s/w VS-file case can't be mistaken for any of those.

Nit: Same as above, I'd prefer something like:

     ```
     Use CPU_NONE for vsfile_cpu instead, so that "no h/w guest interrupt
     file attached" can't be mistaken for an unknown hart or a not-yet-valid
     pcpu_info[] entry.
     ```

Reviewed-by Baptiste Le Duc <[email protected]>

Introduce CPU_NONE, a CPU number which doesn't identify any physical CPU, to be used wherever a pCPU has to be recorded but there may be none.

Its first user is vsfile_cpu in struct vimsic_state, which records the pCPU whose h/w guest interrupt file backs the vCPU's IMSIC VS-file. NR_CPUS has been used there as the value meaning that there is no such pCPU, i.e. that the s/w VS-file is in use. NR_CPUS is however an upper bound rather than a "no CPU" marker, and it already carries other
meanings: hartid_to_cpuid() returns it for a hart Xen doesn't know, and pcpu_info[] uses it for an entry whose processor_id isn't valid yet.

Use CPU_NONE for vsfile_cpu instead, so that the s/w VS-file case can't be mistaken for any of those.

xen/arch/riscv/include/asm/config.h +3 -0

@@ -136,6 +136,9 @@
 #define OPT_CONSOLE_STR "dtuart"
 #define INVALID_VCPU_ID MAX_VIRT_CPUS
 
+/* A CPU number which doesn't identify any physical CPU. */
+#define CPU_NONE (~0U)
+
 /* Linkage for RISCV */
 #ifdef __ASSEMBLER__
 #define CODE_FILL /* empty */

Quoted 11 lines · Oleksii Kurochko

Introduce CPU_NONE, a CPU number which doesn't identify any physical CPU, to be used wherever a pCPU has to be recorded but there may be none.

Its first user is vsfile_cpu in struct vimsic_state, which records the pCPU whose h/w guest interrupt file backs the vCPU's IMSIC VS-file. NR_CPUS has been used there as the value meaning that there is no such pCPU, i.e. that the s/w VS-file is in use. NR_CPUS is however an upper bound rather than a "no CPU" marker, and it already carries other
meanings: hartid_to_cpuid() returns it for a hart Xen doesn't know, and pcpu_info[] uses it for an entry whose processor_id isn't valid yet.

Yet these could (and imo should, subsequently) entirely plausibly also be switched to CPU_NONE. IOW I don't think ...

Quoted 2 lines

Use CPU_NONE for vsfile_cpu instead, so that the s/w VS-file case can't be mistaken for any of those.

... this is a plausible justification (nor in its re-written form as per Baptiste's suggestion).

If you want such a distinction, you'll need separate sentinels for every one such field.

Quoted 15 lines

xen/arch/riscv/include/asm/config.h +3 -0

@@ -136,6 +136,9 @@
  #define OPT_CONSOLE_STR "dtuart"
  #define INVALID_VCPU_ID MAX_VIRT_CPUS
 
+/* A CPU number which doesn't identify any physical CPU. */
+#define CPU_NONE (~0U)
+
  /* Linkage for RISCV */
  #ifdef __ASSEMBLER__
  #define CODE_FILL /* empty */

xen/arch/riscv/include/asm/imsic.h struct vimsic_state { · line 77

Quoted 8 lines

xen/arch/riscv/include/asm/imsic.h +1 -1

@@ -71,7 +71,7 @@ struct vimsic_state {
       */
      unsigned int guest_file_id;
      /*
-     * s/w IMSIC VS-file -> vsfile_cpu == NR_CPUS
+     * s/w IMSIC VS-file -> vsfile_cpu == CPU_NONE
       * h/w IMSIC VS-file -> vsfile_cpu < NR_CPUS
       */
      unsigned int vsfile_cpu;

As can be seen from the comment alone, there's now the risk of CPU_NONE < NR_CPUS, at which point users of the field will be screwed. I think you want

     BUILD_BUG_ON(CPU_NONE < NR_CPUS);

somewhere.

Guests running under Xen program interrupt routing by writing to APLIC MMIO registers. Xen must intercept these accesses to enforce interrupt isolation between domains and to translate guest routing intent into the underlying physical MSI topology.

Register the vAPLIC range with the MMIO dispatch and emulate the accesses:
- reads and writes are masked with the domain's authorised interrupt bitmap, so that a guest can neither observe nor affect interrupts it does not own;
- target registers are shadowed per source, so that a guest reads back what it wrote. The h/w register instead gets the hart index and guest interrupt file of the pCPU the target vCPU's interrupt file lives on, as the APLIC uses these fields directly to compute the MSI address. As long as the target vCPU has no h/w guest interrupt file, only the shadow copy is updated;
- only MSI delivery mode is emulated: target writes made while the guest's domaincfg.DM is clear are ignored.

Delegation (APLIC_SOURCECFG_D) is not yet supported.

Co-developed-by Romain Caritey <[email protected]>

Signed-off-by Oleksii Kurochko <[email protected]>

---
Changes in v3:
- sourcecfg store emulation: mask the guest value with the new APLIC_SOURCECFG_WMASK (D and SM are the only implemented fields, all the other bits are reserved and read as zero) instead of rejecting the write when the value is above APLIC_SOURCECFG_SM_LEVEL_LOW. The dropped check compared the whole value against 7 rather than the SM field, so a legal SM with anything set in the reserved bits 9:3 was rejected. SM is WARL and vAPLIC keeps no shadow copy of sourcecfg[], so a reserved SM value is left to the h/w to normalize; add a TODO covering what will have to be emulated here once sourcecfg[] is shadowed.
- Introduce APLIC_SOURCECFG_WMASK in asm/aplic.h.
- target store emulation: reword the comment about a non-zero guest index to say that such a write is illegal and is therefore ignored as a whole, that the shadow copy keeps the zero it was allocated with, and that aplic_msi_target_gen() overwrites the field with vcpu_guest_file_id() anyway. Drop the now-redundant "Ignore such writes ..." comment.
- s/aplic_hart_field/aplic_hart_index/ (and the local variable with it):
the value is the APLIC "Hart Index", not a field of it.
- Clarify the MSI target address documentation above aplic_hart_index():
name the Guest Index in the address formula, say that the APLIC computes the MSI target address itself in MSI delivery mode, and label the two parts of the 14-bit Hart Index field as g and h to match the surrounding text.
- Don't add the MSI target address scheme diagram to asm/imsic.h: it would duplicate the one documented above aplic_hart_index().
- Fix the comment on APLIC_TARGET_IPRIO: the field exists when the target is in direct delivery mode (domaincfg.DM = 0), not "in DM mode".
- Rearrange the case blocks of vaplic_emulate_load() and vaplic_emulate_store() primarily by register offset: APLIC_DOMAINCFG comes first and APLIC_SOURCECFG handling directly follows it; the set/clr-num registers are also listed in offset order. No functional change.
- Rename the first parameter of aplic_msi_target_gen() from target_vcpu to v.
- Use a const-qualified pointer in aplic_hw_read_reg()'s cast to match readl()'s prototype.
- Put APLIC_xMSICFGADDR_PPN_HHX_MASK(), APLIC_xMSICFGADDR_PPN_LHX_MASK() and APLIC_xMSICFGADDR_PPN_LHX_SHIFT() on a single line each.
- Use gdprintk() instead of dprintk() for guest-triggered messages in vaplic.c and drop the redundant "is passed" from the word_idx message.
- Call domain_vaplic_deinit() in both failure paths of domain_vaplic_init() instead of open-coding the clean-up.
- aplic_msi_target_gen() takes the hart index from the pCPU the vCPU's interrupt file lives on (vsfile_cpu) instead of v->processor, which may already name the new pCPU while the vCPU is being migrated. Callers have to hold vsfile_lock until the value is written to the h/w, which the vAPLIC target write path now does.
- A target register write for a vCPU which has no h/w guest interrupt file yet (its vsfile_cpu is CPU_NONE) only updates the stored copy; the h/w register isn't programmed, as there is neither a hart nor a guest interrupt file to point it at. Add a TODO about s/w guest interrupt files, for which the h/w register will have to be written too.
- Turn vaplic_regs.target[] into vaplic_regs.sources[], an array of struct vaplic_source holding the guest's view of the target register together with a per-source lock which keeps the stored copy and the h/w target register updated as a pair. Document how many elements it holds and why index 0 is unused.
- Drop the emulation of target register writes in direct delivery mode:
such writes are ignored with a one-time warning. Resolve the target vCPU only in the MSI delivery mode path, where it is used.
---
Changes in v2:
- Shadow the guest-written target registers in a per-domain array and serve reads of APLIC_TARGET_* from it: the hardware register holds the value produced by aplic_msi_target_gen() (physical hart field, VS-file id), so reading it back would expose the host layout and return something the guest never wrote. A non-zero guest index is dropped with a one-time warning instead of being echoed back.
- aplic_hart_field(): take a CPU id instead of a hartid and derive both the group and the hart index from msi->base_addr + msi->offset - the hart index bits are a part of that offset, so the hartid can't be used as the hart index. Add APLIC_xMSICFGADDR_PPN_LHX_{MASK,SHIFT} for that.
- Document the IMSIC MSI target address layout and the APLIC hart index packing above aplic_hart_field(), and correct the corresponding diagram in asm/imsic.h.
- Drop the mask parameter of aplic_hw_read_reg() and apply the authorization mask in vaplic_emulate_load() instead.
- vaplic_emulate_{load,store}(): return bool instead of int, rename v/d to curr/currd and add ASSERT(curr == current).
- Use domain_vcpu() instead of open-coded d->vcpu[] indexing when resolving the target hart index.
- s/APLIC_REG_OFFSET_MASK/APLIC_CTRL_REGION_OFFSET_MASK/.
- Update store emulation handling for target register to be able to deal with target format in both cases (MSI and Direct).
- Update store emulation handling of domaincfg. There is no need to force DM/IE mode here (it will be forced/checked on Xen irq handler side).
- Rename APLIC_DEFAULT_PRIORITY and move to aplic.h as default prioity value is used in vaplic code too now.
---

---
 xen/arch/riscv/aplic-priv.h         |   2 +
 xen/arch/riscv/aplic.c              | 138 +++++++++-
 xen/arch/riscv/include/asm/aplic.h  |  37 +++
 xen/arch/riscv/include/asm/vaplic.h |  19 ++
 xen/arch/riscv/vaplic.c             | 384 +++++++++++++++++++++++++++-
 5 files changed, 575 insertions(+), 5 deletions(-)

xen/arch/riscv/aplic-priv.h +2 -0

index 35100d3a64..2245029b10 100644
@@ -47,4 +47,6 @@ struct aplic_priv {
  */
 extern unsigned int guest_aplic_num_sources;
 
+uint32_t aplic_msi_target_gen(const struct vcpu *v, uint32_t base_val);
+
 #endif /* ASM_RISCV_APLIC_PRIV_H */

xen/arch/riscv/aplic.c +0 -0

index 17c96177a3..febc451760 100644

xen/arch/riscv/aplic.c +0 -2

 #include <xen/spinlock.h>
 #include <xen/types.h>
 #include <xen/vmap.h>

@@ -28,8 +29,6 @@
 #include <asm/io.h>
 #include <asm/riscv_encoding.h>
 
-#define APLIC_DEFAULT_PRIORITY  1
-
 static struct aplic_priv aplic = {
     .lock = SPIN_LOCK_UNLOCKED,
 };

@@ -38,6 +37,137 @@ static struct intc_info __ro_after_init aplic_info = {
     .hw_variant = INTC_APLIC,

xen/arch/riscv/aplic.c +4 -0

+ *
+ *   ( Base PPN | (g << (HHXS + 12)) | (h << LHXS) | Guest Index ) << 12
+ *
+ * where Base PPN, HHXS, LHXS, HHXW and LHXW come from the {m,s}msiaddrcfg[h]

xen/arch/riscv/aplic.c +61 -0

+ * - 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
+ * 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.
+ *
+ * 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_index(unsigned int cpu)
+{
+    const struct imsic_config *imsic = imsic_get_config();
+    const struct imsic_msi *msi = &imsic->msi[cpu];
+    /* Low Hart Index Shift */
+    unsigned int lhxs = imsic->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
+     * 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;
+}
+

xen/arch/riscv/aplic.c +9 -0

+
+    ASSERT(rw_is_locked(&vimsic_state->vsfile_lock));
+    ASSERT(vimsic_state->vsfile_cpu < NR_CPUS);
+
+    hart_index = aplic_hart_index(vimsic_state->vsfile_cpu);
+
+    base_val &= APLIC_TARGET_EIID;
+    base_val |= MASK_INSR(guest_id, APLIC_TARGET_GUEST_IDX);
+    base_val |= MASK_INSR(hart_index, APLIC_TARGET_HART_IDX);

xen/arch/riscv/aplic.c +8 -2

+
+    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 +183,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);

xen/arch/riscv/include/asm/aplic.h +37 -0

index a2af55d54f..664bda2cb7 100644
@@ -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

@@ -58,6 +65,12 @@
 #define   APLIC_SOURCECFG_SM_EDGE_FALL  0x5
 #define   APLIC_SOURCECFG_SM_LEVEL_HIGH 0x6
 #define   APLIC_SOURCECFG_SM_LEVEL_LOW  0x7
+/*
+ * All other bits of sourcecfg[] are reserved and read as zero, so drop them
+ * on a write.
+ */
+#define  APLIC_SOURCECFG_WMASK          (APLIC_SOURCECFG_D | \
+                                         APLIC_SOURCECFG_SM)
 
 #define APLIC_SMSICFGADDR               0x1bc8
 #define APLIC_SMSICFGADDRH              0x1bcc

@@ -89,6 +102,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 direct delivery mode (domaincfg.DM = 0) */
+#define  APLIC_TARGET_IPRIO             GENMASK(7, 0)
+#define   APLIC_TARGET_IPRIO_DEFAULT    1U
 
 #define APLIC_IDC_SIZE                  32
 

@@ -98,6 +114,24 @@
 #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 +175,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 */

xen/arch/riscv/include/asm/vaplic.h +19 -0

index 96080bfbc2..3a555742bb 100644
@@ -11,6 +11,7 @@
 #define ASM__RISCV__VAPLIC_H
 
 #include <xen/kernel.h>
+#include <xen/spinlock.h>
 #include <xen/types.h>
 
 #include <asm/intc.h>

@@ -19,13 +20,31 @@ struct domain;
 
 #define to_vaplic(d) container_of((d)->arch.vintc, struct vaplic, vintc)
 
+/* State of one vAPLIC interrupt source. */
+struct vaplic_source {
+    /* Protects target together with the h/w target register of the source. */
+    spinlock_t lock;
+
+    /* Guest's view of the APLIC target register. */
+    uint32_t target;
+};
+
 struct vaplic_regs {
     uint32_t domaincfg;
+
+    /*
+     * Indexed by source number. The array holds d->arch.vintc->nr_virqs
+     * elements; sources[0] is unused as APLIC interrupt sources start from 1.
+     */
+    struct vaplic_source *sources;
 };
 
 struct vaplic {
     struct vintc vintc;
     struct vaplic_regs regs;
+
+    paddr_t regs_start;
+    unsigned int regs_size;
 };
 
 int domain_vaplic_init(struct domain *d);

xen/arch/riscv/vaplic.c +16 -0

index 6fbdc07805..ea4b0dc889 100644
@@ -17,6 +17,7 @@
 #include <asm/aia.h>
 #include <asm/imsic.h>
 #include <asm/intc.h>
+#include <asm/mmio.h>
 #include <asm/vaplic.h>
 
 #include "aplic-priv.h"

@@ -27,6 +28,313 @@ unsigned int __ro_after_init guest_aplic_num_sources;
 
 #define FDT_VAPLIC_INT_CELLS 2
 
+#define AUTH_IRQ_BIT(d, irqn) \
+    (((irqn) < (d)->arch.vintc->nr_virqs) && \
+     test_bit(irqn, (d)->arch.vintc->used_irqs))
+
+/*
+ * Convert a byte offset (within a SETIP/CLRIP/SETIE/CLRIE register group) to
+ * a 32-bit word index into the used_irqs bitmap. Each word covers 32
+ * interrupt sources. For SOURCECFG and TARGET groups the same division also
+ * yields the interrupt number directly, because those arrays store one 32-bit
+ * register per source.
+ */
+#define regoffset_to_word_idx(reg_val) ((reg_val) / sizeof(uint32_t))
+
+static uint32_t vaplic_target_read(const struct domain *d, unsigned int irqn)
+{

xen/arch/riscv/vaplic.c +12 -0

+                                          unsigned int word_idx)
+{
+    unsigned int first_bit = word_idx * sizeof(uint32_t) * BITS_PER_BYTE;
+
+    if ( word_idx >= DIV_ROUND_UP(currd->arch.vintc->nr_virqs,
+                                  sizeof(uint32_t) * BITS_PER_BYTE) )
+    {
+        gdprintk(XENLOG_DEBUG, "incorrect word_idx(%u)\n", word_idx);
+
+        return 0;
+    }
+

xen/arch/riscv/vaplic.c +40 -0

+    uint32_t auth_mask;
+    unsigned int i;
+
+    ASSERT(curr == current);
+
+    switch ( offset )
+    {
+    case APLIC_DOMAINCFG:
+        *out = vaplic->regs.domaincfg;
+
+        return true;
+
+    case APLIC_SETIP_BASE ... APLIC_SETIP_LAST:
+    case APLIC_CLRIP_BASE ... APLIC_CLRIP_LAST:
+    case APLIC_SETIE_BASE ... APLIC_SETIE_LAST:
+        i = regoffset_to_word_idx(offset & APLIC_SETCLR_OFFSET_MASK);
+        auth_mask = generate_auth_mask(currd, i);
+
+        break;
+
+    case APLIC_SETIPNUM:
+    case APLIC_CLRIPNUM:
+    case APLIC_SETIENUM:
+    case APLIC_CLRIE_BASE ... APLIC_CLRIE_LAST:
+    case APLIC_CLRIENUM:
+    case APLIC_SETIPNUM_LE:
+        /*
+         * Based on the RISC-V AIA spec a read of these registers
+         * always returns zero
+         */
+        *out = 0;
+
+        return true;
+
+    case APLIC_TARGET_BASE ... APLIC_TARGET_LAST:
+        /*
+         * As target registers start from 1:
+         *  0x3000 genmsi
+         *  0x3004 target[1]
+         *  0x3008 target[2]

xen/arch/riscv/vaplic.c +7 -0

+    default:
+        gdprintk(XENLOG_WARNING, "Unhandled APLIC read at offset %#x\n",
+                 offset);
+
+        return false;
+    }
+

xen/arch/riscv/vaplic.c +78 -0

+
+        return true;
+    }
+
+    case APLIC_SOURCECFG_BASE ... APLIC_SOURCECFG_LAST:
+        /*
+         * Only D (bit 10) and SM (bits 2:0) are implemented, the rest of the
+         * bits are reserved and read as zero, so ignore what a guest writes
+         * to them.
+         */
+        value &= APLIC_SOURCECFG_WMASK;
+
+        if ( value & APLIC_SOURCECFG_D )
+        {
+            gdprintk(XENLOG_ERR, "APLIC_SOURCECFG_D isn't supported\n");
+
+            goto fail;
+        }
+
+        /*
+         * SM is WARL, so the reserved values 0x2 and 0x3 need no handling
+         * here: vAPLIC keeps no shadow copy of sourcecfg[], the value is
+         * written straight to the h/w register and is read back from it, so
+         * it is the h/w which substitutes a legal value for an illegal one.
+         *
+         * TODO: when vAPLIC starts to shadow sourcecfg[], the WARL behaviour
+         * will have to be emulated here instead, e.g.:
+         *   if ( value == 0x2 || value == 0x3 )
+         *       value = APLIC_SOURCECFG_SM_INACTIVE;
+         */
+
+        /*
+         * 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;
+
+        break;
+
+    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_SETIPNUM:
+    case APLIC_CLRIPNUM:
+    case APLIC_SETIENUM:
+    case APLIC_CLRIENUM:
+    case APLIC_SETIPNUM_LE:
+        if ( !value || !AUTH_IRQ_BIT(currd, value) )
+            return true;
+
+        break;
+
+    case APLIC_TARGET_BASE ... APLIC_TARGET_LAST:
+    {
+        struct vaplic *vaplic = to_vaplic(currd);
+        /*
+         * Look at vaplic_emulate_load() for explanation why APLIC_GENMSI is
+         * subtracted.
+         */

xen/arch/riscv/vaplic.c +53 -0

+            unsigned int guest_hart_idx =
+                MASK_EXTR(value, APLIC_TARGET_HART_IDX);
+            unsigned long flags;
+            struct vimsic_state *vimsic_state;
+
+            target_vcpu = domain_vcpu(currd, guest_hart_idx);
+
+            if ( !target_vcpu )
+            {
+                gdprintk(XENLOG_ERR, "Invalid vCPU id in target register\n");
+
+                /* Ignore such writings */
+                return true;
+            }
+
+            vimsic_state = target_vcpu->arch.vimsic_state;
+
+            /*
+             * 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.
+             */
+            if ( MASK_EXTR(value, APLIC_TARGET_GUEST_IDX) )
+            {
+                printk_once(XENLOG_WARNING
+                            "%pd: vAPLIC target guest index != 0 is unsupported\n",
+                            currd);
+
+                return true;
+            }
+
+            /*
+             * src->lock keeps the stored copy and the h/w register updated
+             * as a pair.
+             */
+            spin_lock_irqsave(&src->lock, flags);
+
+            write_atomic(&src->target, value);
+
+            /* IRQs are already off under src->lock. */
+            read_lock(&vimsic_state->vsfile_lock);
+
+            /*
+             * The h/w register names the hart and the guest interrupt file
+             * MSIs are sent to, and there is none of either until the target
+             * vCPU is given a h/w guest interrupt file. The stored copy

xen/arch/riscv/vaplic.c +11 -0

+
+            read_unlock(&vimsic_state->vsfile_lock);
+
+            spin_unlock_irqrestore(&src->lock, flags);
+
+            return true;
+        }
+
+        /* printk_once(), as a guest could otherwise flood the log. */
+        printk_once(XENLOG_WARNING
+                    "%pd: vAPLIC direct delivery mode isn't supported\n",

xen/arch/riscv/vaplic.c +7 -0

+    }
+
+    aplic_hw_write_reg(offset, value);
+
+    return true;
+}
+
 static int __init cf_check vaplic_make_domu_dt_node(struct kernel_info *kinfo)
 {
     struct domain *d = kinfo->bd.d;

xen/arch/riscv/vaplic.c +13 -0

+    }
+
+    if ( !vaplic_emulate_load(v, info->gpa, &val) )
+        return IO_ABORT;
+
+    /* APLIC registers are 32-bit; zero-extend to the guest register width. */
+    info->data = val;
+
+    return IO_HANDLED;
+}
+
+static enum io_state cf_check vaplic_mmio_write(struct vcpu *v,
+                                                const mmio_info_t *info)

xen/arch/riscv/vaplic.c +20 -1

+    }
+
+    if ( !vaplic_emulate_store(v, info->gpa, info->data) )
+        return IO_ABORT;
+
+    return IO_HANDLED;
+}
+
+static const struct mmio_handler_ops vaplic_mmio_ops = {
+    .read  = vaplic_mmio_read,
+    .write = vaplic_mmio_write,
+};
+
 static const struct vintc_ops vintc_ops = {
     .vcpu_init = vcpu_imsic_init,
     .vcpu_deinit = vcpu_imsic_deinit,

@@ -103,6 +461,8 @@ static const struct vintc_ops vintc_ops = {
 int domain_vaplic_init(struct domain *d)
 {
     struct vaplic *vaplic = xvzalloc(struct vaplic);
+    unsigned int i;
+    int rc;
 
     if ( !vaplic )
         return -ENOMEM;

@@ -123,7 +483,28 @@ int domain_vaplic_init(struct domain *d)
      */
     d->arch.vintc->nr_virqs = guest_aplic_num_sources + 1;
 
-    return 0;
+    /* Slot 0 is unused: APLIC source numbering starts at 1 (see used_irqs). */
+    vaplic->regs.sources = xvzalloc_array(struct vaplic_source,
+                                          d->arch.vintc->nr_virqs);
+    if ( !vaplic->regs.sources )
+    {

Quoted 18 lines

Guests running under Xen program interrupt routing by writing to APLIC MMIO registers. Xen must intercept these accesses to enforce interrupt isolation between domains and to translate guest routing intent into the underlying physical MSI topology.

Register the vAPLIC range with the MMIO dispatch and emulate the accesses:
- reads and writes are masked with the domain's authorised interrupt bitmap, so that a guest can neither observe nor affect interrupts it does not own;
- target registers are shadowed per source, so that a guest reads back what it wrote. The h/w register instead gets the hart index and guest interrupt file of the pCPU the target vCPU's interrupt file lives on, as the APLIC uses these fields directly to compute the MSI address. As long as the target vCPU has no h/w guest interrupt file, only the shadow copy is updated;
- only MSI delivery mode is emulated: target writes made while the guest's domaincfg.DM is clear are ignored.

Quoted 4 lines

Delegation (APLIC_SOURCECFG_D) is not yet supported.

Co-developed-by Romain Caritey <[email protected]>

Signed-off-by Oleksii Kurochko <[email protected]>

Co-developed-by: needs a matching Signed-off-by: from Romain right after it.

Quoted 6 lines

xen/arch/riscv/aplic-priv.h +0 -0

@@ -47,4 +47,6 @@ struct aplic_priv {
   */
  extern unsigned int guest_aplic_num_sources;
 

xen/arch/riscv/aplic.c line 19

Quoted 12 lines

xen/arch/riscv/aplic-priv.h +2 -0

+uint32_t aplic_msi_target_gen(const struct vcpu *v, uint32_t base_val);
+
  #endif /* ASM_RISCV_APLIC_PRIV_H */

xen/arch/riscv/aplic.c +1 -0

index 17c96177a3..febc451760 100644
@@ -16,6 +16,7 @@
  #include <xen/irq.h>
  #include <xen/mm.h>
  #include <xen/sections.h>
+#include <xen/sched.h>

Nit: <xen/sched.h> sorts before <xen/sections.h>.

Quoted 15 lines

xen/arch/riscv/aplic.c +0 -2

  #include <xen/spinlock.h>
  #include <xen/types.h>
  #include <xen/vmap.h>

@@ -28,8 +29,6 @@
  #include <asm/io.h>
  #include <asm/riscv_encoding.h>
 
-#define APLIC_DEFAULT_PRIORITY  1
-
  static struct aplic_priv aplic = {
      .lock = SPIN_LOCK_UNLOCKED,
  };

@@ -38,6 +37,137 @@ static struct intc_info __ro_after_init aplic_info = {
      .hw_variant = INTC_APLIC,

xen/arch/riscv/aplic.c static struct intc_info __ro_after_init aplic_info = { · line 47

Quoted 10 lines

xen/arch/riscv/aplic.c +8 -0

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

Sorry but as I already mentioned before, Hart Index is equal to (g << lhxw) | h I think it is a miss as you explained this later correctly ....

I would propose this:

     ```
     * The physical address of an outgoing MSI is calculated by bitwise ORing a
     * Base Physical Page Number (Base PPN) with the group number (g), the
     * hart number relative to the group (h) and, for a supervisor-level
     * interrupt domain, the Guest Index:
     ```

Quoted 5 lines

xen/arch/riscv/aplic.c +4 -0

+ *
+ *   ( Base PPN | (g << (HHXS + 12)) | (h << LHXS) | Guest Index ) << 12
+ *
+ * where Base PPN, HHXS, LHXS, HHXW and LHXW come from the {m,s}msiaddrcfg[h]

xen/arch/riscv/aplic.c static struct intc_info __ro_after_init aplic_info = { · line 61

Quoted 10 lines

xen/arch/riscv/aplic.c +10 -0

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

... here

Quoted 56 lines

xen/arch/riscv/aplic.c +56 -0

+ * - 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
+ * 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.
+ *
+ * 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_index(unsigned int cpu)
+{
+    const struct imsic_config *imsic = imsic_get_config();
+    const struct imsic_msi *msi = &imsic->msi[cpu];
+    /* Low Hart Index Shift */
+    unsigned int lhxs = imsic->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
+     * 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)) &

xen/arch/riscv/aplic.c static struct intc_info __ro_after_init aplic_info = { · line 151

Quoted 10 lines

xen/arch/riscv/aplic.c +10 -0

+        APLIC_xMSICFGADDR_PPN_LHX_MASK(lhxw);
+
+    return (g << lhxw) | h;
+}
+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)));

Nit: the parentheses around offset < aplic.size aren't needed:

         ASSERT(offset < aplic.size && IS_ALIGNED(offset, sizeof(uint32_t)));

xen/arch/riscv/aplic.c static struct intc_info __ro_after_init aplic_info = { · line 164

Quoted 13 lines

xen/arch/riscv/aplic.c +13 -0

+
+    spin_lock_irqsave(&aplic.lock, flags);
+    val = readl((const 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)));

Same here.

Quoted 14 lines

xen/arch/riscv/aplic.c +14 -0

+/*
+ * Convert a byte offset (within a SETIP/CLRIP/SETIE/CLRIE register group) to
+ * a 32-bit word index into the used_irqs bitmap. Each word covers 32
+ * interrupt sources. For SOURCECFG and TARGET groups the same division also
+ * yields the interrupt number directly, because those arrays store one 32-bit
+ * register per source.
+ */
+#define regoffset_to_word_idx(reg_val) ((reg_val) / sizeof(uint32_t))
+
+static uint32_t vaplic_target_read(const struct domain *d, unsigned int irqn)
+{
+    const struct vaplic *vaplic = to_vaplic(d);
+
+    /* target[0] doesn't exist so irqn == 0 should be impossible */

xen/arch/riscv/aplic.c static struct intc_info __ro_after_init aplic_info = { · line 161

Quoted 7 lines

xen/arch/riscv/aplic.c +7 -0

+    if ( !irqn || irqn >= vaplic->vintc.nr_virqs )
+        return 0;
+
+    return read_atomic(&vaplic->regs.sources[irqn].target);
+}
+
+static inline uint32_t generate_auth_mask(const struct domain *currd,

Nit: inline in a .c file isn't needed, the compiler decides anyway.

Quoted 19 lines

xen/arch/riscv/aplic.c +19 -0

+                                          unsigned int word_idx)
+{
+    unsigned int first_bit = word_idx * sizeof(uint32_t) * BITS_PER_BYTE;
+
+    if ( word_idx >= DIV_ROUND_UP(currd->arch.vintc->nr_virqs,
+                                  sizeof(uint32_t) * BITS_PER_BYTE) )
+    {
+        gdprintk(XENLOG_DEBUG, "incorrect word_idx(%u)\n", word_idx);
+
+        return 0;
+    }
+
+    return currd->arch.vintc->used_irqs[first_bit / BITS_PER_LONG] >>
+           (first_bit % BITS_PER_LONG);
+}
+
+static bool vaplic_emulate_load(const struct vcpu *curr, paddr_t addr,
+                                uint32_t *out)
+{

xen/arch/riscv/aplic.c

Quoted 3 lines

xen/arch/riscv/aplic.c +3 -0

+    const struct domain *currd = curr->domain;
+    const struct vaplic *vaplic = to_vaplic(currd);
+    const unsigned int offset = addr & APLIC_CTRL_REGION_OFFSET_MASK;

With that, every access with addr >= APLIC_CTRL_REGION_OFFSET_MASK aliases onto the same control region: e.g. an access to GUEST_APLIC_S_BASE + 0x4000 (idelivery of IDC 0) ends up in the APLIC_DOMAINCFG case, and a store there rewrites domaincfg.

It also silently relies on GUEST_APLIC_S_BASE being 16 KiB aligned.

The AIA spec ("Memory-mapped control region for an interrupt domain") says IDC structures start at 0x4000 and are only needed for direct delivery mode, and that all other bytes of the control region are reserved and read-only zero, writes to them being ignored. So I'd expect something like:

     const paddr_t offset = addr - vaplic->regs_start;

Then read/write out of control region would go to "default:" case.

and APLIC_CTRL_REGION_OFFSET_MASK could go away.

Quoted 41 lines

xen/arch/riscv/aplic.c +40 -0

+    uint32_t auth_mask;
+    unsigned int i;
+
+    ASSERT(curr == current);
+
+    switch ( offset )
+    {
+    case APLIC_DOMAINCFG:
+        *out = vaplic->regs.domaincfg;
+
+        return true;
+
+    case APLIC_SETIP_BASE ... APLIC_SETIP_LAST:
+    case APLIC_CLRIP_BASE ... APLIC_CLRIP_LAST:
+    case APLIC_SETIE_BASE ... APLIC_SETIE_LAST:
+        i = regoffset_to_word_idx(offset & APLIC_SETCLR_OFFSET_MASK);
+        auth_mask = generate_auth_mask(currd, i);
+
+        break;
+
+    case APLIC_SETIPNUM:
+    case APLIC_CLRIPNUM:
+    case APLIC_SETIENUM:
+    case APLIC_CLRIE_BASE ... APLIC_CLRIE_LAST:
+    case APLIC_CLRIENUM:
+    case APLIC_SETIPNUM_LE:
+        /*
+         * Based on the RISC-V AIA spec a read of these registers
+         * always returns zero
+         */
+        *out = 0;
+
+        return true;
+
+    case APLIC_TARGET_BASE ... APLIC_TARGET_LAST:
+        /*
+         * As target registers start from 1:
+         *  0x3000 genmsi
+         *  0x3004 target[1]
+         *  0x3008 target[2]

xen/arch/riscv/aplic.c

Quoted 10 lines

xen/arch/riscv/aplic.c +10 -0

+         *   ...
+         *  0x3FFC target[1023]
+         * It is necessary to calculate an interrupt number by subtracting
+         * APLIC_GENMSI instead of APLIC_TARGET_BASE.
+         */
+        i = regoffset_to_word_idx(offset - APLIC_GENMSI);
+
+        *out = AUTH_IRQ_BIT(currd, i) ? vaplic_target_read(currd, i) : 0;
+
+        return true;

genmsi (0x3000) isn't covered by any case, so an access to it ends up in default: and faults while in MSI delivery mode it isn't read-only zero:

   Hart Index and EIID behave as in a target register, and Busy is
   read-only. I would suggest to add a dedicated switch case:
   
     ```
     case APLIC_GENMSI:
         *out = vaplic_is_msi_mode(currd) ? vaplic->regs.genmsi : 0;

         return true;
     ```

With a new helper vaplic_is_msi_mode -> bool that could also be used in write counterpart and a new genmsi field in vaplic->regs.

If we assume the guest will never try to read genmsi then it is okay but it might worth to at least mention it somewhere.

Quoted 12 lines

xen/arch/riscv/aplic.c +12 -0

+    default:
+        gdprintk(XENLOG_WARNING, "Unhandled APLIC read at offset %#x\n",
+                 offset);
+
+        return false;
+    }
+
+    *out = aplic_hw_read_reg(offset) & auth_mask;
+
+    return true;
+}
+

xen/arch/riscv/aplic.c

Quoted 5 lines

xen/arch/riscv/aplic.c +5 -0

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

Same aliasing issue as in vaplic_emulate_load(), here with the write going to the aliased register.

xen/arch/riscv/aplic.c

Quoted 12 lines

xen/arch/riscv/aplic.c +11 -0

+
+    ASSERT(curr == current);
+
+    switch ( offset )
+    {
+    case APLIC_DOMAINCFG:
+    {
+        struct vaplic *vaplic = to_vaplic(currd);
+
+        vaplic->regs.domaincfg = APLIC_DOMAINCFG_RO |
+                                 (value & APLIC_DOMAINCFG_WMASK);

APLIC_DOMAINCFG_WMASK includes DM, so a guest writing DM=0 reads 0 back and assumes direct delivery mode is in effect. However, only MSI delivery is emulated: there are no IDC structures, and with DM=0 the target handling below drops the write, so the guest just stops getting interrupts.

DM is WARL, so an MSI-only APLIC is expected to keep it at 1 whatever is written, which is how software figures out that direct mode isn't available. Can't we do the same here, as is already done for BE? I.e. drop DM from APLIC_DOMAINCFG_WMASK, and OR APLIC_DOMAINCFG_DM in here and in domain_vaplic_init(). That would also justify sizing the region at APLIC_MIN_SIZE (see my comment on regs_size at the end).

Quoted 81 lines

xen/arch/riscv/aplic.c +81 -0

+
+        return true;
+    }
+
+    case APLIC_SOURCECFG_BASE ... APLIC_SOURCECFG_LAST:
+        /*
+         * Only D (bit 10) and SM (bits 2:0) are implemented, the rest of the
+         * bits are reserved and read as zero, so ignore what a guest writes
+         * to them.
+         */
+        value &= APLIC_SOURCECFG_WMASK;
+
+        if ( value & APLIC_SOURCECFG_D )
+        {
+            gdprintk(XENLOG_ERR, "APLIC_SOURCECFG_D isn't supported\n");
+
+            goto fail;
+        }
+
+        /*
+         * SM is WARL, so the reserved values 0x2 and 0x3 need no handling
+         * here: vAPLIC keeps no shadow copy of sourcecfg[], the value is
+         * written straight to the h/w register and is read back from it, so
+         * it is the h/w which substitutes a legal value for an illegal one.
+         *
+         * TODO: when vAPLIC starts to shadow sourcecfg[], the WARL behaviour
+         * will have to be emulated here instead, e.g.:
+         *   if ( value == 0x2 || value == 0x3 )
+         *       value = APLIC_SOURCECFG_SM_INACTIVE;
+         */
+
+        /*
+         * 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;
+
+        break;
+
+    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_SETIPNUM:
+    case APLIC_CLRIPNUM:
+    case APLIC_SETIENUM:
+    case APLIC_CLRIENUM:
+    case APLIC_SETIPNUM_LE:
+        if ( !value || !AUTH_IRQ_BIT(currd, value) )
+            return true;
+
+        break;
+
+    case APLIC_TARGET_BASE ... APLIC_TARGET_LAST:
+    {
+        struct vaplic *vaplic = to_vaplic(currd);
+        /*
+         * 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) )

xen/arch/riscv/aplic.c

Quoted 7 lines

xen/arch/riscv/aplic.c +7 -0

+            /* Interrupt not enabled, ignore it */
+            return true;
+
+        if ( vaplic->regs.domaincfg & APLIC_DOMAINCFG_DM )
+        {
+            struct vaplic_source *src = &vaplic->regs.sources[srcn];
+            struct vcpu *target_vcpu;

target_vcpu is only read, so this can be const struct vcpu *.

Quoted 63 lines

xen/arch/riscv/aplic.c +62 -0

+            unsigned int guest_hart_idx =
+                MASK_EXTR(value, APLIC_TARGET_HART_IDX);
+            unsigned long flags;
+            struct vimsic_state *vimsic_state;
+
+            target_vcpu = domain_vcpu(currd, guest_hart_idx);
+
+            if ( !target_vcpu )
+            {
+                gdprintk(XENLOG_ERR, "Invalid vCPU id in target register\n");
+
+                /* Ignore such writings */
+                return true;
+            }
+
+            vimsic_state = target_vcpu->arch.vimsic_state;
+
+            /*
+             * 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.
+             */
+            if ( MASK_EXTR(value, APLIC_TARGET_GUEST_IDX) )
+            {
+                printk_once(XENLOG_WARNING
+                            "%pd: vAPLIC target guest index != 0 is unsupported\n",
+                            currd);
+
+                return true;
+            }
+
+            /*
+             * src->lock keeps the stored copy and the h/w register updated
+             * as a pair.
+             */
+            spin_lock_irqsave(&src->lock, flags);
+
+            write_atomic(&src->target, value);
+
+            /* IRQs are already off under src->lock. */
+            read_lock(&vimsic_state->vsfile_lock);
+
+            /*
+             * The h/w register names the hart and the guest interrupt file
+             * MSIs are sent to, and there is none of either until the target
+             * vCPU is given a h/w guest interrupt file. The stored copy
+             * reaches the h/w at that point.
+             *
+             * TODO: once s/w guest interrupt files are supported, a vCPU using
+             * one still needs its MSIs, so the h/w register will have to be
+             * written for it too, pointing at an interrupt file Xen receives
+             * them in on the vCPU's behalf.
+             */
+            if ( vimsic_state->vsfile_cpu != CPU_NONE )
+                aplic_hw_write_reg(offset,

Quoted 1 line

+ aplic_msi_target_gen(target_vcpu, value));

The comment above says the stored copy reaches the h/w once the vCPU gets a h/w guest interrupt file, but nothing in this patch does that: it only happens with vaplic_reconfigure_target(), added by "xen/riscv: introduce vaplic_reconfigure_target()" and called from imsic_vsfile_move() by "xen/riscv: remap interrupts to new IMSIC VS-file". At this commit a target written before the first attach never reaches the h/w.

Quoted 15 lines

xen/arch/riscv/aplic.c +14 -0

+
+            read_unlock(&vimsic_state->vsfile_lock);
+
+            spin_unlock_irqrestore(&src->lock, flags);
+
+            return true;
+        }
+
+        /* printk_once(), as a guest could otherwise flood the log. */
+        printk_once(XENLOG_WARNING
+                    "%pd: vAPLIC direct delivery mode isn't supported\n",
+                    currd);
+
+        return true;

xen/arch/riscv/aplic.c

Quoted 7 lines

xen/arch/riscv/aplic.c +7 -0

+    }
+
+    default:
+    fail:
+        gdprintk(XENLOG_WARNING,
+                 "Unhandled APLIC write at offset %#x (value %#x)\n", offset,
+                 value);

Nit: now that the arguments wrap, put offset on its own line as well.

xen/arch/riscv/aplic.c

Quoted 3 lines

xen/arch/riscv/aplic.c +2 -0

+
+        return false;

So in this function you are assuming the guest will never try to write genmsi. Is it expected? so a guest accessing it faults. Linux doesn't use it today, but the spec defines it in MSI delivery mode. Could it at least get an explicit case (reads as zero, writes ignored, with a TODO about extempore MSI injection) rather than relying on guests never touching it?

Quoted 15 lines

xen/arch/riscv/aplic.c +7 -0

+    }
+
+    aplic_hw_write_reg(offset, value);
+
+    return true;
+}
+
  static int __init cf_check vaplic_make_domu_dt_node(struct kernel_info *kinfo)
  {
      struct domain *d = kinfo->bd.d;

@@ -95,6 +403,56 @@ static const struct vintc_init_ops __initconstrel init_ops = {
      .make_domu_dt_node = vaplic_make_domu_dt_node,
  };
 

xen/arch/riscv/aplic.c static const struct vintc_init_ops __initconstrel init_ops = { · line 407

Quoted 2 lines

xen/arch/riscv/aplic.c +2 -0

+static enum io_state cf_check vaplic_mmio_read(struct vcpu *v,
+                                               mmio_info_t *info)

Jan asked in v1 to rename the parameter to curr once ASSERT(v == current) is added. Same for vaplic_mmio_write().

xen/arch/riscv/aplic.c static const struct vintc_init_ops __initconstrel init_ops = { · line 419

Quoted 13 lines

xen/arch/riscv/aplic.c +12 -0

+{
+    uint32_t val;
+
+    ASSERT(v == current);
+
+    if ( info->len != sizeof(uint32_t) ||
+         !IS_ALIGNED(info->gpa, sizeof(uint32_t)) )
+    {
+        gdprintk(XENLOG_DEBUG,
+                 "VAPLIC: unaligned/wrong-width read gpa=%"PRIpaddr" len=%u\n",
+                 info->gpa, info->len);
+        return IO_ABORT;

Please drop the "VAPLIC:" prefix, gdprintk() already says where the message comes from. Also a blank line before the return, as you do elsewhere in the file.

Quoted 14 lines

xen/arch/riscv/aplic.c +13 -0

+    }
+
+    if ( !vaplic_emulate_load(v, info->gpa, &val) )
+        return IO_ABORT;
+
+    /* APLIC registers are 32-bit; zero-extend to the guest register width. */
+    info->data = val;
+
+    return IO_HANDLED;
+}
+
+static enum io_state cf_check vaplic_mmio_write(struct vcpu *v,
+                                                const mmio_info_t *info)

xen/arch/riscv/aplic.c static const struct vintc_init_ops __initconstrel init_ops = { · line 442

Quoted 10 lines

xen/arch/riscv/aplic.c +10 -0

+{
+    ASSERT(v == current);
+
+    if ( info->len != sizeof(uint32_t) ||
+         !IS_ALIGNED(info->gpa, sizeof(uint32_t)) )
+    {
+        gdprintk(XENLOG_DEBUG,
+                 "VAPLIC: unaligned/wrong-width write gpa=%"PRIpaddr" len=%u\n",
+                 info->gpa, info->len);
+        return IO_ABORT;

Same here.

Quoted 42 lines

xen/arch/riscv/aplic.c +27 -1

+    }
+
+    if ( !vaplic_emulate_store(v, info->gpa, info->data) )
+        return IO_ABORT;
+
+    return IO_HANDLED;
+}
+
+static const struct mmio_handler_ops vaplic_mmio_ops = {
+    .read  = vaplic_mmio_read,
+    .write = vaplic_mmio_write,
+};
+
  static const struct vintc_ops vintc_ops = {
      .vcpu_init = vcpu_imsic_init,
      .vcpu_deinit = vcpu_imsic_deinit,

@@ -103,6 +461,8 @@ static const struct vintc_ops vintc_ops = {
  int domain_vaplic_init(struct domain *d)
  {
      struct vaplic *vaplic = xvzalloc(struct vaplic);
+    unsigned int i;
+    int rc;
 
      if ( !vaplic )
          return -ENOMEM;

@@ -123,7 +483,28 @@ int domain_vaplic_init(struct domain *d)
       */
      d->arch.vintc->nr_virqs = guest_aplic_num_sources + 1;
 
-    return 0;
+    /* Slot 0 is unused: APLIC source numbering starts at 1 (see used_irqs). */
+    vaplic->regs.sources = xvzalloc_array(struct vaplic_source,
+                                          d->arch.vintc->nr_virqs);
+    if ( !vaplic->regs.sources )
+    {
+        domain_vaplic_deinit(d);
+
+        return -ENOMEM;
+    }
+
+    for ( i = 0; i < d->arch.vintc->nr_virqs; i++ )
+        spin_lock_init(&vaplic->regs.sources[i].lock);

xen/arch/riscv/aplic.c int domain_vaplic_init(struct domain *d) · line 500

Quoted 3 lines

xen/arch/riscv/aplic.c +3 -0

+
+    vaplic->regs_start = GUEST_APLIC_S_BASE;
+    vaplic->regs_size = APLIC_SIZE(d->max_vcpus);

The vAPLIC only supports MSI delivery (msi-parent → vIMSIC, no direct mode emulated), so no IDC structures are needed and APLIC_MIN_SIZE is enough. To make that explicit I'll also hardwire domaincfg.DM to 1 (WARL), so the guest can't select a mode the region doesn't back.

Quoted 18 lines · Baptiste Le Duc

Quoted 18 lines

Guests running under Xen program interrupt routing by writing to APLIC MMIO registers. Xen must intercept these accesses to enforce interrupt isolation between domains and to translate guest routing intent into the underlying physical MSI topology.

Register the vAPLIC range with the MMIO dispatch and emulate the accesses:
- reads and writes are masked with the domain's authorised interrupt bitmap, so that a guest can neither observe nor affect interrupts it does not own;
- target registers are shadowed per source, so that a guest reads back what it wrote. The h/w register instead gets the hart index and guest interrupt file of the pCPU the target vCPU's interrupt file lives on, as the APLIC uses these fields directly to compute the MSI address. As long as the target vCPU has no h/w guest interrupt file, only the shadow copy is updated;
- only MSI delivery mode is emulated: target writes made while the guest's domaincfg.DM is clear are ignored.

Quoted 6 lines

Quoted 4 lines

Delegation (APLIC_SOURCECFG_D) is not yet supported.

Co-developed-by Romain Caritey <[email protected]>

Signed-off-by Oleksii Kurochko <[email protected]>

Co-developed-by: needs a matching Signed-off-by: from Romain right after it.

I will contact Romain and ask for his Signed-off-by.