Re: [PATCH v1 06/17] xen/riscv: map IMSIC interrupt file for vCPUs

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.07.2026 18:02, Oleksii Kurochko wrote:
> A guest running in VS-mode expects its own IMSIC S-file at offset 0 of its
> guest-physical IMSIC block. Physically, the guest-file (G-file) assigned to
> this vCPU lives at a hart-relative offset given by guest_file_id (assigned
> via the vGEIN allocator). Therefore, imsic_map_guest_file() uses stage-2
> translation to redirect the guest's fixed per-vCPU GPA page (offset 0) to
> the specific physical guest-file page.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> The corresponding unmap of the IMSIC interrupt file will be introduced
> separately when the need arises.

Doesn't the need exist right away? There is ...

> @@ -342,6 +344,67 @@ static int __init imsic_parse_node(const struct 
> dt_device_node *node,
>      return 0;
>  }
>  
> +/*
> + * Map the physical IMSIC guest interrupt file (G-file) assigned to vCPU v
> + * into the domain's stage-2 guest-physical address space.
> + *
> + * In the machine's physical address space (SPA), each hart's IMSIC
> + * supervisor-level file (S-file) is located at offset 0 of its address 
> block,
> + * followed contiguously by GEILEN guest files at offsets of 1, 2, ..., N 
> pages.
> + *
> + * Because a guest OS running in VS-mode expects its own supervisor-level
> + * interrupt file to be at offset 0 of its guest-physical IMSIC block, the
> + * hypervisor must use stage-2 address translation to map the vCPU's
> + * guest-physical "supervisor" page (GPA offset 0) to the specific
> + * physical guest file page (SPA offset guest_file_id) on the physical hart.
> + *
> + * Xen pins each vCPU to a pCPU (v->processor) and assigns it a physical

... an apparently wrong assumption here: Xen doesn't normally pin vCPU-s.
When a vCPU migrates between pCPU-s, clearly the mapping referencing the
page associated with the old hart needs tearing down again.

That said, since the new mapping will appear at the same GFN, the original
mapping may simply end up being replaced. If such direct replacement is
legitimate to do, maybe this could actually be mentioned here?

> + * guest file index (guest_file_id) from the vGEIN allocator. A guest_file_id
> + * of 0 indicates that no hardware guest file is selected (matching the
> + * architectural behavior where vGEIN = 0 in the hstatus CSR selects no
> + * guest external interrupt source), requiring the VS-file to be emulated
> + * in software.
> + *
> + * The base guest-physical address advertised to the guest in the device
> + * tree matches offset 0 of the vCPU's virtual IMSIC block. Stage-2
> + * translation ensures that guest supervisor accesses to this page are
> + * transparently routed to the real hardware VS-file granted to it on
> + * the current pCPU.
> + */
> +int imsic_map_guest_file(struct vcpu *v, unsigned int vsfile_id)
> +{
> +    int res = 0;
> +    struct domain *d = v->domain;
> +    unsigned int cpu = v->processor;
> +    vaddr_t gaddr = imsic_cfg.base_addr + (IMSIC_MMIO_PAGE_SZ * v->vcpu_id);
> +    paddr_t paddr;
> +    unsigned long guest_stride;
> +
> +    /* Nothing to map in the case of sw interrupt file. */
> +    if ( !vsfile_id )
> +        return res;
> +
> +    guest_stride = vsfile_id * IMSIC_MMIO_PAGE_SZ;

To me "stride" feels the wrong term here, as there's nothing that repeats.
"offset" likely would be better, assuming the use of this local variable is
really deemed worth it, as it's used ...

> +    paddr = imsic_cfg.msi[cpu].base_addr + imsic_cfg.msi[cpu].offset +
> +            guest_stride;

... only here.

> +#ifdef IMSIC_DEBUG
> +    printk("%s: %pv: ga(%#lx) -> pa(%#lx), cpu(%#x), guest_file_id(%d) "
> +           "base_addr(%#lx) offset(%#lx)\n", __func__, v, gaddr, paddr, cpu,
> +           vsfile_id, imsic_cfg.msi[cpu].base_addr, 
> imsic_cfg.msi[cpu].offset);
> +#endif
> +
> +    res = map_regions_p2mt(d, gaddr_to_gfn(gaddr),
> +                           PFN_DOWN(IMSIC_MMIO_PAGE_SZ), maddr_to_mfn(paddr),
> +                           arch_dt_passthrough_p2m_type());
> +    if ( res )
> +        printk("%s: Failed to map %#lx to the guest at %#lx\n",
> +               __func__, paddr, gaddr);

I think you mean to use PRIpaddr with paddr_t (oddly enough there's no
PRIgaddr).

Jan