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

On 2026-08-13 11:42 +0200, Oleksii Kurochko wrote:
> 
> 
> 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 <[email protected]>
> > 
> > 
> >>
> >> 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 
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.
> 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
> 
> 
>