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

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

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.
>      unsigned int lhxs = imsic->guest_index_bits;
>      unsigned int lhxw = imsic->hart_index_bits;
>      unsigned int hhxw = imsic->group_index_bits;
>      unsigned int hhxs =
>          imsic->group_index_shift - APLIC_xMSICFGADDR_PPN_SHIFT * 2;
>      /*
>       * msi->base_addr is the base of the MMIO regset this CPU's interrupt
>       * files live in, and one regset can cover several harts; msi->offset
>       * selects this CPU's block inside it.  The hart index bits are part of
>       * that offset, so both indexes have to be derived from the full 
> address.
>       */
>      paddr_t target_addr = msi->base_addr + msi->offset;
>      unsigned long tppn = target_addr >> APLIC_xMSICFGADDR_PPN_SHIFT;
>      unsigned long group_index =
>          (tppn >> APLIC_xMSICFGADDR_PPN_HHX_SHIFT(hhxs)) &
>          APLIC_xMSICFGADDR_PPN_HHX_MASK(hhxw);
>      unsigned long hart_index =
>          (tppn >> APLIC_xMSICFGADDR_PPN_LHX_SHIFT(lhxs)) &
>          APLIC_xMSICFGADDR_PPN_LHX_MASK(lhxw);
> 
>      return (group_index << lhxw) | hart_index;
> }
> 
> (note that during writing that I found an issue, it should be really 
> passed Xen cpu id, not hartid as msi[] is iterated through Xen cpu id so 
> I've taken that into account when wrote an implementation mentioned above)
> 
> Generally I think I am okay with both version of how to get hart_index 
> (or pass it by an argument or extract it).
> 
> >>
> >> For systems that use IMSIC groups, the IMSIC address layout is defined
> >> by the following parameters:
> >>
> >> * `lhxw` (Low Hart Index Width, or *k*): the number of bits used for the
> >> hart number within a group.
> >> * `hhxw` (High Hart Index Width, or *j*): the number of bits used for
> >> the group number.
> > Is group number appelation equivalent to group index?
> > 
> > I think with if what I wrote above is correct, the proper definition for
> > `hhxw` and `hhxs` should be:
> > * `hhxw` (High Hart Index Width, or *j*): the number of bits used for
> > the `Hart Index` field within the physical address.
> >> * `hhxs` (High Hart Index Shift): the bit offset of the combined
> >> hart/group index field within the physical address.
> > * `hhxs` (High Hart Index Shift): the bit offset of the `Hart Index`
> > field within the physical address.
> >> To extract the group index, we first shift the address by `hhxs` so that
> >> the group index bits are aligned, and then apply a mask derived from
> >> `hhxw` to isolate those bits.
> >>
> >> The hardware performs the same operation to extract the hart index from
> >> the MSI address. However, in our case we already know which hart should
> >> receive the interrupt (`hartid`), so there is no need to extract the
> >> hart index from the base address. We only need to recover the group
> >> index and combine it with `hartid` to construct the value expected by
> >> the `target` register.
> > 
> > Why don't we direclty extract the Hart Index as target directly needs it
> > as explained in the 4.5.16.2 point of the AIA spec:
> > target[31:18] = Hart Index
> > target[17:12] = Guest Index
> > target[10:0] = EEID
> > It'd be easier as we just have to do shift from HHXS and apply HHXW.
> 
>  From IMSIC's DT-binding description we have:
> 
>    XLEN-1            > (HART Index MSB)                  12    0
>    |                  |                                  |     |
>    -------------------------------------------------------------
>    |xxxxxx|Group Index|xxxxxxxxxxx|HART Index|Guest Index|  0  |
>    -------------------------------------------------------------
> 
> If you see there is a set of "xxxxxx" between HART and Group Indexes 
I think I'm missunderstanding the spec, as I wrote before I thought that
[1] `Hart index` = group_idx << LHXW | hart_idx_within_the_group so,
does the Hart Index in the schema refer to hart_idx_within_the_group or
to [1]? The naming makes me a bit confuse.
> that is the reason why we have to extract HART and Group Index 
> separately as when h/w will work with target register it doesn't know 
> about "xxxxx" at all so from h/w point of view target's register hart 
> field looks like |Group Index|Hart Index|. In other words, h/w will do
> the following with TARGET's hart index field:
>      group_idx = hart_idx >> lhxw;
>      hart_idx &= APLIC_xMSICFGADDR_PPN_LHX_MASK(lhxw);
> 
> and then embed group_idx and hart_idx into the structure above.
> 
> Does it make sense?
> 
> > 
> > I will try to draw some schema to make the AIA spec more explicit. Maybe
> > it could be part of this series, I don't know what is the xen policy
> > about diagram and stuff like that. Do you know more about that?
> 
> Unfortunately, no, I don't.
> 
> > In order 
> > to not do a job with no needed at all.
> > 
> 
> IMO, it is enough only AIA spec here to understand. At least, it is 
> clear to me.
> 
> ~ Oleksii
> 
> 
>