[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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/20240307140307.646078-3-apatel@xxxxxxxxxxxxxxxx/),
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

 

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.

Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.