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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 8/6/26 4:48 PM, Jan Beulich wrote:
> 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.
The word “pin” was incorrect to use here. What I meant is that a vCPU is 
assigned to a pCPU by scheduler and of course it could be re-scheduled 
by a scheduler to another pCPU (maybe for NULL scheduler such 
re-scheduling don't happen...), and after this assignment happens, the 
IMSIC interrupt file mapping needs to be recalculated.
> 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?
Yes, the GFN isn’t changed for a vCPU. The plan was for 
map_regions_p2mt() to simply replace the corresponding PTE for the GFN, 
which is why imsic_unmap_guest_file() isn’t really needed now.
I will re-phrase this paragraph to:
 * A vCPU runs on the pCPU the scheduler picked for it (v->processor), and
 * the guest file it is given (guest_file_id, from the vGEIN allocator)
 * belongs to that very pCPU's IMSIC. 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.
 *
 * Consequently the mapping installed here is only valid as long as the 
vCPU
 * stays on that pCPU. When it migrates, a VS-file is acquired on the new
 * pCPU and mapped at the very same GFN, so the stale mapping needs no
 * explicit tear-down: it is simply replaced.

> > + * 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);
I just noticed that imsic_cfg.base_addr isn't really good to use here. 
It should be GUEST_IMSIC_S_BASE instead.
> > +    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.
I will apply your suggestion.

> > +#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).
I’m wondering if it wouldn’t be better to use paddr_t for gaddr as well, 
since technically it is a guest *physical address*. In that case, 
PRIpaddr could be used to print both paddr and gaddr variables.
Also, could this be the reason why PRIgaddr doesn’t exist? Basically, a 
GPA could be considered a physical address, while for a GVA there is 
already PRIvaddr.
Thanks!

Best regards,
 Oleksii