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

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 
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