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

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 <[email protected]>
>> ---
>> 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