Re: [PATCH v1 13/27] xen/riscv: add basic VGEIN management for AIA guests

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.04.2026 13:34, Oleksii Kurochko wrote:
> On 4/16/26 2:21 PM, Jan Beulich wrote:
>> On 13.04.2026 16:42, Oleksii Kurochko wrote:
>>> On 4/2/26 12:03 PM, Jan Beulich wrote:
>>>> On 10.03.2026 18:08, Oleksii Kurochko wrote:
>>>>> @@ -14,12 +27,109 @@ bool aia_available(void)
>>>>>        return is_aia_available;
>>>>>    }
>>>>>    
>>>>> +int __init vgein_init(unsigned int cpu)
>>>>
>>>> If this needs invoking once per CPU being brought up, it can't be __init.
>>>
>>> Yes, it is going to be called inside the secondary CPU bring-up function.
>>>
>>> __init sections are removed much later, after all CPUs are brought up,
>>> so it looks like that at the moment when secondary CPUs are being
>>> brought up, __init still exists and can be called.
>>
>> No. And I said so before. See XEN_SYSCTL_cpu_hotplug (and ignore it having
>> "hotplug" in the name, but merely consider that you can take CPUs offline
>> at runtime, and later bring them online again).
> 
> Thanks, now it makes sense to drop __init.
> 
>>>> Also - static?
>>>
>>> It isn't static because it will be called inside the secondary CPU
>>> bring-up function.
>>
>> As it doesn't need calling from the outside for the boot CPU, it's not
>> obvious why it would need calling from the outside for secondary ones.
> 
> I think I am confused here by what do you mean by "calling from the 
> outside".

As in "from another translation unit".

> It should be called during boot or secondary CPU initialization (if AIA 
> is used), right?
> Then considering that VGEIN-related stuff is connected to AIA then 
> vgein_init() is expected to live in aia.c and considering that it wants 
> to be called by secondary CPU boot code it can't be static.

Putting my question differently: Why is there a difference here between
boot CPU and secondary CPUs. My (general) expectation would be that either
the function wants calling from outside of aia.c in all cases, or it could
be static (now and later).

>>>>> +{
>>>>> +    struct vgein_bmp *vgein = &per_cpu(vgein_bmp, cpu);
>>>>> +
>>>>> +    csr_write(CSR_HGEIE, -1UL);
>>>>> +    vgein->geilen = flsl(csr_read(CSR_HGEIE));
>>>>> +    csr_write(CSR_HGEIE, 0);
>>>>> +    if ( vgein->geilen )
>>>>> +        vgein->geilen--;
>>>>
>>>> I don't understand this. The "len" in "geilen" stands for "length", I 
>>>> suppose,
>>>> i.e. the number of bits. Hmm, the spec itself is inconsistent: "The number 
>>>> of
>>>> bits implemented in hgeip and hgeie for guest external interrupts is 
>>>> UNSPECIFIED
>>>> and may be zero. This number is known as GEILEN." This may or may not 
>>>> include
>>>> bit 0 (which is implemented, but r/o zero). Then saying "Hence, if GEILEN 
>>>> is
>>>> nonzero, bits GEILEN:1 shall be writable in ..." suggests 0 isn't 
>>>> included, but
>>>> that's not unambiguous.
>>>
>>> But they explicitly wrote that: The least-significant bits are
>>> implemented first, apart from bit 0. So bit 0 is explicitly excluded.
>>
>> Fine, but not including it in anything named *LEN feels - as said - 
>> ambiguous.
> 
> Agree, it is. I just tried to follow the spec naming here. But I am okay 
> to rename it to max_gein, for example.

Perhaps better stick to spec naming, but consider adding a clarifying comment.

>>>>> +    /*
>>>>> +     * All vCPU guest interrupt files are used and we don't support a 
>>>>> case
>>>>> +     * when number of vCPU on 1 pCPU is bigger then geilen.
>>>>> +     */
>>>>
>>>> This wants checking in vgein_init() then. CPUs (beyond the boot one)
>>>> violating this should not be brought online.
>>>
>>> It'll be nice. But we can't know how many vCPUs will be ran on pCPU when
>>> vgein_init() is executed.
>>
>> I don't understand: How does it matter how many vCPU-s will exist later on
>> (and will run wherever)?
> 
> IIUC, you want to check what is mentioned in the comment in vcpu_init(). 
> The comment says that it is checking that number of vCPU on one pCPU 
> isn't bigger then geilen. To check that we have to know an amount of 
> vCPU potentially will be ran on pCPU.

Well, not quite. I don't understand at all why the number of vCPU-s in
a system would be limited by any pCPU property. I don't think we have
any such constraint on x86, for example.

Jan