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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 8/13/26 11:49 AM, Baptiste Le Duc wrote:
> On 2026-08-13 11:42 +0200, Oleksii Kurochko wrote:
> > > > +/*
> > > > + * Map the physical IMSIC guest interrupt file (G-file) assigned to vCPU v
> > > Nit: What is this `v`?
> > A function argument. But I will just drop v from the comment.
> >
> > > > + * 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
> > >
> > > > + * 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);
> > > The variable holds a guest-physical address, so vaddr_t is the wrong
> > > type should be either paddr_t or gaddr_t.
> > Agree, paddr_t will be better what was mentioned in thread with Jan B.
> >
> > > > +    paddr_t paddr;
> > > > +    unsigned long guest_stride;
> > > > +
> > > > +    /* Nothing to map in the case of sw interrupt file. */
> > > There is no software interrupt file implementation in this series, patch 11
> > > turns the non-MSI path into a BUG_ON(). So "vsfile_id == 0" today means "this
> > > vCPU gets no external interrupts at all and nothing tells anybody". Worth
> > > saying so plainly here rather than implying a fallback exists.
> > I would ask then different question will this function change when IMSIC
> > interrupt file support will be added? I think - no as in the case of
> Did you forget s/w word? If not it's weird as IMSIC interrupt file is
> the current topic of this patch series.
> > IMSIC interrupt file we don't need any stage-2 mapping. So here it is
> here too.
Yes, sorry, I missed s/w word.

> > just a check that nothing should be mapped for non-hw-assisted interrupt
> > files.
> >
> > > > +    if ( !vsfile_id )
> > > > +        return res;
> > > > +
> > > > +    guest_stride = vsfile_id * IMSIC_MMIO_PAGE_SZ;
> > >
> > > > +
> > > > +    paddr = imsic_cfg.msi[cpu].base_addr + imsic_cfg.msi[cpu].offset +
> > > > +            guest_stride;
> > >
> > > > +
> > > > +#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",
~ Oleksii