Re: [PATCH v1 05/17] xen/riscv: implement virtual APLIC MMIO emulation

On 8/12/26 11:47 AM, Baptiste Le Duc wrote:
> On 2026-08-11 18:24 +0200, Oleksii Kurochko wrote:
>>
>>
>> On 8/11/26 5:29 PM, Baptiste Le Duc wrote:
>>> On 2026-08-11 16:36 +0200, Oleksii Kurochko wrote:
>>>>
>>>>
>>>> On 8/11/26 11:21 AM, Baptiste Le Duc wrote:
>>>>> On 2026-08-07 18:08:21+02:00, Oleksii Kurochko wrote:
>>>>>> On 8/6/26 4:28 PM, Jan Beulich wrote:
>>>>>>
>>>>>>> On 20.07.2026 18:02, Oleksii Kurochko wrote:
>>>>>>>
>>>>>>> For this tag to have any meaning, it should move ahead of the --- above;
>>>>>>> the explanations ...
>>>>>>>
>>>>>>>
>>>>>>> ... here rather explain the restriction on the R-b, not its odd placement.
>>>>>>>
>>>>>>>
>>>>>>> As this looks to be recurring - please get versioning of your series right.
>>>>>>> The series is supposedly v1, but here you give the impression of it being
>>>>>>> v3. If there really was an earlier v2 posting, why isn't the entire series
>>>>>>> here v3?
>>>>>>
>>>>>> It is v3 before before it was a part of another patch series connected
>>>>>> to dom0less config enablement.
>>>>>>
>>>>>> Would it be better to just write in "Change in v3" that it is moved from
>>>>>> another patch series + link to that patch series? Or it will be enough
>>>>>> just to drop "Changes in v2 and v1" and just start from v1?
>>>>>>
>>>>>>> PLease can you, before submitting, self-review your patches? I'm really
>>>>>>> getting tired of having to repeatedly point out basic style issues, like
>>>>>>> the overlong line here.
>>>>>>
>>>>>> Sorry for that, I will write an extra checker for such cases to not miss
>>>>>> them.
>>>>>>
>>>>>>> It extends to the other local variables here, but I'll use these two to
>>>>>>> try to make my point: I'm struggling to associate the names with the
>>>>>>> values they are set to. Likely "hxw" is an abbreviation of hart index
>>>>>>> width, but (a) what's the leading 'l' then and (b) why is there no 'g'
>>>>>>> in "hhxw"? By using hard to grasp names, you make it hard to actually
>>>>>>> understand the subsequent expressions, in particular ...
>>>>>>
>>>>>> The names it taken directly from AIA spec:
>>>>>>
>>>>>> The use of this value and fields HHXS (High Hart Index Shift), LHXS (Low
>>>>>> Hart Index Shift), HHXW (High Hart Index Width), and LHXW (Low Hart
>>>>>> Index Width) for determining target addresses for MSIs is described
>>>>>> later, in Section 4.9.1.
>>>>>>
>>>>>> The AIA specification interprets the machine-level hart index as a
>>>>>> combination of the **group index** (`g`) and the **hart index within the
>>>>>> group** (`h`), according to the following formulas:
>>>>>>
>>>>>> ```
>>>>>> (1) g = (machine-level hart index >> LHXW) & (2^HHXW − 1)
>>>>>> (2) h = machine-level hart index & (2^LHXW − 1)
>>>>>> ```
>>>>>>
>>>>>> (In our case, the machine-level hart index is equal to `mhartid`, i.e.
>>>>>> the hart index.)
>>>>> Therefore, if I understand correclty, if we take the Hart Index as
>>>>> defined in the AIA spec, we should have:
>>>>> Hart Index = (g << LHXW) | h
>>>>> Is it correct?
>>>>
>>>> Yes.
>>>>
>>>> But note that in the current version of aplic_hart_field(), hart_id is
>>>> passed directly, so there is no need to extract h as described in the
>>>> AIA specification. We only need to concatenate it with the group index
>>>> that we have already extracted.
>>>>
>>>> This is partly because aplic_hart_field() uses only .base_addr, which
>>>> does not contain hart_index.
>>>>
>>>> If we want to follow the AIA specification fully, using its terminology,
>>>> the code should look something like:
>>>>
>>>> static unsigned long aplic_hart_field(unsigned int cpu)
>>>> {
>>>>        const struct imsic_config *imsic = imsic_get_config();
>>>>        const struct imsic_msi *msi = &imsic->msi[cpu];
>>> Could you please specify how this function will be used and when? It's
>>> hard for me to understand how imsic->msi[cpu] is filled.
>>
>> imsic->msi[] is filled during IMSIC initialization in imsic_init(),
>> based on the MMIO regset specified in the IMSIC node’s reg property and
>> the number of parents specified in the interrupts-extended property.
>> This is explained to some extent in the comment above local target_addr
>> in aplic_hart_field() (a little further down).
>>
>> I am not 100% sure that I fully understand the connection between your
>> question and the sentence after it, but I planned to write the following
>> above the function declaration:
>>
>>
>> /*
>>    * The arrangement of IMSIC interrupt files in MMIO space follows a
>> topology
>>    * defined by the RISC-V AIA specification. An IMSIC group is a set of
>>    * interrupt files (e.g., in a cluster or socket) co-located in memory.
>>    *
>>    * The physical address of an outgoing MSI is calculated by bitwise ORing a
>>    * Base Physical Page Number (Base PPN) with the Group Index (g), the Hart
>>    * Index (h) and, for a supervisor-level interrupt domain, the Guest Index:
>>    *
>>    *   ( Base PPN | (g << (HHXS + 12)) | (h << LHXS) | guest ) << 12
>>    *
>>    * where Base PPN, HHXS, LHXS, HHXW and LHXW come from the
>> {m,s}msiaddrcfg[h]
>>    * registers of the interrupt domain that sends the MSI:
>>    *
>>    * XLEN-1           HHXS+24             LHXS+12          12          0
>>    * |                |                   |                |           |
>>    * -------------------------------------------------------------------
>>    * |xxxx|Group Index|xxxxxxxx|Hart Index|xxxx|Guest Index|     0     |
>>    * -------------------------------------------------------------------
>>    *
>>    * - xxxx: the remaining bits of the Base PPN. The specification
>> requires the
>>    *   Base PPN to have zeros in the positions where the indices are OR-ed.
>>    * - Group Index (g): placed at bit (HHXS + 24) of the physical address.
>>    * - Hart Index (h): placed at bit (LHXS + 12) of the physical address.
>>    * - Guest Index: selects one of the 4 KiB pages right above the hart's own
>>    *   supervisor-level file, i.e. it starts at bit 12; LHXS must
>> therefore be
>>    *   at least as large as the number of guest index bits.
> 
> I think the name `Hart Index` is confusing here. In fact, you previously
> confirmed it refers to target[i] bits 31:18, i.e. the packed number
> (g << LHXW) | h, but here you say `Hart Index` is equivalent to h, which
> makes no sense.
> 
> I know this diagram came from Linux (Anup Patel, Nov 2022,

Not really, this diagram was created from scratch. I think you are 
referring to that one in struct imsic_config but the idea is the same 
and the comment in struct imsic_config should be fixed too. I will 
re-use what we agreed here.

> https://lore.kernel.org/all/[email protected]/),
> where "HART Index" is simply the name of the riscv,hart-index-bits DT
> property. Linux's own APLIC driver then reuses a single hart_index
> variable for h and for (g << LHXW) | h in consecutive lines, without a
> comment, which is confusing - if I understand correctly, obviously :)
> 
> I think this diagram could be better aligned with the AIA spec:
> 
>      * XLEN-1       HHXS+24          LHXS+12          12          0
>      * |            |                |                |           |
>      * ------------------------------------------------------------
>      * |xxxx|   g   |xxxxxxxx|   h   |xxxx|Guest Index|     0     |
>      * ------------------------------------------------------------
>      *
>      * - g: group number
>      * - h: hart number relative to the group
>      * - xxxx: remaining Base PPN bits; each gap may be zero-width.
> 
> What do you think? It would allow us to keep a single meaning for the
> `Hart Index` field, the same one as target[i] bits 31:18 i.e. (g <<
> LHXW) | h.

I agree g and h better describes AIA spec and probably will be easier to 
do a grep in AIA spec.

Thanks.

~ Oleksii