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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 8/13/26 11:06 AM, Baptiste Le Duc 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>
>
> > diff --git a/xen/arch/riscv/imsic.c b/xen/arch/riscv/imsic.c
> > index ffce77209c..c5ae74e456 100644
> > --- a/xen/arch/riscv/imsic.c
> > +++ b/xen/arch/riscv/imsic.c
> > @@ -25,7 +25,9 @@
> >   #include <xen/spinlock.h>
> >   #include <xen/xvmalloc.h>
> >   
> > +#include <asm/aia.h>
> >   #include <asm/imsic.h>
> > +#include <asm/p2m.h>
> >   
> >   #define IMSIC_HART_SIZE(guest_bits) (BIT(guest_bits, U) * IMSIC_MMIO_PAGE_SZ)
> >   
> > @@ -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
> 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 
IMSIC interrupt file we don't need any stage-2 mapping. So here it is 
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",
> Maybe a use of dprintk() would be more appropriate?
Agree, dprintk() will be better.

Thanks.

~ Oleksii